[PATCH] inspector: Fix virt-inspector on *BSD guests (RHBZ#1144138).
--- inspector/inspector.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/inspector/inspector.c b/inspector/inspector.c index 3583c61df..30d279987 100644 --- a/inspector/inspector.c +++ b/inspector/inspector.c @@ -347,6 +347,7 @@ output_root (xmlTextWriterPtr xo, char *root) char buf[32]; char *canonical_root; size_t size; + int is_bsd; XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "operatingsystem")); @@ -362,6 +363,10 @@ output_root (xmlTextWriterPtr xo, char *root) if (STRNEQ (str, "unknown")) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "name", BAD_CAST str)); + is_bsd = + STREQ (str, "freebsd") || + STREQ (str, "netbsd") || + STREQ (str, "openbsd"); free (str); str = guestfs_inspect_get_arch (g, root); @@ -451,8 +456,13 @@ output_root (xmlTextWriterPtr xo, char *root) /* 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. */ - if (inspect_apps || inspect_icon) { + if ((inspect_apps || inspect_icon) && !is_bsd) { inspect_mount_root (g, root); if (inspect_apps) -- 2.13.2
On Thursday, 5 October 2017 17:36:09 CEST Richard W.M. Jones wrote:
--- inspector/inspector.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/inspector/inspector.c b/inspector/inspector.c index 3583c61df..30d279987 100644 --- a/inspector/inspector.c +++ b/inspector/inspector.c @@ -347,6 +347,7 @@ output_root (xmlTextWriterPtr xo, char *root) char buf[32]; char *canonical_root; size_t size; + int is_bsd;
XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "operatingsystem"));
@@ -362,6 +363,10 @@ output_root (xmlTextWriterPtr xo, char *root) if (STRNEQ (str, "unknown")) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "name", BAD_CAST str)); + is_bsd = + STREQ (str, "freebsd") || + STREQ (str, "netbsd") || + STREQ (str, "openbsd"); free (str);
str = guestfs_inspect_get_arch (g, root); @@ -451,8 +456,13 @@ output_root (xmlTextWriterPtr xo, char *root)
/* 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. The trick here is what check_for_filesystem_on in daemon/inspect_fs.ml does, by trying few mount options for ufs filesystems: usually one of those attempts succeeds, so we extract the information from that partition. Later on in virt-inspector, though, these extra mount options are not retained, so it fails. The two possible options I had in mind were: a) duplicate the same mount logic also in virt-inspector, which is ugly, duplicates code, etc b) save these extra mount options, exposing them via a new inspect_* API: IMHO it is ugly as well, since it would "make official" the extra mount attempts hack tried in inspect_fs.ml but both are ugly, in one way or another, so I did not implement either of them. Anyway, IMHO this patch is not the correct approach. -- Pino Toscano
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:
--- inspector/inspector.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/inspector/inspector.c b/inspector/inspector.c index 3583c61df..30d279987 100644 --- a/inspector/inspector.c +++ b/inspector/inspector.c @@ -347,6 +347,7 @@ output_root (xmlTextWriterPtr xo, char *root) char buf[32]; char *canonical_root; size_t size; + int is_bsd;
XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "operatingsystem"));
@@ -362,6 +363,10 @@ output_root (xmlTextWriterPtr xo, char *root) if (STRNEQ (str, "unknown")) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "name", BAD_CAST str)); + is_bsd = + STREQ (str, "freebsd") || + STREQ (str, "netbsd") || + STREQ (str, "openbsd"); free (str);
str = guestfs_inspect_get_arch (g, root); @@ -451,8 +456,13 @@ output_root (xmlTextWriterPtr xo, char *root)
/* 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 - 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. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
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:
--- inspector/inspector.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/inspector/inspector.c b/inspector/inspector.c index 3583c61df..30d279987 100644 --- a/inspector/inspector.c +++ b/inspector/inspector.c @@ -347,6 +347,7 @@ output_root (xmlTextWriterPtr xo, char *root) char buf[32]; char *canonical_root; size_t size; + int is_bsd;
XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "operatingsystem"));
@@ -362,6 +363,10 @@ output_root (xmlTextWriterPtr xo, char *root) if (STRNEQ (str, "unknown")) XMLERROR (-1, xmlTextWriterWriteElement (xo, BAD_CAST "name", BAD_CAST str)); + is_bsd = + STREQ (str, "freebsd") || + STREQ (str, "netbsd") || + STREQ (str, "openbsd"); free (str);
str = guestfs_inspect_get_arch (g, root); @@ -451,8 +456,13 @@ output_root (xmlTextWriterPtr xo, char *root)
/* 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. (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. 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. -- Pino Toscano
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
participants (2)
-
Pino Toscano -
Richard W.M. Jones