thr3ads.net - Libguestfs - [Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse [Sep 2023]

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

Richard W.M. Jones

2023-Sep-03 15:23 UTC

[Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse

Allow these options to be specified using human sizes, for example
this now works:

  nbdcopy --request-size=32M ...
---
 copy/copy-sparse-allocated.sh    |  2 +-
 copy/copy-sparse-no-extents.sh   |  2 +-
 copy/copy-sparse-request-size.sh |  2 +-
 copy/main.c                      | 37 ++++++++++++++++++++------------
 copy/nbdcopy.h                   |  2 +-
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/copy/copy-sparse-allocated.sh b/copy/copy-sparse-allocated.sh
index c65ddea79f..e1fe9cf463 100755
--- a/copy/copy-sparse-allocated.sh
+++ b/copy/copy-sparse-allocated.sh
@@ -31,7 +31,7 @@ requires nbdkit eval --version
 out=copy-sparse-allocated.out
 cleanup_fn rm -f $out
 
-$VG nbdcopy --allocated --request-size=32768 -- \
+$VG nbdcopy --allocated --request-size=32K -- \
     [ nbdkit --exit-with-parent data data='
              1
              @1073741823 1
diff --git a/copy/copy-sparse-no-extents.sh b/copy/copy-sparse-no-extents.sh
index cff356978b..9368c564e9 100755
--- a/copy/copy-sparse-no-extents.sh
+++ b/copy/copy-sparse-no-extents.sh
@@ -39,7 +39,7 @@ requires nbdkit eval --version
 out=copy-sparse-no-extents.out
 cleanup_fn rm -f $out
 
-$VG nbdcopy --request-size=33554432 --no-extents -S 0 -- \
+$VG nbdcopy --request-size=32M --no-extents -S 0 -- \
     [ nbdkit --exit-with-parent data data='
              1
              @1073741823 1
diff --git a/copy/copy-sparse-request-size.sh b/copy/copy-sparse-request-size.sh
index dc8caeafd1..dd28695f68 100755
--- a/copy/copy-sparse-request-size.sh
+++ b/copy/copy-sparse-request-size.sh
@@ -39,7 +39,7 @@ requires nbdkit eval --version
 out=copy-sparse-request-size.out
 cleanup_fn rm -f $out
 
-$VG nbdcopy --no-extents -S 0 --request-size=1048576 -- \
+$VG nbdcopy --no-extents -S 0 --request-size=1M -- \
     [ nbdkit --exit-with-parent data data='
              1
              @33554431 1
diff --git a/copy/main.c b/copy/main.c
index 6928a4acde..47b1ea8be0 100644
--- a/copy/main.c
+++ b/copy/main.c
@@ -141,6 +141,8 @@ main (int argc, char *argv[])
   };
   int c;
   size_t i;
+  int64_t i64;
+  const char *error, *pstr;
 
   /* Set prog to basename argv[0]. */
   prog = strrchr (argv[0], '/');
@@ -210,26 +212,32 @@ main (int argc, char *argv[])
       break;
 
     case QUEUE_SIZE_OPTION:
-      if (sscanf (optarg, "%u", &queue_size) != 1) {
-        fprintf (stderr, "%s: --queue-size: could not parse: %s\n",
-                 prog, optarg);
+      i64 = human_size_parse (optarg, &error, &pstr);
+      if (i64 == -1) {
+        fprintf (stderr, "%s: --queue-size: %s: %s\n", prog, error,
pstr);
         exit (EXIT_FAILURE);
       }
+      if (i64 > UINT_MAX) {
+        fprintf (stderr, "%s: --queue-size is too large\n", prog);
+        exit (EXIT_FAILURE);
+      }
+      queue_size = i64;
       break;
 
     case REQUEST_SIZE_OPTION:
-      if (sscanf (optarg, "%u", &request_size) != 1) {
-        fprintf (stderr, "%s: --request-size: could not parse: %s\n",
-                 prog, optarg);
+      i64 = human_size_parse (optarg, &error, &pstr);
+      if (i64 == -1) {
+        fprintf (stderr, "%s: --request-size: %s: %s\n", prog, error,
pstr);
         exit (EXIT_FAILURE);
       }
-      if (request_size < MIN_REQUEST_SIZE || request_size >
MAX_REQUEST_SIZE ||
-              !is_power_of_2 (request_size)) {
+      if (i64 < MIN_REQUEST_SIZE || i64 > MAX_REQUEST_SIZE ||
+          !is_power_of_2 (i64)) {
         fprintf (stderr,
                 "%s: --request-size: must be a power of 2 within
%d-%d\n",
                  prog, MIN_REQUEST_SIZE, MAX_REQUEST_SIZE);
         exit (EXIT_FAILURE);
       }
+      request_size = i64;
       break;
 
     case 'R':
@@ -241,17 +249,18 @@ main (int argc, char *argv[])
       break;
 
     case 'S':
-      if (sscanf (optarg, "%u", &sparse_size) != 1) {
-        fprintf (stderr, "%s: --sparse: could not parse: %s\n",
-                 prog, optarg);
+      i64 = human_size_parse (optarg, &error, &pstr);
+      if (i64 == -1) {
+        fprintf (stderr, "%s: --sparse: %s: %s\n", prog, error,
pstr);
         exit (EXIT_FAILURE);
       }
-      if (sparse_size != 0 &&
-          (sparse_size < 512 || !is_power_of_2 (sparse_size))) {
-        fprintf (stderr, "%s: --sparse: must be a power of 2 and >=
512\n",
+      if (i64 != 0 &&
+          (i64 < 512 || i64 > UINT_MAX || !is_power_of_2 (i64))) {
+        fprintf (stderr, "%s: --sparse: must be a power of 2, between 512
and UINT_MAX\n",
                  prog);
         exit (EXIT_FAILURE);
       }
+      sparse_size = i64;
       break;
 
     case 'T':
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
index 465b7052e7..ade53d1a05 100644
--- a/copy/nbdcopy.h
+++ b/copy/nbdcopy.h
@@ -28,7 +28,7 @@
 #include "vector.h"
 
 #define MIN_REQUEST_SIZE 4096
-#define MAX_REQUEST_SIZE (32 * 1024 * 1024)
+#define MAX_REQUEST_SIZE (32 * 1024 * 1024) /* must be <= UNSIGNED_MAX */
 
 /* This must be a multiple of MAX_REQUEST_SIZE.  Larger is better up
  * to a point, but it reduces the effectiveness of threads if the work
-- 
2.41.0

Laszlo Ersek

2023-Sep-04 08:13 UTC

head link

[Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse

On 9/3/23 17:23, Richard W.M. Jones wrote:> Allow these options to be specified using human sizes, for example
> this now works:
> 
>   nbdcopy --request-size=32M ...
> ---
>  copy/copy-sparse-allocated.sh    |  2 +-
>  copy/copy-sparse-no-extents.sh   |  2 +-
>  copy/copy-sparse-request-size.sh |  2 +-
>  copy/main.c                      | 37 ++++++++++++++++++++------------
>  copy/nbdcopy.h                   |  2 +-
>  5 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/copy/copy-sparse-allocated.sh b/copy/copy-sparse-allocated.sh
> index c65ddea79f..e1fe9cf463 100755
> --- a/copy/copy-sparse-allocated.sh
> +++ b/copy/copy-sparse-allocated.sh
> @@ -31,7 +31,7 @@ requires nbdkit eval --version
>  out=copy-sparse-allocated.out
>  cleanup_fn rm -f $out
>  
> -$VG nbdcopy --allocated --request-size=32768 -- \
> +$VG nbdcopy --allocated --request-size=32K -- \
>      [ nbdkit --exit-with-parent data data='
>               1
>               @1073741823 1
> diff --git a/copy/copy-sparse-no-extents.sh
b/copy/copy-sparse-no-extents.sh
> index cff356978b..9368c564e9 100755
> --- a/copy/copy-sparse-no-extents.sh
> +++ b/copy/copy-sparse-no-extents.sh
> @@ -39,7 +39,7 @@ requires nbdkit eval --version
>  out=copy-sparse-no-extents.out
>  cleanup_fn rm -f $out
>  
> -$VG nbdcopy --request-size=33554432 --no-extents -S 0 -- \
> +$VG nbdcopy --request-size=32M --no-extents -S 0 -- \
>      [ nbdkit --exit-with-parent data data='
>               1
>               @1073741823 1
> diff --git a/copy/copy-sparse-request-size.sh
b/copy/copy-sparse-request-size.sh
> index dc8caeafd1..dd28695f68 100755
> --- a/copy/copy-sparse-request-size.sh
> +++ b/copy/copy-sparse-request-size.sh
> @@ -39,7 +39,7 @@ requires nbdkit eval --version
>  out=copy-sparse-request-size.out
>  cleanup_fn rm -f $out
>  
> -$VG nbdcopy --no-extents -S 0 --request-size=1048576 -- \
> +$VG nbdcopy --no-extents -S 0 --request-size=1M -- \
>      [ nbdkit --exit-with-parent data data='
>               1
>               @33554431 1
> diff --git a/copy/main.c b/copy/main.c
> index 6928a4acde..47b1ea8be0 100644
> --- a/copy/main.c
> +++ b/copy/main.c
> @@ -141,6 +141,8 @@ main (int argc, char *argv[])
>    };
>    int c;
>    size_t i;
> +  int64_t i64;
> +  const char *error, *pstr;
>  
>    /* Set prog to basename argv[0]. */
>    prog = strrchr (argv[0], '/');
> @@ -210,26 +212,32 @@ main (int argc, char *argv[])
>        break;
>  
>      case QUEUE_SIZE_OPTION:
> -      if (sscanf (optarg, "%u", &queue_size) != 1) {
> -        fprintf (stderr, "%s: --queue-size: could not parse:
%s\n",
> -                 prog, optarg);
> +      i64 = human_size_parse (optarg, &error, &pstr);
> +      if (i64 == -1) {
> +        fprintf (stderr, "%s: --queue-size: %s: %s\n", prog,
error, pstr);
>          exit (EXIT_FAILURE);
>        }
> +      if (i64 > UINT_MAX) {
> +        fprintf (stderr, "%s: --queue-size is too large\n",
prog);
(1) Print "optarg" (or even format back "i64") here?
> +        exit (EXIT_FAILURE);
> +      }
> +      queue_size = i64;
>        break;
>  
>      case REQUEST_SIZE_OPTION:
> -      if (sscanf (optarg, "%u", &request_size) != 1) {
> -        fprintf (stderr, "%s: --request-size: could not parse:
%s\n",
> -                 prog, optarg);
> +      i64 = human_size_parse (optarg, &error, &pstr);
> +      if (i64 == -1) {
> +        fprintf (stderr, "%s: --request-size: %s: %s\n", prog,
error, pstr);
>          exit (EXIT_FAILURE);
>        }
> -      if (request_size < MIN_REQUEST_SIZE || request_size >
MAX_REQUEST_SIZE ||
> -              !is_power_of_2 (request_size)) {
> +      if (i64 < MIN_REQUEST_SIZE || i64 > MAX_REQUEST_SIZE ||
> +          !is_power_of_2 (i64)) {
>          fprintf (stderr,
>                  "%s: --request-size: must be a power of 2 within
%d-%d\n",
>                   prog, MIN_REQUEST_SIZE, MAX_REQUEST_SIZE);
(2) Same comment as (1).

(Albeit not as much justified as at (1). At (1), the patch *stops*
printing the out-of-range "optarg", while at (2), the patch *continues
not to print* the out-of-range "optarg".)
>          exit (EXIT_FAILURE);
>        }
> +      request_size = i64;
>        break;
(3) I'll come back to this later...
>  
>      case 'R':
> @@ -241,17 +249,18 @@ main (int argc, char *argv[])
>        break;
>  
>      case 'S':
> -      if (sscanf (optarg, "%u", &sparse_size) != 1) {
> -        fprintf (stderr, "%s: --sparse: could not parse: %s\n",
> -                 prog, optarg);
> +      i64 = human_size_parse (optarg, &error, &pstr);
> +      if (i64 == -1) {
> +        fprintf (stderr, "%s: --sparse: %s: %s\n", prog, error,
pstr);
>          exit (EXIT_FAILURE);
>        }
> -      if (sparse_size != 0 &&
> -          (sparse_size < 512 || !is_power_of_2 (sparse_size))) {
> -        fprintf (stderr, "%s: --sparse: must be a power of 2 and
>= 512\n",
> +      if (i64 != 0 &&
> +          (i64 < 512 || i64 > UINT_MAX || !is_power_of_2 (i64))) {
> +        fprintf (stderr, "%s: --sparse: must be a power of 2, between
512 and UINT_MAX\n",
>                   prog);
(4) For consistency with the pre-patch code, consider printing optarg or
i64 here as well.

(5) For consistency with (2), I'd suggest printing "within %u-%u"
--
that does two things for us: clarifies that 512 precisely is permitted
("between" is a bit murky there), plus prints UINT_MAX numerically.
>          exit (EXIT_FAILURE);
>        }
> +      sparse_size = i64;
>        break;
>  
>      case 'T':
> diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
> index 465b7052e7..ade53d1a05 100644
> --- a/copy/nbdcopy.h
> +++ b/copy/nbdcopy.h
> @@ -28,7 +28,7 @@
>  #include "vector.h"
>  
>  #define MIN_REQUEST_SIZE 4096
> -#define MAX_REQUEST_SIZE (32 * 1024 * 1024)
> +#define MAX_REQUEST_SIZE (32 * 1024 * 1024) /* must be <= UNSIGNED_MAX
*/
(6) Good update, but what about not touching this location, and adding a
STATIC_ASSERT at (3) instead? (I.e., just before assigning
"request_size".)
>  
>  /* This must be a multiple of MAX_REQUEST_SIZE.  Larger is better up
>   * to a point, but it reduces the effectiveness of threads if the work
Address as many as you wish from the above;

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Laszlo

Libguestfs - Sep 2023 - [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse

[Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse

[Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse