Libguestfs - Jun 2020 - Re: [PATCH] daemon: inspect_fs_windows: Handle parted errors

By creating an empty disk and using it as the first disk of the vm (i.e.
/dev/sda, /dev/sdb{1,2} contains the windows fses) we change the
iteration order of the disks.
This causes inspect_os() to fail since Parted returns a Unix_error if
the device does not contain any partitions - fix this by handling this
Unix_error.

Signed-off-by: Sam Eiderman <sameid@google.com>
---
 daemon/inspect_fs_windows.ml | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/daemon/inspect_fs_windows.ml b/daemon/inspect_fs_windows.ml
index c4a05bc38..bc6b98b60 100644
--- a/daemon/inspect_fs_windows.ml
+++ b/daemon/inspect_fs_windows.ml
@@ -365,8 +365,10 @@ and map_registry_disk_blob_mbr devices blob      let device
List.find (
         fun dev ->
-          Parted.part_get_parttype dev = "msdos" &&
-            pread dev 4 0x01b8 = diskid
+          try
+            Parted.part_get_parttype dev = "msdos" &&
+              pread dev 4 0x01b8 = diskid
+          with Unix.Unix_error (Unix.EINVAL, _, _) -> false
       ) devices in
 
     (* Next 8 bytes are the offset of the partition in bytes(!) given as
@@ -402,7 +404,10 @@ and map_registry_disk_blob_gpt partitions blob          fun
part ->
           let partnum = Devsparts.part_to_partnum part in
           let device = Devsparts.part_to_dev part in
-          let typ = Parted.part_get_parttype device in
+          let typ +            try
+              Parted.part_get_parttype device
+            with Unix.Unix_error (Unix.EINVAL, _, _) -> "unknown"
in
           if typ <> "gpt" then false
           else (
             let guid = Parted.part_get_gpt_guid device partnum in
-- 
2.27.0.212.ge8ba1cc988-goog

On Tuesday, 30 June 2020 10:33:40 CEST Sam Eiderman
wrote:
> By creating an empty disk and using it as the first disk of the vm (i.e.
> /dev/sda, /dev/sdb{1,2} contains the windows fses) we change the
> iteration order of the disks.
> This causes inspect_os() to fail since Parted returns a Unix_error if
> the device does not contain any partitions - fix this by handling this
> Unix_error.
> 
> Signed-off-by: Sam Eiderman <sameid@google.com>
> ---
>  daemon/inspect_fs_windows.ml | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/inspect_fs_windows.ml b/daemon/inspect_fs_windows.ml
> index c4a05bc38..bc6b98b60 100644
> --- a/daemon/inspect_fs_windows.ml
> +++ b/daemon/inspect_fs_windows.ml
> @@ -365,8 +365,10 @@ and map_registry_disk_blob_mbr devices blob >     
let device >        List.find (
>          fun dev ->
> -          Parted.part_get_parttype dev = "msdos" &&
> -            pread dev 4 0x01b8 = diskid
> +          try
> +            Parted.part_get_parttype dev = "msdos" &&
> +              pread dev 4 0x01b8 = diskid
> +          with Unix.Unix_error (Unix.EINVAL, _, _) -> false
The old C inspection code did not check for the partition type:
https://github.com/libguestfs/libguestfs/blob/stable-1.36/lib/inspect-fs-windows.c#L591
The partition type check was added recently:
https://github.com/libguestfs/libguestfs/commit/7b4d13626ff878b124d01ec452a4fd0052934133

TBH I'd rather prefer that we check for the situation explicitly
before, rather than catching a generic EINVAL error. Or, even better,
try to avoid altogether the situation that 7b4d13626ff878 checks,
handling this issue as well.
> @@ -402,7 +404,10 @@ and map_registry_disk_blob_gpt partitions blob >   
fun part ->
>            let partnum = Devsparts.part_to_partnum part in
>            let device = Devsparts.part_to_dev part in
> -          let typ = Parted.part_get_parttype device in
> +          let typ > +            try
> +              Parted.part_get_parttype device
> +            with Unix.Unix_error (Unix.EINVAL, _, _) ->
"unknown" in
>            if typ <> "gpt" then false
>            else (
Ditto.

-- 
Pino Toscano

Yea, I noticed that commit, but since it was used in gpt too
regardless of that commit, I decided not to mention this regression.

I also noticed that list_filesystems() still works, this is probably
due to filtering of devices (daemon/listfs.ml):

"let devices = List.filter is_not_partitioned_device devices in"

What do you have in mind here?

On Tue, Jun 30, 2020 at 12:33 PM Pino Toscano <ptoscano@redhat.com>
wrote:
>
> On Tuesday, 30 June 2020 10:33:40 CEST Sam Eiderman wrote:
> > By creating an empty disk and using it as the first disk of the vm
(i.e.
> > /dev/sda, /dev/sdb{1,2} contains the windows fses) we change the
> > iteration order of the disks.
> > This causes inspect_os() to fail since Parted returns a Unix_error if
> > the device does not contain any partitions - fix this by handling this
> > Unix_error.
> >
> > Signed-off-by: Sam Eiderman <sameid@google.com>
> > ---
> >  daemon/inspect_fs_windows.ml | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/daemon/inspect_fs_windows.ml
b/daemon/inspect_fs_windows.ml
> > index c4a05bc38..bc6b98b60 100644
> > --- a/daemon/inspect_fs_windows.ml
> > +++ b/daemon/inspect_fs_windows.ml
> > @@ -365,8 +365,10 @@ and map_registry_disk_blob_mbr devices blob >
>      let device > >        List.find (
> >          fun dev ->
> > -          Parted.part_get_parttype dev = "msdos" &&
> > -            pread dev 4 0x01b8 = diskid
> > +          try
> > +            Parted.part_get_parttype dev = "msdos"
&&
> > +              pread dev 4 0x01b8 = diskid
> > +          with Unix.Unix_error (Unix.EINVAL, _, _) -> false
>
> The old C inspection code did not check for the partition type:
>
https://github.com/libguestfs/libguestfs/blob/stable-1.36/lib/inspect-fs-windows.c#L591
> The partition type check was added recently:
>
https://github.com/libguestfs/libguestfs/commit/7b4d13626ff878b124d01ec452a4fd0052934133
>
> TBH I'd rather prefer that we check for the situation explicitly
> before, rather than catching a generic EINVAL error. Or, even better,
> try to avoid altogether the situation that 7b4d13626ff878 checks,
> handling this issue as well.
>
> > @@ -402,7 +404,10 @@ and map_registry_disk_blob_gpt partitions blob
> >          fun part ->
> >            let partnum = Devsparts.part_to_partnum part in
> >            let device = Devsparts.part_to_dev part in
> > -          let typ = Parted.part_get_parttype device in
> > +          let typ > > +            try
> > +              Parted.part_get_parttype device
> > +            with Unix.Unix_error (Unix.EINVAL, _, _) ->
"unknown" in
> >            if typ <> "gpt" then false
> >            else (
>
> Ditto.
>
> --
> Pino Toscano_______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs