[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] Supporting btrfs subvolumes during inspection



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]