<div dir="auto">Gentle ping</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 30, 2020, 14:25 Sam Eiderman <<a href="mailto:sameid@google.com">sameid@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yea, I noticed that commit, but since it was used in gpt too<br>
regardless of that commit, I decided not to mention this regression.<br>
<br>
I also noticed that list_filesystems() still works, this is probably<br>
due to filtering of devices (daemon/<a href="http://listfs.ml" rel="noreferrer noreferrer" target="_blank">listfs.ml</a>):<br>
<br>
"let devices = List.filter is_not_partitioned_device devices in"<br>
<br>
What do you have in mind here?<br>
<br>
On Tue, Jun 30, 2020 at 12:33 PM Pino Toscano <<a href="mailto:ptoscano@redhat.com" target="_blank" rel="noreferrer">ptoscano@redhat.com</a>> wrote:<br>
><br>
> On Tuesday, 30 June 2020 10:33:40 CEST Sam Eiderman wrote:<br>
> > By creating an empty disk and using it as the first disk of the vm (i.e.<br>
> > /dev/sda, /dev/sdb{1,2} contains the windows fses) we change the<br>
> > iteration order of the disks.<br>
> > This causes inspect_os() to fail since Parted returns a Unix_error if<br>
> > the device does not contain any partitions - fix this by handling this<br>
> > Unix_error.<br>
> ><br>
> > Signed-off-by: Sam Eiderman <<a href="mailto:sameid@google.com" target="_blank" rel="noreferrer">sameid@google.com</a>><br>
> > ---<br>
> >  daemon/<a href="http://inspect_fs_windows.ml" rel="noreferrer noreferrer" target="_blank">inspect_fs_windows.ml</a> | 11 ++++++++---<br>
> >  1 file changed, 8 insertions(+), 3 deletions(-)<br>
> ><br>
> > diff --git a/daemon/<a href="http://inspect_fs_windows.ml" rel="noreferrer noreferrer" target="_blank">inspect_fs_windows.ml</a> b/daemon/<a href="http://inspect_fs_windows.ml" rel="noreferrer noreferrer" target="_blank">inspect_fs_windows.ml</a><br>
> > index c4a05bc38..bc6b98b60 100644<br>
> > --- a/daemon/<a href="http://inspect_fs_windows.ml" rel="noreferrer noreferrer" target="_blank">inspect_fs_windows.ml</a><br>
> > +++ b/daemon/<a href="http://inspect_fs_windows.ml" rel="noreferrer noreferrer" target="_blank">inspect_fs_windows.ml</a><br>
> > @@ -365,8 +365,10 @@ and map_registry_disk_blob_mbr devices blob =<br>
> >      let device =<br>
> >        List.find (<br>
> >          fun dev -><br>
> > -          Parted.part_get_parttype dev = "msdos" &&<br>
> > -            pread dev 4 0x01b8 = diskid<br>
> > +          try<br>
> > +            Parted.part_get_parttype dev = "msdos" &&<br>
> > +              pread dev 4 0x01b8 = diskid<br>
> > +          with Unix.Unix_error (Unix.EINVAL, _, _) -> false<br>
><br>
> The old C inspection code did not check for the partition type:<br>
> <a href="https://github.com/libguestfs/libguestfs/blob/stable-1.36/lib/inspect-fs-windows.c#L591" rel="noreferrer noreferrer" target="_blank">https://github.com/libguestfs/libguestfs/blob/stable-1.36/lib/inspect-fs-windows.c#L591</a><br>
> The partition type check was added recently:<br>
> <a href="https://github.com/libguestfs/libguestfs/commit/7b4d13626ff878b124d01ec452a4fd0052934133" rel="noreferrer noreferrer" target="_blank">https://github.com/libguestfs/libguestfs/commit/7b4d13626ff878b124d01ec452a4fd0052934133</a><br>
><br>
> TBH I'd rather prefer that we check for the situation explicitly<br>
> before, rather than catching a generic EINVAL error. Or, even better,<br>
> try to avoid altogether the situation that 7b4d13626ff878 checks,<br>
> handling this issue as well.<br>
><br>
> > @@ -402,7 +404,10 @@ and map_registry_disk_blob_gpt partitions blob =<br>
> >          fun part -><br>
> >            let partnum = Devsparts.part_to_partnum part in<br>
> >            let device = Devsparts.part_to_dev part in<br>
> > -          let typ = Parted.part_get_parttype device in<br>
> > +          let typ =<br>
> > +            try<br>
> > +              Parted.part_get_parttype device<br>
> > +            with Unix.Unix_error (Unix.EINVAL, _, _) -> "unknown" in<br>
> >            if typ <> "gpt" then false<br>
> >            else (<br>
><br>
> Ditto.<br>
><br>
> --<br>
> Pino Toscano_______________________________________________<br>
> Libguestfs mailing list<br>
> <a href="mailto:Libguestfs@redhat.com" target="_blank" rel="noreferrer">Libguestfs@redhat.com</a><br>
> <a href="https://www.redhat.com/mailman/listinfo/libguestfs" rel="noreferrer noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/libguestfs</a><br>
</blockquote></div>