Re: [PATCH v2 2/5] qemu_img: add error report to cvtnum

[Eyal Moscovici]

On 07/05/2020 0:59, Eric Blake wrote:
> On 5/6/20 4:34 PM, Eyal Moscovici wrote:
> > All calls to cvtnum check the return value and print the same error 
> > message more
> > or less. And so error reporting moved to cvtnum to reduce duplicate 
> > code and
> > provide a single error message.
> >
> > Acked-by: Mark Kanda <address@hidden>
> > Signed-off-by: Eyal Moscovici <address@hidden>
> > ---
> >   qemu-img.c                 | 63 ++++++++++++++++----------------------
> >   tests/qemu-iotests/049.out |  4 +--
> >   2 files changed, 28 insertions(+), 39 deletions(-)
>
> > -    err = qemu_strtosz(s, NULL, &value);
> > -    if (err < 0) {
> > +    err = qemu_strtosz(arg_value, NULL, &value);
> > +    if (err < 0 && err != -ERANGE) {
> > +        error_report("Invalid %s specified! You may use "
> > +                     "k, M, G, T, P or E suffixes for ", arg_name);
> > +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
> > +                     "petabytes and exabytes.");
> >          return err;
> >      }
> > -    if (value > INT64_MAX) {
> > +    if (err == -ERANGE || value > INT64_MAX) {
> > +        error_report("Invalid %s specified! Must be less than 8 EiB!",
>
> Copied from our pre-existing errors, but why are we shouting at our 
> user?  This would be a good time to s/!/./ to tone it down a bit.
Sure, will fix.
> > @@ -4491,10 +4488,12 @@ static int img_dd_bs(const char *arg,
> >   {
> >       int64_t res;
> >   -    res = cvtnum(arg);
> > +    res = cvtnum("bs", arg);
> >   -    if (res <= 0) {
> > -        error_report("invalid number: '%s'", arg);
> > +    if (res < 0) {
> > +        return 1;
> > +    } else if (res == 0) {
> > +        error_report("Invalid bs specified! It cannot be 0.");
>
> Maybe it's worth two functions:
>
> int64_t cvtnum_full(const char *name, const char *value, int64_t min, 
> int64_t max)
>
> and then a common helper:
>
> int64_t cvtnum(const char *name, const char *value) {
>     return cvtnum_full(name, value, 0, INT64_MAX);
> }
>
> many existing callers remain with cvtnum(), but callers like this 
> could be cvtnum("bs", arg, 1, INT64_MAX).  You'd still have to 
> special-case other restrictions, such as whether a number must a 
> power-of-2, but that's fewer places.
Great idea, I will create two functions.
> > +++ b/tests/qemu-iotests/049.out
> > @@ -92,13 +92,13 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 
> > size=1649267441664 cluster_size=65536 l
> >   == 3. Invalid sizes ==
> >     qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
> > -qemu-img: Image size must be less than 8 EiB!
> > +qemu-img: Invalid image size specified! Must be less than 8 EiB!
>
> Nice that you checked for iotest fallout.  Is this really the only 
> impacted test?
Thanks, yes.