On 04/29/22 12:29, Richard W.M. Jones wrote: > On Fri, Apr 29, 2022 at 12:08:47PM +0200, Laszlo Ersek wrote: >> On 04/28/22 14:27, Richard W.M. Jones wrote: >>> For each drive added, return the name. For example calling this with >>> index 0 will return the string "/dev/sda". I called it >>> guestfs_device_name (not drive_name) for consistency with the existing >>> guestfs_device_index function. >>> >>> You don't really need to call this function. You can follow the >>> advice here: >>> https://libguestfs.org/guestfs.3.html#block-device-naming >>> and assume that drives are added with predictable names like >>> "/dev/sda", "/dev/sdb", etc. >>> >>> However it's useful to expose the internal guestfs_int_drive_name >>> function since especially handling names beyond index 26 is tricky >>> (https://rwmj.wordpress.com/2011/01/09/how-are-linux-drives-named-beyond-drive-26-devsdz/) >>> >>> Fixes: https://github.com/libguestfs/libguestfs/issues/80 >>> --- >>> generator/actions_core.ml | 24 +++++++++++++++++++++++- >>> lib/drives.c | 15 +++++++++++++++ >>> 2 files changed, 38 insertions(+), 1 deletion(-) >>> >>> diff --git a/generator/actions_core.ml b/generator/actions_core.ml >>> index ce9ee39cca..dc12fdc33e 100644 >>> --- a/generator/actions_core.ml >>> +++ b/generator/actions_core.ml >>> @@ -737,7 +737,29 @@ returns the index of the device in the list of devices. >>> Index numbers start from 0. The named device must exist, >>> for example as a string returned from C<guestfs_list_devices>. >>> >>> -See also C<guestfs_list_devices>, C<guestfs_part_to_dev>." }; >>> +See also C<guestfs_list_devices>, C<guestfs_part_to_dev>, >>> +C<guestfs_device_name>." }; >>> + >>> + { defaults with >>> + name = "device_name"; added = (1, 49, 1); >>> + style = RString (RPlainString, "name"), [Int "index"], []; >>> + tests = [ >>> + InitEmpty, Always, TestResult ( >>> + [["device_name"; "0"]], "STREQ (ret, \"/dev/sda\")"), []; >>> + InitEmpty, Always, TestResult ( >>> + [["device_name"; "1"]], "STREQ (ret, \"/dev/sdb\")"), []; >>> + InitEmpty, Always, TestLastFail ( >>> + [["device_name"; "99"]]), [] >>> + ]; >>> + shortdesc = "convert device index to name"; >>> + longdesc = "\ >>> +This function takes a device index and returns the device >>> +name. For example index C<0> will return the string C</dev/sda>. >>> + >>> +The drive index must have been added to the handle. >>> + >>> +See also C<guestfs_list_devices>, C<guestfs_part_to_dev>, >>> +C<guestfs_device_index>." }; >>> >>> { defaults with >>> name = "shutdown"; added = (1, 19, 16); >>> diff --git a/lib/drives.c b/lib/drives.c >>> index fd95308d2d..ecd8b79c65 100644 >>> --- a/lib/drives.c >>> +++ b/lib/drives.c >>> @@ -31,6 +31,7 @@ >>> #include <netdb.h> >>> #include <arpa/inet.h> >>> #include <assert.h> >>> +#include <errno.h> >>> #include <libintl.h> >>> >>> #include "c-ctype.h" >>> @@ -1084,3 +1085,17 @@ guestfs_impl_device_index (guestfs_h *g, const char >>> *device) >>> error (g, _("%s: device not found"), device); >>> return r; >>> } >>> + >>> +char * >>> +guestfs_impl_device_name (guestfs_h *g, int index) >>> +{ >>> + char name[64] = "/dev/sd"; >>> + >>> + if (index < 0 || index > g->nr_drives) { >> >> Upper bound check should be ">=". > > Urrr ... > >>> + guestfs_int_error_errno (g, EINVAL, _("drive index out of range")); >>> + return NULL; >>> + } >>> + >>> + guestfs_int_drive_name (index, &name[7]); >>> + return safe_strdup (g, name); >> >> Hrpmf, the connection between offset "7" and the length of "/dev/sd" is >> kinda ugly. :) How about: >> >> char drive_name[64]; >> >> /* ... */ >> guestfs_int_drive_name (index, drive_name); >> return safe_asprintf (g, "/dev/sd%s", drive_name); >> >> ? >> >> Finally, regarding which alternative we should go with, I'm totally >> undecided; I don't immediately see how exactly either alternative helps >> the ticket reporter do what they want to do. > > Yeah I don't think either of them do. Further discussion didn't > really resolve anything there either. > > However I do think this version (if it was fixed) is quite useful. At > the moment we publish the block device naming guide. But we then > leave people on their own when it comes to generating correct Linux > device names beyond device #26. They can use guestfs_- list_devices > and pick the right array index, but that's quite round-about. I guess > the only reason this hasn't been a problem before is that very few > users add more than one or two disks (even virt-v2v has problems with > lots of disks). > > I'm not especially committed to this.
I don't think this presents a risk of any regressions, or that it would become a difficulty for maintenance, so I'm happy to R-b with the check fixed and with safe_asprintf(). Thanks! Laszlo > > Rich. > _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
