Richard W.M. Jones
2017-Apr-28 09:08 UTC
[Libguestfs] [PATCH] common/options: Change drv struct to store drive index instead of device name.
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. --- align/scan.c | 2 +- cat/cat.c | 2 +- cat/filesystems.c | 2 +- cat/log.c | 2 +- cat/ls.c | 2 +- cat/tail.c | 2 +- common/options/options.c | 30 +++++++++--------------------- common/options/options.h | 20 ++++++++++---------- df/main.c | 2 +- diff/diff.c | 4 ++-- edit/edit.c | 2 +- fish/fish.c | 12 ++++++++---- format/format.c | 2 +- fuse/guestmount.c | 2 +- inspector/inspector.c | 2 +- rescue/rescue.c | 3 +-- rescue/suggest.c | 2 +- 17 files changed, 42 insertions(+), 51 deletions(-) diff --git a/align/scan.c b/align/scan.c 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); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/cat/cat.c b/cat/cat.c index 9fa8b4f..990fc90 100644 --- a/cat/cat.c +++ b/cat/cat.c @@ -236,7 +236,7 @@ main (int argc, char *argv[]) } /* Add drives, inspect and mount. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/cat/filesystems.c b/cat/filesystems.c index 3bedf88..39b8cc3 100644 --- a/cat/filesystems.c +++ b/cat/filesystems.c @@ -346,7 +346,7 @@ main (int argc, char *argv[]) } /* Add drives. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/cat/log.c b/cat/log.c index eca8459..d1d22a7 100644 --- a/cat/log.c +++ b/cat/log.c @@ -209,7 +209,7 @@ main (int argc, char *argv[]) /* Add drives, inspect and mount. Note that inspector is always true, * and there is no -m option. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/cat/ls.c b/cat/ls.c index 590e645..774cab5 100644 --- a/cat/ls.c +++ b/cat/ls.c @@ -359,7 +359,7 @@ main (int argc, char *argv[]) } /* Add drives, inspect and mount. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/cat/tail.c b/cat/tail.c index 0759c9d..2797b86 100644 --- a/cat/tail.c +++ b/cat/tail.c @@ -279,7 +279,7 @@ do_tail (int argc, char *argv[], /* list of files in the guest */ int processed; /* Add drives, inspect and mount. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) return -1; diff --git a/common/options/options.c b/common/options/options.c index c9948d7..1965e1f 100644 --- a/common/options/options.c +++ b/common/options/options.c @@ -67,7 +67,6 @@ option_a (const char *arg, const char *format, struct drv **drvsp) error (EXIT_FAILURE, errno, "access: %s", uri.path); drv->type = drv_a; - drv->nr_drives = -1; drv->a.filename = uri.path; drv->a.format = format; @@ -76,7 +75,6 @@ option_a (const char *arg, const char *format, struct drv **drvsp) else { /* Remote storage. */ drv->type = drv_uri; - drv->nr_drives = -1; drv->uri.path = uri.path; drv->uri.protocol = uri.protocol; drv->uri.server = uri.server; @@ -103,7 +101,6 @@ option_d (const char *arg, struct drv **drvsp) error (EXIT_FAILURE, errno, "calloc"); drv->type = drv_d; - drv->nr_drives = -1; drv->d.guest = optarg; drv->next = *drvsp; @@ -111,22 +108,15 @@ option_d (const char *arg, struct drv **drvsp) } 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); - free (drv->device); - drv->device = NULL; - - if (asprintf (&drv->device, "/dev/sd%c", next_drive) == -1) - error (EXIT_FAILURE, errno, "asprintf"); + drv->drive_index = drive_index; switch (drv->type) { case drv_a: @@ -153,7 +143,7 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) exit (EXIT_FAILURE); drv->nr_drives = 1; - next_drive++; + drive_index++; break; case drv_uri: @@ -186,7 +176,7 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) exit (EXIT_FAILURE); drv->nr_drives = 1; - next_drive++; + drive_index++; break; case drv_d: @@ -195,7 +185,7 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) exit (EXIT_FAILURE); drv->nr_drives = r; - next_drive += r; + drive_index += r; break; case drv_N: @@ -208,7 +198,7 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) exit (EXIT_FAILURE); drv->nr_drives = 1; - next_drive++; + drive_index++; break; case drv_scratch: @@ -218,7 +208,7 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) exit (EXIT_FAILURE); drv->nr_drives = 1; - next_drive++; + drive_index++; break; default: /* keep GCC happy */ @@ -226,7 +216,7 @@ add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive) } } - return next_drive; + return drive_index; } static void display_mountpoints_on_failure (const char *mp_device, const char *user_supplied_options); @@ -320,8 +310,6 @@ free_drives (struct drv *drv) if (!drv) return; free_drives (drv->next); - free (drv->device); - switch (drv->type) { case drv_a: free (drv->a.filename); diff --git a/common/options/options.h b/common/options/options.h index 1598daf..7e4d269 100644 --- a/common/options/options.h +++ b/common/options/options.h @@ -43,14 +43,14 @@ extern int in_virt_rescue; struct drv { struct drv *next; - char *device; /* Device name inside the appliance (eg. /dev/sda). - * This is filled in when we add the drives in - * add_drives. Note that guests (-d option) may - * have multiple drives, in which case this is the - * first drive, and nr_drives is the number of - * drives used. - */ - int nr_drives; /* number of drives for this guest */ + /* Drive index. This is filled in by add_drives(). */ + size_t drive_index; + + /* Number of drives represented by this 'drv' struct. For -d this + * can be != 1 because a guest can have more than one disk. For + * others it is always 1. This is filled in by add_drives(). + */ + size_t nr_drives; enum { drv_a, /* -a option (without URI) */ @@ -123,8 +123,8 @@ extern char *read_key (const char *param); /* in options.c */ extern void option_a (const char *arg, const char *format, struct drv **drvsp); extern void option_d (const char *arg, struct drv **drvsp); -extern char add_drives_handle (guestfs_h *g, struct drv *drv, char next_drive); -#define add_drives(drv, next_drive) add_drives_handle (g, drv, next_drive) +extern char add_drives_handle (guestfs_h *g, struct drv *drv, size_t drive_index); +#define add_drives(drv, drive_index) add_drives_handle (g, drv, drive_index) extern void mount_mps (struct mp *mp); extern void free_drives (struct drv *drv); extern void free_mps (struct mp *mp); diff --git a/df/main.c b/df/main.c index 19ae77a..1c8159d 100644 --- a/df/main.c +++ b/df/main.c @@ -278,7 +278,7 @@ main (int argc, char *argv[]) CLEANUP_FREE char *name = NULL; /* Add domains/drives from the command line (for a single guest). */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/diff/diff.c b/diff/diff.c index 92ebdf7..7effcba 100644 --- a/diff/diff.c +++ b/diff/diff.c @@ -362,7 +362,7 @@ main (int argc, char *argv[]) unsigned errors = 0; /* Mount up first guest. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); @@ -373,7 +373,7 @@ main (int argc, char *argv[]) errors++; /* Mount up second guest. */ - add_drives_handle (g2, drvs2, 'a'); + add_drives_handle (g2, drvs2, 0); if (guestfs_launch (g2) == -1) exit (EXIT_FAILURE); diff --git a/edit/edit.c b/edit/edit.c index a5ef7f9..6e608a8 100644 --- a/edit/edit.c +++ b/edit/edit.c @@ -261,7 +261,7 @@ main (int argc, char *argv[]) } /* Add drives. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/fish/fish.c b/fish/fish.c index c47576f..183c800 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -362,7 +362,6 @@ main (int argc, char *argv[]) if (!drv) error (EXIT_FAILURE, errno, "calloc"); drv->type = drv_N; - drv->nr_drives = -1; p = strchr (optarg, '='); if (p != NULL) { *p = '\0'; @@ -466,7 +465,7 @@ main (int argc, char *argv[]) CHECK_OPTION_format_consumed; /* If we've got drives to add, add them now. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); /* If we've got mountpoints or prepared drives or -i option, we must * launch the guest and mount them. @@ -586,8 +585,13 @@ prepare_drives (struct drv *drv) { if (drv) { prepare_drives (drv->next); - if (drv->type == drv_N) - prepare_drive (drv->N.filename, drv->N.data, drv->device); + if (drv->type == drv_N) { + char device[64]; + + strcpy (device, "/dev/sd"); + guestfs_int_drive_name (drv->drive_index, &device[7]); + prepare_drive (drv->N.filename, drv->N.data, device); + } } } diff --git a/format/format.c b/format/format.c index 4e6ed26..ec6d33f 100644 --- a/format/format.c +++ b/format/format.c @@ -249,7 +249,7 @@ main (int argc, char *argv[]) const char *wipefs[] = { "wipefs", NULL }; /* Add domains/drives from the command line (for a single guest). */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/fuse/guestmount.c b/fuse/guestmount.c index 98933bc..a711603 100644 --- a/fuse/guestmount.c +++ b/fuse/guestmount.c @@ -360,7 +360,7 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); /* Do the guest drives and mountpoints. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); if (inspector) diff --git a/inspector/inspector.c b/inspector/inspector.c index 1120776..3672e3c 100644 --- a/inspector/inspector.c +++ b/inspector/inspector.c @@ -271,7 +271,7 @@ main (int argc, char *argv[]) /* Add drives, inspect and mount. Note that inspector is always true, * and there is no -m option. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); diff --git a/rescue/rescue.c b/rescue/rescue.c index 19baa87..2bf5f26 100644 --- a/rescue/rescue.c +++ b/rescue/rescue.c @@ -372,7 +372,7 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); /* Do the guest drives and mountpoints. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); if (inspector) @@ -673,7 +673,6 @@ add_scratch_disk (struct drv **drvs) if (!drv) error (EXIT_FAILURE, errno, "calloc"); drv->type = drv_scratch; - drv->nr_drives = -1; drv->scratch.size = INT64_C (10737418240); drv->next = *drvs; *drvs = drv; diff --git a/rescue/suggest.c b/rescue/suggest.c index 78792f2..f1d0fb6 100644 --- a/rescue/suggest.c +++ b/rescue/suggest.c @@ -54,7 +54,7 @@ do_suggestion (struct drv *drvs) read_only = 1; /* Add drives. */ - add_drives (drvs, 'a'); + add_drives (drvs, 0); /* Free up data structures, no longer needed after this point. */ free_drives (drvs); -- 2.9.3
Pino Toscano
2017-May-03 16:28 UTC
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. -- Pino Toscano
Richard W.M. Jones
2017-May-03 18:28 UTC
Re: [Libguestfs] [PATCH] common/options: Change drv struct to store drive index instead of device name.
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
Reasonably Related Threads
- Re: [PATCH] common/options: Change drv struct to store drive index instead of device name.
- [common PATCH] options: add '--blocksize' option for C-based tools
- [common PATCH v2 1/1] options: add '--blocksize' option for C-based tools
- [common PATCH v4 0/1] options: add '--blocksize' option for C-based tools
- [common PATCH v3 0/1] options: add '--blocksize' option for C-based tools