Libguestfs - Oct 2020 - Re: [PATCH v3 8/8] lib/canonical-name.c: Hide EINVAL error from underlying API call.

On Thu, Sep 17, 2020 at 01:40:04PM +0100, Richard W.M. Jones
wrote:
>When guestfs_lvm_canonical_lv_name was called with a /dev/dm* or
>/dev/mapper* name which was not an LV then a noisy error would be
>printed.  This would typically have happened with encrypted disks, and
>now happens very noticably when inspecting Windows BitLocker-
>encrypted guests.
>
>Using the modified error behaviour of this API from the previous
>commit we can now hide that error in that specific case.  (Even in
>this case the underlying error message still gets logged in debug
>output).
>---
> lib/canonical-name.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/lib/canonical-name.c b/lib/canonical-name.c
>index 052bbf12c..e0c7918b4 100644
>--- a/lib/canonical-name.c
>+++ b/lib/canonical-name.c
>@@ -46,9 +46,14 @@ guestfs_impl_canonical_device_name (guestfs_h *g, const
char *device)
>   }
>   else if (STRPREFIX (device, "/dev/mapper/") ||
>            STRPREFIX (device, "/dev/dm-")) {
>-    /* XXX hide errors */
>+    guestfs_push_error_handler (g, NULL, NULL);
Doesn't this hide the other error which might be important as well?  The
only
one I can find it the file not existing, but it means this function might fail
without an error message if I understand it correctly.
>     ret = guestfs_lvm_canonical_lv_name (g, device);
>-    if (ret == NULL)
>+    guestfs_pop_error_handler (g);
>+    /* EINVAL is expected if the device is somelike a LUKS- or
>+     * BitLocker-encrypted volume, so simply return the original
>+     * name in that case.
>+     */
>+    if (ret == NULL && guestfs_last_errno (g) == EINVAL)
>       ret = safe_strdup (g, device);
>   }
>   else
>-- 
>2.27.0
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

On Fri, Oct 09, 2020 at 11:47:28AM +0200, Martin Kletzander
wrote:
> On Thu, Sep 17, 2020 at 01:40:04PM +0100, Richard W.M. Jones wrote:
> >When guestfs_lvm_canonical_lv_name was called with a /dev/dm* or
> >/dev/mapper* name which was not an LV then a noisy error would be
> >printed.  This would typically have happened with encrypted disks, and
> >now happens very noticably when inspecting Windows BitLocker-
> >encrypted guests.
> >
> >Using the modified error behaviour of this API from the previous
> >commit we can now hide that error in that specific case.  (Even in
> >this case the underlying error message still gets logged in debug
> >output).
> >---
> >lib/canonical-name.c | 9 +++++++--
> >1 file changed, 7 insertions(+), 2 deletions(-)
> >
> >diff --git a/lib/canonical-name.c b/lib/canonical-name.c
> >index 052bbf12c..e0c7918b4 100644
> >--- a/lib/canonical-name.c
> >+++ b/lib/canonical-name.c
> >@@ -46,9 +46,14 @@ guestfs_impl_canonical_device_name (guestfs_h *g,
const char *device)
> >  }
> >  else if (STRPREFIX (device, "/dev/mapper/") ||
> >           STRPREFIX (device, "/dev/dm-")) {
> >-    /* XXX hide errors */
> >+    guestfs_push_error_handler (g, NULL, NULL);
> 
> Doesn't this hide the other error which might be important as well? 
The only
> one I can find it the file not existing, but it means this function might
fail
> without an error message if I understand it correctly.
The fundamental problem is that the idea of passing strings as device
names turned out to have been rather misconceived.  If we didn't pass
strings as device names then this API wouldn't be needed at all and
errors like file not existing probably wouldn't be possible, or could
be handled as soon as they are passed to the API.  But as it's baked
into the API in fundamental ways there's not much to do about it now.
> >    ret = guestfs_lvm_canonical_lv_name (g, device);
> >-    if (ret == NULL)
> >+    guestfs_pop_error_handler (g);
> >+    /* EINVAL is expected if the device is somelike a LUKS- or
> >+     * BitLocker-encrypted volume, so simply return the original
> >+     * name in that case.
> >+     */
> >+    if (ret == NULL && guestfs_last_errno (g) == EINVAL)
> >      ret = safe_strdup (g, device);
This code doesn't entirely hide the errors.  They should still appear
in debug messages, which means the all important "virt-v2v -v -x"
debugging method will tell us the true reason if there's some problem
like system calls failing with EIO.

Nevertheless looking at this again it could be possible to grab
guestfs_last_error in the !EINVAL path and call error() with it, which
I think should be sufficient to expose any unexpected errors.

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/

This is the patch I tested which works (on top of the
patch posted):

diff --git a/lib/canonical-name.c b/lib/canonical-name.c
index e0c7918b4..ae4def692 100644
--- a/lib/canonical-name.c
+++ b/lib/canonical-name.c
@@ -53,8 +53,16 @@ guestfs_impl_canonical_device_name (guestfs_h *g, const char
*device)
      * BitLocker-encrypted volume, so simply return the original
      * name in that case.
      */
-    if (ret == NULL && guestfs_last_errno (g) == EINVAL)
-      ret = safe_strdup (g, device);
+    if (ret == NULL) {
+      if (guestfs_last_errno (g) == EINVAL)
+        ret = safe_strdup (g, device);
+      else
+        /* Make sure the original error gets pushed through the
+         * error handlers.
+         */
+        guestfs_int_error_errno (g, guestfs_last_errno (g),
+                                 "%s", guestfs_last_error (g));
+    }
   }
   else
     ret = safe_strdup (g, device);

---

Current upstream:

$ guestfish scratch 1M : run : canonical-device-name "/dev/dm-999"
libguestfs: error: lvm_canonical_lv_name: stat: /dev/dm-999: No such file or
directory
/dev/dm-999

Patch posted without the above patch added:

$ ./run guestfish scratch 1M : run : canonical-device-name
"/dev/dm-999"

(no output, but the command fails with exit code 1)

Patch posted + above patch:

$ ./run guestfish scratch 1M : run : canonical-device-name
"/dev/dm-999"
libguestfs: error: lvm_canonical_lv_name: stat: /dev/dm-999: No such file or
directory

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 Fri, Oct 09, 2020 at 11:33:43AM +0100, Richard W.M. Jones
wrote:
> 
> This is the patch I tested which works (on top of the
> patch posted):
> 
> diff --git a/lib/canonical-name.c b/lib/canonical-name.c
> index e0c7918b4..ae4def692 100644
> --- a/lib/canonical-name.c
> +++ b/lib/canonical-name.c
> @@ -53,8 +53,16 @@ guestfs_impl_canonical_device_name (guestfs_h *g, const
char *device)
>       * BitLocker-encrypted volume, so simply return the original
>       * name in that case.
>       */
> -    if (ret == NULL && guestfs_last_errno (g) == EINVAL)
> -      ret = safe_strdup (g, device);
> +    if (ret == NULL) {
> +      if (guestfs_last_errno (g) == EINVAL)
> +        ret = safe_strdup (g, device);
> +      else
> +        /* Make sure the original error gets pushed through the
> +         * error handlers.
> +         */
> +        guestfs_int_error_errno (g, guestfs_last_errno (g),
> +                                 "%s", guestfs_last_error (g));
> +    }
>    }
>    else
>      ret = safe_strdup (g, device);
> 
> ---
> 
> Current upstream:
> 
> $ guestfish scratch 1M : run : canonical-device-name
"/dev/dm-999"
> libguestfs: error: lvm_canonical_lv_name: stat: /dev/dm-999: No such file
or directory
> /dev/dm-999           <-----
> 
> Patch posted without the above patch added:
> 
> $ ./run guestfish scratch 1M : run : canonical-device-name
"/dev/dm-999"
> 
> (no output, but the command fails with exit code 1)
> 
> Patch posted + above patch:
> 
> $ ./run guestfish scratch 1M : run : canonical-device-name
"/dev/dm-999"
> libguestfs: error: lvm_canonical_lv_name: stat: /dev/dm-999: No such file
or directory
Actually I didn't notice this, but it improves on the current upstream
behaviour.

Current upstream calls the error handlers and returns a non-error
original string (see <----- line above) and exit code 0.  With the
patch we return an error.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org