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

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