Libguestfs - Feb 2016 - Displaying mountables in the error output of guestfish etc

Cédric pointed out this problem we have:

$ guestmount -a /var/lib/libvirt/images/sles12sp1-pv.img -m
'/dev/sda1:/:subvol=.snapshots/2/snapshot:btrfs' /mnt
libguestfs: error: mount_vfs: /dev/sda1 on / (options:
'subvol=.snapshots/2/snapshot'): mount: mount(2) failed: No such file or
directory
guestmount: '/dev/sda1' could not be mounted.
guestmount: Check mount(8) man page to ensure options
'subvol=.snapshots/2/snapshot'
guestmount: are supported by the filesystem that is being mounted.
guestmount: Did you mean to mount one of these filesystems?
guestmount:     btrfsvol:/dev/sda1/@ (btrfs)
guestmount:     btrfsvol:/dev/sda1/@/.snapshots (btrfs)
guestmount:     btrfsvol:/dev/sda1/@/opt (btrfs)
guestmount:     btrfsvol:/dev/sda1/@/srv (btrfs)
guestmount:     btrfsvol:/dev/sda1/@/tmp (btrfs)
guestmount:     btrfsvol:/dev/sda1/@/home (btrfs)
[etc]

The problem here is the strings btrfsvol:... are the internal
"mountable" format, and shouldn't be displayed to end users for a
couple of reasons:

(1) It's not intended that people would "know about" these
strings.
You're only compliant with the API if you get a string from an API
like guestfs_list_filesystems and pass it to another API like
guestfs_mount.

(2) These strings aren't actually useful, because you cannot pass them
back to the guestfish/guestmount `-m' option.  Instead you have to use
the non-obvious form:

  -m /dev/sda1:/:subvol=/@/tmp

How do we solve this?  Trickier than it seems.

The first attempt to Cédric tried was to call
guestfs_internal_parse_mountable which conveniently splits the
internal string into a device and a volume.  As the name suggests,
that is an internal API.  We shouldn't be calling this from our tools,
and in any case it doesn't work unless we define -DGUESTFS_PRIVATE=1
all over the place.  So that's a bad idea.

I think probably the way to solve this is with new public APIs for
returning the device and volume name from a mountable.  Similar to
guestfs_internal_parse_mountable in fact, but a public API that we
commit to.

  char *device = guestfs_mountable_device (g, const char *mountable);

  char *volume = guestfs_mountable_volume (g, const char *mountable);

If a mountable doesn't have a (sub-)volume, the second API should
probably return a well-defined errno (ESRCH or ENOENT?).

The code in fish/options.c then just has to:

  foreach fs in guestfs_list_filesystems:
    device = guestfs_mountable_device fs
    volume = guestfs_mountable_volume fs
    if guestfs_mountable_volume fs returned well defined errno:
      printf "-m %s" device
    else
      printf "-m %s:/:subvol=%s" device volume

That's my idea anyway ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Friday 26 February 2016 16:50:56 Richard W.M. Jones
wrote:
> 
> Cédric pointed out this problem we have:
> 
> $ guestmount -a /var/lib/libvirt/images/sles12sp1-pv.img -m
'/dev/sda1:/:subvol=.snapshots/2/snapshot:btrfs' /mnt
> libguestfs: error: mount_vfs: /dev/sda1 on / (options:
'subvol=.snapshots/2/snapshot'): mount: mount(2) failed: No such file or
directory
> guestmount: '/dev/sda1' could not be mounted.
> guestmount: Check mount(8) man page to ensure options
'subvol=.snapshots/2/snapshot'
> guestmount: are supported by the filesystem that is being mounted.
> guestmount: Did you mean to mount one of these filesystems?
> guestmount:     btrfsvol:/dev/sda1/@ (btrfs)
> guestmount:     btrfsvol:/dev/sda1/@/.snapshots (btrfs)
> guestmount:     btrfsvol:/dev/sda1/@/opt (btrfs)
> guestmount:     btrfsvol:/dev/sda1/@/srv (btrfs)
> guestmount:     btrfsvol:/dev/sda1/@/tmp (btrfs)
> guestmount:     btrfsvol:/dev/sda1/@/home (btrfs)
> [etc]
> 
> The problem here is the strings btrfsvol:... are the internal
> "mountable" format, and shouldn't be displayed to end users
for a
> couple of reasons:
> 
> (1) It's not intended that people would "know about" these
strings.
> You're only compliant with the API if you get a string from an API
> like guestfs_list_filesystems and pass it to another API like
> guestfs_mount.
> 
> (2) These strings aren't actually useful, because you cannot pass them
> back to the guestfish/guestmount `-m' option.  Instead you have to use
> the non-obvious form:
> 
>   -m /dev/sda1:/:subvol=/@/tmp
> 
> How do we solve this?  Trickier than it seems.
> 
> The first attempt to Cédric tried was to call
> guestfs_internal_parse_mountable which conveniently splits the
> internal string into a device and a volume.  As the name suggests,
> that is an internal API.  We shouldn't be calling this from our tools,
> and in any case it doesn't work unless we define -DGUESTFS_PRIVATE=1
> all over the place.  So that's a bad idea.
> 
> I think probably the way to solve this is with new public APIs for
> returning the device and volume name from a mountable.  Similar to
> guestfs_internal_parse_mountable in fact, but a public API that we
> commit to.
> 
>   char *device = guestfs_mountable_device (g, const char *mountable);
> 
>   char *volume = guestfs_mountable_volume (g, const char *mountable);
> 
> If a mountable doesn't have a (sub-)volume, the second API should
> probably return a well-defined errno (ESRCH or ENOENT?).
Sounds good; I'd just do
- guestfs_mountable_volume -> guestfs_mountable_subvolume
- EINVAL as return value of the above in case of error
> The code in fish/options.c then just has to:
> 
>   foreach fs in guestfs_list_filesystems:
>     device = guestfs_mountable_device fs
>     volume = guestfs_mountable_volume fs
>     if guestfs_mountable_volume fs returned well defined errno:
>       printf "-m %s" device
>     else
>       printf "-m %s:/:subvol=%s" device volume
... switching the order of the checks:

  if subvolume:
    printf "-m %s:/:subvol=%s" device subvolume
  else
    printf "-m %s" device

this way, if there will be more "subparts" in the filesystem
identifiers, they can be added as cascading conditions in the if.

-- 
Pino Toscano