On Mon, Sep 04, 2023 at 10:13:45AM +0200, Laszlo Ersek wrote:
> 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;
I think I have addressed everything!
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Upstream in d814ac4d47..b244bacce5
Note I had to squash patches 3 & 4 together, else bisection doesn't in
fact work.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
