Re: [Libguestfs] [PATCH] common/options: Change drv struct to store drive index instead of device name.

On Friday, 28 April 2017 11:08:21 CEST Richard W.M. Jones wrote: > The device name is only used by guestfish (when using the -N option to > prepare drives). We constructed the device name very naively, > basically ‘sprintf ("/dev/sd%c", next_drive)’. > > This stores the device index instead, and only constructs the device > name in guestfish. Also the device name is constructed properly using > guestfs_int_drive_name so it can cope with #drives > 26. > --- Looks nice, a couple of notes below. > index 7ae8adf..26f77fd 100644 > --- a/align/scan.c > +++ b/align/scan.c > @@ -236,7 +236,7 @@ main (int argc, char *argv[]) > error (EXIT_FAILURE, 0, _("--uuid option cannot be used with -a or -d")); > > /* Add domains/drives from the command line (for a single guest). */ > - add_drives (drvs, 'a'); > + add_drives (drvs, 0); This, and in all the other files, is using 0 as starting index. Since add_drives is a macro, I'd remove its drive_index parameter, and always pass 0 to add_drives_handle. The only change needed would be... > char > -add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) > +add_drives_handle (guestfs_h *g, struct drv *drv, size_t drive_index) > { > int r; > struct guestfs_add_drive_opts_argv ad_optargs; > > - if (next_drive > 'z') > - error (EXIT_FAILURE, 0, _("too many drives added on the command line")); > - > if (drv) { > - next_drive = add_drives (drv->next, next_drive); > + drive_index = add_drives (drv->next, drive_index); ... here, to: drive_index = add_drives_handle (g, drv->next, drive_index);

Also, unrelated to these changes: since add_drives_handle is recursive, I guess it will overflow the stack when trying to add lots of disks, like the ones (i.e. even 20k!) I see in the experiments on your blog.