[Libguestfs] Re: [libguestfs BUG] ISO images can't be inspected properly

On 3/7/24 16:27, Richard W.M. Jones wrote: > On Thu, Feb 29, 2024 at 05:37:08PM +0200, Andrey Drobyshev wrote: >> I discovered that when a VM has an ISO attached via CD-ROM, libguestfs >> fails to read its partition table and therefore fails to perform basic >> operations on a VM, e.g. listing a directory or editing a file: >> >> # virsh domblklist rocky9-vm >> Target Source >> --------------------------------------------------------------------------- >> sda /vz/vmprivate/e827caad-b9e4-4858-b3dc-53ae6b8a5145/harddisk.hdd >> sdb /vzt/images/Rocky-9.3-x86_64-minimal.iso > > [Download the ISO from here: https://rockylinux.org/download/]

<rescue> dmesg | grep sda:

>> # virt-ls -d rocky9-vm / >> libguestfs: error: inspect_os: sgdisk: Invalid partition data! >> >> # virt-edit -d rocky9-vm /etc/fstab >> libguestfs: error: inspect_os: sgdisk: Invalid partition data! >> >> >> Even virt-inspector can't give us proper output on an ISO9660 image: >> >> # virt-inspector /vzt/images/Rocky-9.3-x86_64-minimal.iso >> libguestfs: error: inspect_os: sgdisk: Invalid partition data! >> virt-inspector: no operating system could be detected inside this disk >> image. >> ... >> >> >> First I found a 3 years old topic where you suggested that sgdisk itself >> might be too old: >> https://listman.redhat.com/archives/libguestfs/2021-March/025939.html. >> However this doesn't seem to be it in this case: >> I've compiled the latest version from >> https://sourceforge.net/projects/gptfdisk -- no luck as well. Seems >> that sgdisk isn't capable of recognizing ISO9660 images. >> >> Then I noticed 2 things: >> >> 1. libguestfs appliance is being created (with backend=libvirt) with ISO >> attached as device='disk' (not 'cdrom'): >> >>> <disk type='file' device='disk'> >>> <driver name='qemu' type='qcow2' cache='unsafe'/> >>> <source file='/tmp/libguestfsD7IHvF/overlay2.qcow2' index='2'/> >>> <backingStore type='file' index='5'> >>> <format type='raw'/> >>> <source file='/vzt/images/Rocky-9.3-x86_64-minimal.iso'/> >>> <backingStore/> >>> </backingStore> >>> <target dev='sdb' bus='scsi'/> >>> <alias name='scsi0-0-1-0'/> >>> <address type='drive' controller='0' bus='0' target='1' unit='0'/> >>> </disk> >> >> That forces this disk to be supplied with "scsi-hd" driver in use, which >> in turn leads to the device being visible inside the guest as a regular >> SCSI hdd /dev/sdb. Shouldn't we, for instance, make sure that for such >> images device='cdrom' and "scsi-cd" driver are being used? In this case >> the guest will see it as /dev/sr0, and I suspect srX devices are being >> skipped by guestfs_inspect_os. > > I think /dev/srX would not be ignored: > > https://github.com/libguestfs/libguestfs/blob/729d6d55ea84494f0398d02450b... > > But anyway I don't think that's the problem here. We try to present > any disk image (even an ISO) as a regular disk to the libguestfs > appliance. That's because we don't need or care about the special > features of /dev/srX like the ability to eject a disk. > > I think the central problem is why libguestfs cannot inspect the ISO. > > $ virt-inspector -a Rocky-9.3-x86_64-minimal.iso > libguestfs: error: inspect_os: sgdisk: Invalid partition data! > virt-inspector: no operating system could be detected inside this disk image. > > $ guestfish --ro -a Rocky-9.3-x86_64-minimal.iso -i > libguestfs: error: inspect_os: sgdisk: Invalid partition data! > > etc > > Adding -vx options shows the problem is with sgdisk: > > command: sgdisk returned 2 > command: sgdisk: stderr: > Invalid partition data! > ocaml_exn: 'inspect_os' raised 'Failure' exception > guestfsd: error: sgdisk: Invalid partition data! > guestfsd: => inspect_os (0x1e0) took 0.13 secs > > I believe this comes from: > > https://github.com/libguestfs/libguestfs/blob/729d6d55ea84494f0398d02450b... > > I modified the error message in this file to see what it's trying to > do and it comes from the call to 'part_get_gpt_type'. > > I think it's a regression in commit b699111e04. > Just to be accurate here: commit b699111e04 ("daemon: list-filesystems: Filter out MBR extended partitions.") hasn't introduced the call to Parted.part_get_gpt_type, it was done by the commit 16192ad95 ("daemon: list-filesystems: Change the way we filter out LDM partitions.") coming from the same series. > Do you want to try the attached patch? > Thank you, I've tried the patch and ignoring the sgdisk error does help to overcome the failure when working with domains having an ISO attached. However I'm not convinced that simply ignoring the error is the right thing to do: what if there's an error on other than ISO, where we do indeed want to be aware of it? I think we should rather make an exception for a CD/DVD images. AFAIU ISO9660 images', as well as UDF images' structures aren't supposed to have partition tables. Rather these formats fool tools like sgdisk, parted etc. into thinking there're actual partition tables. If so, why don't we just skip listing separate partitions and look at the image as a whole when listing file systems. What do you think of the attached patch? > Note this still won't allow the ISO to be inspected, but it avoids the > "Invalid partition data!" error, and maybe will cure the original > problem above. > You're right, inspection still doesn't work properly (with my patch as well). Now the commands fail further: $ guestfish --ro -a /vzt/images/Rocky-9.3-x86_64-minimal.iso -i guestfish: no operating system was found on this disk If using guestfish ‘-i’ option, remove this option and instead use the commands ‘run’ followed by ‘list-filesystems’. You can then mount filesystems you want by hand using the ‘mount’ or ‘mount-ro’ command. ..... $ virt-ls -a /vzt/images/Rocky-9.3-x86_64-minimal.iso / virt-ls: no operating system was found on this disk If using guestfish ‘-i’ option, remove this option and instead use the commands ‘run’ followed by ‘list-filesystems’. You can then mount filesystems you want by hand using the ‘mount’ or ‘mount-ro’ command. ..... That seems to be because there's no <root> element in the inspection result: $ virt-inspector -a /vzt/images/Rocky-9.3-x86_64-minimal.iso <?xml version="1.0"?> <operatingsystems/> Shouldn't we tweak inspect_get_roots() (from daemon/inspect.ml) to recognize ISO9660 as well? >> 2. When trying to perform the same operation via guestfish, everything >> appears to be working: >> >> # guestfish -a /vzt/images/Rocky-9.3-x86_64-minimal.iso -m /dev/sda ls / >>> /dev/null && echo $? >> 0 > > I think this is just because we're avoiding inspection? > >> File edit works as well. From debug output I see that sgdisk isn't >> being invoked at all. That means there's a code path for the same >> operations where sgdisk can be omitted. Maybe we should follow that >> same code path when invoking other tools like virt-ls or virt-edit? >> >> >> Overall, what would be an appropriate fix in your opinion? We should at >> least make sure that working with domains (with the "-d" option) works >> no matter what. >> >> Andrey > > Rich. >

From 2a703ca4abb93fce76985afc34052e1372939ba0 Mon Sep 17 00:00:00 2001 From: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; Date: Mon, 11 Mar 2024 19:13:26 +0200 Subject: [PATCH] daemon: listfs: Ignore partitions when dealing with ISO9660 or UDF ISO9660 or UDF images (CD/DVD) don't really have partition table, however tools like sgdisk might think otherwise. Currently when working with a domain having a cdrom attached, we get: $ virt-ls -d VM / guestfsd: error: sgdisk: Invalid partition data! Let's detect filesystem type of such a block device (with blkid) and treat it as a whole in case it has type "iso9660" or "udf", e.g. ignoring what appears to be separate partitions. Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; --- daemon/listfs.ml | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/daemon/listfs.ml b/daemon/listfs.ml index 3c6e86fa5..15422be78 100644 --- a/daemon/listfs.ml +++ b/daemon/listfs.ml @@ -43,8 +43,17 @@ let rec list_filesystems () = let devices = List.filter is_not_partitioned_device devices in List.iter (check_with_vfs_type ret) devices; + (* CD or DVD devices (ISO9660 or UDF). + * We want to treat such devices as a whole, even if it appears + * to have a partition table. + *) + let devices = Devsparts.list_devices () in + let devices = List.filter is_device_cd_or_dvd devices in + List.iter (check_with_vfs_type ret) devices; + (* Partitions. *) let partitions = Devsparts.list_partitions () in + let partitions = List.filter is_not_fake_cd_or_dvd_partition partitions in let partitions = List.filter is_partition_can_hold_filesystem partitions in List.iter (check_with_vfs_type ret) partitions; @@ -138,11 +147,7 @@ and is_mbr_extended parttype device partnum = and is_mbr_bogus parttype device partnum = parttype = "msdos" && partnum = 1 && Utils.has_bogus_mbr device -(* Use vfs-type to check for a filesystem of some sort of [device]. - * Appends (device, vfs_type) to the ret parameter (there may be - * multiple devices found in the case of btrfs). - *) -and check_with_vfs_type ret device = +and device_to_mountable_vfstype device = let mountable = Mountable.of_device device in let vfs_type = try Blkid.vfs_type mountable @@ -151,6 +156,22 @@ and check_with_vfs_type ret device = eprintf "check_with_vfs_type: %s: %s\n" device (Printexc.to_string exn); "" in + (mountable, vfs_type) + +and is_device_cd_or_dvd device = + let mountable, vfs_type = device_to_mountable_vfstype device in + vfs_type = "iso9660" || vfs_type = "udf" + +and is_not_fake_cd_or_dvd_partition partition = + let device = Devsparts.part_to_dev partition in + not (is_device_cd_or_dvd device) + +(* Use vfs-type to check for a filesystem of some sort of [device]. + * Appends (device, vfs_type) to the ret parameter (there may be + * multiple devices found in the case of btrfs). + *) +and check_with_vfs_type ret device = + let mountable, vfs_type = device_to_mountable_vfstype device in if vfs_type = "" then List.push_back ret (mountable, "unknown") -- 2.39.3