On Mon, Mar 11, 2024 at 07:32:30PM +0200, Andrey Drobyshev wrote: > 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/]
We had a bit of a discussion here about this image, which we think is a Hybrid ISO generated by either this exact tool or a similar one: https://wiki.syslinux.org/wiki/index.php?title=Isohybrid The image contains ISO with MBR + GPT partition table. For some reason sgdisk cannot seem to read the GPT partition table properly, so either it's badly formatted or maybe there is a bug in sgdisk: This hybrid ISO image: $ file /var/tmp/Rocky-9.3-x86_64-minimal.iso /var/tmp/Rocky-9.3-x86_64-minimal.iso: ISO 9660 CD-ROM filesystem data (DOS/MBR boot sector) 'Rocky-9-3-x86_64-dvd' (bootable) Just some regular ISO: $ file /mnt/media/software/vmware/VMware-VCSA-all-7.0.0-15952498.iso /mnt/media/software/vmware/VMware-VCSA-all-7.0.0-15952498.iso: ISO 9660 CD-ROM filesystem data 'VMware VCSA' Linux kernel and sfdisk can parse the MBR: $ virt-rescue --ro -a /var/tmp/Rocky-9.3-x86_64-minimal.iso ><rescue> dmesg | grep sda: [ 0.520088] sda: sda1 sda2 $ sfdisk -l /var/tmp/Rocky-9.3-x86_64-minimal.iso Disk /var/tmp/Rocky-9.3-x86_64-minimal.iso: 1.57 GiB, 1686568960 bytes, 3294080 sectors ... Device Boot Start End Sectors Size Id Type /var/tmp/Rocky-9.3-x86_64-minimal.iso1 * 0 3293407 3293408 1.6G 0 Empty /var/tmp/Rocky-9.3-x86_64-minimal.iso2 664 14851 14188 6.9M ef EFI ( But sgdisk cannot: $ sgdisk /var/tmp/Rocky-9.3-x86_64-minimal.iso -i 1 Invalid partition data! I just wonder if a better way to do this is to ignore partitions that appear on a hybrid ISO by actually detecting hybrid ISO as a specific format (rather than hard coding "iso9660" or "udf"). Your patch tries to modify list_filesystems, but this could also be done in list_partitions. Still thinking about this ... Do we have a bug about it? I think a key issue is why does sgdisk not recognise the partition table. Rich. > >> # 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/729d6d55ea84494f0398d02450bd29c39c55f0bd/daemon/devsparts.ml#L52 > > > > 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/729d6d55ea84494f0398d02450bd29c39c55f0bd/daemon/parted.ml#L167 > > > > 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.drobys...@virtuozzo.com> > 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.drobys...@virtuozzo.com> > --- > 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 > -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
