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

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

On Fri, Oct 09, 2020 at 11:37:41AM +0100, Richard W.M. Jones
wrote:
> 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.
This breaks virt-inspector, at least when we run the test suite which
has a phony Ubuntu guest with a non-existent /dev/mapper/* in its
/etc/fstab.

The manpage for guestfs_canonical_device_name[1] is sort of ambiguous
here.  It says that "/dev/mapper/*" is

  Converted to /dev/VG/LV form using guestfs_lvm_canonical_lv_name.

and guestfs_lvm_canonical_lv_name will certainly return an error for a
non-existent name.

However it does also say:

  Other strings are returned unmodified.

You can sort of read it both ways.  Because it breaks a long-standing
user of this API I'm going to change this so it behaves more like the
current upstream code (original error goes to debug, return original
device string).

Rich.

[1] https://libguestfs.org/guestfs.3.html#guestfs_canonical_device_name

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Fri, Oct 09, 2020 at 02:21:09PM +0100, Richard W.M. Jones
wrote:
>On Fri, Oct 09, 2020 at 11:37:41AM +0100, Richard W.M. Jones wrote:
>> 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.
>
>This breaks virt-inspector, at least when we run the test suite which
>has a phony Ubuntu guest with a non-existent /dev/mapper/* in its
>/etc/fstab.
>
>The manpage for guestfs_canonical_device_name[1] is sort of ambiguous
>here.  It says that "/dev/mapper/*" is
>
>  Converted to /dev/VG/LV form using guestfs_lvm_canonical_lv_name.
>
>and guestfs_lvm_canonical_lv_name will certainly return an error for a
>non-existent name.
>
>However it does also say:
>
>  Other strings are returned unmodified.
>
>You can sort of read it both ways.  Because it breaks a long-standing
>user of this API I'm going to change this so it behaves more like the
>current upstream code (original error goes to debug, return original
>device string).
>
OK, that makes sense.
>Rich.
>
>[1] https://libguestfs.org/guestfs.3.html#guestfs_canonical_device_name
>
>-- 
>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>Read my programming and virtualization blog: http://rwmj.wordpress.com
>virt-builder quickly builds VMs from scratch
>http://libguestfs.org/virt-builder.1.html