Re: [Libguestfs] [PATCH v2/alternative 2/2] New API: guestfs_device_name returning the drive name

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-d...) >> >> 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.

Rich.