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

If this information is useful, please help other people find it:
Share via:

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

head link

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

head link

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

Search for more reasonably related threads

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

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

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

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

Reasonably Related Threads