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

[Richard W.M. Jones]

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/