5:53 a.m.
On 21/12/12 11:20, Richard W.M. Jones wrote:
I'd like to think about what calling code would look like. I
think
there are two separate issues, which should be treated as separate
things.
(1) guestfs_inspect_os: What to return from inspect_os, given that
multiple roots may exist on a single device.
(2) guestfs_get_filesystems/mountpoints: How to return the list of
devices and mountpoints.
So treating them as separate issues ...
I don't think these are separate issues, and you provide the example
yourself below.
----------------------------------------
(1) guestfs_inspect_os: What to return from inspect_os, given that
multiple roots may exist on a single device.
I think we should treat the "root" strings as opaque. It doesn't
matter (except see below) what we return from inspect_os as long as
the same thing is recognized as a parameter by the other calls.
There is an exception, which is code that I've written for Windows
which assumes root == C: device, ie. it does:
roots = g.inspect_os ();
if #roots > 1 then die;
root = roots[0];
g.mount (root, "/");
To not break the API we really need to keep returning plain device
names for all existing (non-btrfs) OSes.
This is the example which suggests these are the same issue.
But since that code would break anyway when presented with a btrfs
OS,
we can extend the root string for those. We don't need a flag or a
change to the API types.
----------------------------------------
(2) guestfs_get_filesystems/mountpoints: How to return the list of
devices and mountpoints.
Existing code looks like this:
my @roots = $g->inspect_os ();
for my $root (@roots) {
my %mps = $g->inspect_get_mountpoints ($root);
my @mps = sort { length $a <=> length $b } (keys %mps);
for my $mp (@mps) {
$g->mount_ro ($mps{$mp}, $mp);
# ...
http://libguestfs.org/guestfs-perl.3.html#example-2:-inspect-a-virtual-ma...
If we modify get_mountpoints or even add a new get_mountpoints2 call,
then we push a bunch of complexity up to the user. The mount code has
to look like:
for ($mp, $subvol) (@mps) {
if (!$subvol) {
$g->mount_ro ($mps{$mp}, $mp);
} elsif ($g->vfs_type ($mp) eq "btrfs") {
$g->mount_options ("subvol=$subvol", $mps{$mp}, $mp);
} elsif ($g->vfs_type ($mp) eq "zfs") {
$g->mount_options ("something=$subvol", $mps{$mp}, $mp);
} else {
die "sorry we don't know how to mount this"
}
}
and that's not very nice.
However changing mount to understand more complex strings containing
subvolumes is also not great, and potentially impacts the whole API
(after all, *any* call which might refer to a filesystem such as
vfs_type, would have to be modified).
So I don't know about (2) yet. Something to keep thinking about.
Actually, I like the idea of extending mount and anything else which is
similarly impacted. Given that we will be extending the allowed input to
these functions, not their output, this would not be an API break.
I just did a quick run through of APIs which take a Device argument. I
think we could split these into APIs which genuinely take a device, and
those which expect something with a filesystem on it. It could maybe be
called a Mountable. Any API which expects a Mountable would accept the
extended format.
Again, if we're going to make this change I strongly recommend against
assuming that device+subvolume is sufficient. Creating something capable
of handing anything is a trivial extension at this point, but a
potentially large headache later.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490