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

[Richard W.M. Jones]

On Wed, May 03, 2017 at 06:28:43PM +0200, Pino Toscano wrote:
> 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);

OK, I'll make this change.

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

Indeed it did, but only when I was adding 60k+ drives ...  My solution
there was just to increase the size of the stack.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW