[Libguestfs] Supporting btrfs subvolumes during inspection

[Matthew Booth]

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-machine-disk-image
>
> 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