Libguestfs - Oct 2017 - Re: [PATCH] inspector: Fix virt-inspector on *BSD guests (RHBZ#1144138).

On Fri, Oct 06, 2017 at 09:53:27AM +0200, Pino Toscano
wrote:
> On Thursday, 5 October 2017 23:49:26 CEST Richard W.M. Jones wrote:
> > On Thu, Oct 05, 2017 at 06:55:53PM +0200, Pino Toscano wrote:
> > > On Thursday, 5 October 2017 17:36:09 CEST Richard W.M. Jones
wrote:
> > > >    /* We need to mount everything up in order to read out
the list of
> > > >     * applications and the icon, ie. everything below this
point.
> > > > +   *
> > > > +   * XXX As a workaround for BSD guests, because the Linux
kernel
> > > > +   * driver cannot just mount a UFS filesystem, we must
disable this
> > > > +   * for all *BSD operating systems.  We cannot read the
apps or icon
> > > > +   * from *BSD anyway.
> > > >     */
> > > 
> > > This is not true, libguestfs can actually read those properties. 
The
> > > proof of that is running virt-inspector with --no-applications,
and
> > > --no-icon shows the proper details (such as product name, mount
points,
> > > host name, etc) of the guest.
> > 
> > I'm a bit confused by this comment
> 
> I thought this was a workaround to the fact that mounting UFS partitions
> can fail, without particular mount options:
> 
>   $ ./run virt-inspector -a freebsd-10.qcow2
>   libguestfs: error: mount_ro: mount exited with status 32: mount:
/sysroot: wrong fs type, bad option, bad superblock on /dev/sda2, missing
codepage or helper program, or other error.
Correct.
> (this happens in inspect_mount_root, when mounting it to get the icon
> of the guest, and to list the applications)
> 
> > - libguestfs inspection doesn't
> > know anything about apps and icons from *BSD as far as I can see.
> > This patch just (temporarily) makes it so that virt-inspection acts
> > like --no-applications --no-icon if it sees as *BSD guest, until we
> > think of a better way to do it.
> 
> There are various guests in the same situation -- e.g. Gentoo, Pardus,
> Slackware.
But Gentoo, Pardus & Slackware don't fail in the mount and so don't
need any workaround.
> I do not see why we should special-case *BSD guests, since
> at most (short of fixing the re-mount issue explained above) there will
> be no information extracted.
This isn't correct.  With this patch, instead of virt-inspector
printing nothing and failing with the mount error, you get the basic
information:

$ ./run virt-inspector /var/tmp/freebsd-11.1
<?xml version="1.0"?>
<operatingsystems>
  <operatingsystem>
    <root>/dev/sda5</root>
    <name>freebsd</name>
    <arch>x86_64</arch>
    <distro>freebsd</distro>
    <major_version>0</major_version>
    <minor_version>0</minor_version>
    <hostname></hostname>
    <mountpoints>
      <mountpoint dev="/dev/sda5">/</mountpoint>
    </mountpoints>
    <filesystems>
      <filesystem dev="/dev/sda5">
        <type>ufs</type>
        <uuid>59d658ab7d5c137f</uuid>
      </filesystem>
      <filesystem dev="/dev/sda6"/>
    </filesystems>
  </operatingsystem>
</operatingsystems>

(There is still a problem with major = minor = 0 but I'm going to try
to fix that separately).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Friday, 6 October 2017 12:11:08 CEST Richard W.M. Jones
wrote:
> On Fri, Oct 06, 2017 at 09:53:27AM +0200, Pino Toscano wrote:
> > On Thursday, 5 October 2017 23:49:26 CEST Richard W.M. Jones wrote:
> > > On Thu, Oct 05, 2017 at 06:55:53PM +0200, Pino Toscano wrote:
> > > > On Thursday, 5 October 2017 17:36:09 CEST Richard W.M. Jones
wrote:
> > > > >    /* We need to mount everything up in order to read
out the list of
> > > > >     * applications and the icon, ie. everything below
this point.
> > > > > +   *
> > > > > +   * XXX As a workaround for BSD guests, because the
Linux kernel
> > > > > +   * driver cannot just mount a UFS filesystem, we
must disable this
> > > > > +   * for all *BSD operating systems.  We cannot read
the apps or icon
> > > > > +   * from *BSD anyway.
> > > > >     */
> > > > 
> > > > This is not true, libguestfs can actually read those
properties.  The
> > > > proof of that is running virt-inspector with
--no-applications, and
> > > > --no-icon shows the proper details (such as product name,
mount points,
> > > > host name, etc) of the guest.
> > > 
> > > I'm a bit confused by this comment
> > 
> > I thought this was a workaround to the fact that mounting UFS
partitions
> > can fail, without particular mount options:
> > 
> >   $ ./run virt-inspector -a freebsd-10.qcow2
> >   libguestfs: error: mount_ro: mount exited with status 32: mount:
/sysroot: wrong fs type, bad option, bad superblock on /dev/sda2, missing
codepage or helper program, or other error.
> 
> Correct.
As I said in my first reply to this email, this is the actual issue.
> > I do not see why we should special-case *BSD guests, since
> > at most (short of fixing the re-mount issue explained above) there
will
> > be no information extracted.
> 
> This isn't correct.  With this patch, instead of virt-inspector
> printing nothing and failing with the mount error, you get the basic
> information:
As I said already, using --no-application & --no-icon gives already the
same; also, banning *BSD systems entirely from detailed inspections
means that:
a) whenever UFS is fixed, we will probably forget to remove this code
b) if UFS is not the filesystem used, then this issue does not happen
   at all, and just no extra information are returned (but this is a
   different issue)

Also, the problem is not specific to virt-inspector: any tool that
relies on inspection, and then mounts the partitions according to that,
will face the same issue -- e.g. virt-ls, virt-cat, virt-customize, etc.

That's why I do not think adding a workaround only in virt-inspector
makes much sense per-se, since it will not deal with general issue wrt
UFS partitions.

-- 
Pino Toscano

On Fri, Oct 06, 2017 at 04:08:14PM +0200, Pino Toscano
wrote:
> On Friday, 6 October 2017 12:11:08 CEST Richard W.M. Jones wrote:
> > On Fri, Oct 06, 2017 at 09:53:27AM +0200, Pino Toscano wrote:
> > > On Thursday, 5 October 2017 23:49:26 CEST Richard W.M. Jones
wrote:
> > > > On Thu, Oct 05, 2017 at 06:55:53PM +0200, Pino Toscano
wrote:
> > > > > On Thursday, 5 October 2017 17:36:09 CEST Richard W.M.
Jones wrote:
> > > > > >    /* We need to mount everything up in order to
read out the list of
> > > > > >     * applications and the icon, ie. everything
below this point.
> > > > > > +   *
> > > > > > +   * XXX As a workaround for BSD guests, because
the Linux kernel
> > > > > > +   * driver cannot just mount a UFS filesystem,
we must disable this
> > > > > > +   * for all *BSD operating systems.  We cannot
read the apps or icon
> > > > > > +   * from *BSD anyway.
> > > > > >     */
> > > > > 
> > > > > This is not true, libguestfs can actually read those
properties.  The
> > > > > proof of that is running virt-inspector with
--no-applications, and
> > > > > --no-icon shows the proper details (such as product
name, mount points,
> > > > > host name, etc) of the guest.
> > > > 
> > > > I'm a bit confused by this comment
> > > 
> > > I thought this was a workaround to the fact that mounting UFS
partitions
> > > can fail, without particular mount options:
> > > 
> > >   $ ./run virt-inspector -a freebsd-10.qcow2
> > >   libguestfs: error: mount_ro: mount exited with status 32:
mount: /sysroot: wrong fs type, bad option, bad superblock on /dev/sda2, missing
codepage or helper program, or other error.
> > 
> > Correct.
> 
> As I said in my first reply to this email, this is the actual issue.
> 
> > > I do not see why we should special-case *BSD guests, since
> > > at most (short of fixing the re-mount issue explained above)
there will
> > > be no information extracted.
> > 
> > This isn't correct.  With this patch, instead of virt-inspector
> > printing nothing and failing with the mount error, you get the basic
> > information:
> 
> As I said already, using --no-application & --no-icon gives already the
> same; also, banning *BSD systems entirely from detailed inspections
> means that:
> a) whenever UFS is fixed, we will probably forget to remove this code
> b) if UFS is not the filesystem used, then this issue does not happen
>    at all, and just no extra information are returned (but this is a
>    different issue)
>
> Also, the problem is not specific to virt-inspector: any tool that
> relies on inspection, and then mounts the partitions according to that,
> will face the same issue -- e.g. virt-ls, virt-cat, virt-customize, etc.
> 
> That's why I do not think adding a workaround only in virt-inspector
> makes much sense per-se, since it will not deal with general issue wrt
> UFS partitions.
OK these are fair points.

I'll withdraw this patch.  I still don't really know how to fix
this properly but I'll think about it some more.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW