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 <mark.kanda@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
qemu-img.c | 63 ++++++++++++++++----------------------
tests/qemu-iotests/049.out | 4 +--
2 files changed, 28 insertions(+), 39 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 116a9c6349..4a06ab7fe8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -470,16 +470,22 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
return 0;
}
-static int64_t cvtnum(const char *s)
+static int64_t cvtnum(const char *arg_name, const char *arg_value)
{
int err;
uint64_t value;
- 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!",
+ arg_name);
return -ERANGE;
}
return value;
@@ -572,16 +578,8 @@ static int img_create(int argc, char **argv)
if (optind < argc) {
int64_t sval;
- sval = cvtnum(argv[optind++]);
+ sval = cvtnum("image size", argv[optind++]);
if (sval < 0) {
- if (sval == -ERANGE) {
- error_report("Image size must be less than 8 EiB!");
- } else {
- error_report("Invalid image size specified! You may use k, M, "
- "G, T, P or E suffixes for ");
- error_report("kilobytes, megabytes, gigabytes, terabytes, "
- "petabytes and exabytes.");
- }
goto fail;
}
img_size = (uint64_t)sval;
@@ -2187,8 +2185,10 @@ static int img_convert(int argc, char **argv)
{
int64_t sval;
- sval = cvtnum(optarg);
- if (sval < 0 || !QEMU_IS_ALIGNED(sval, BDRV_SECTOR_SIZE) ||
+ sval = cvtnum("buffer size for sparse output", optarg);
+ if (sval < 0) {
+ goto fail_getopt;
+ } else if (!QEMU_IS_ALIGNED(sval, BDRV_SECTOR_SIZE) ||
sval / BDRV_SECTOR_SIZE > MAX_BUF_SECTORS) {
error_report("Invalid buffer size for sparse output specified. "
"Valid sizes are multiples of %llu up to %llu. Select "
@@ -4291,9 +4291,8 @@ static int img_bench(int argc, char **argv)
break;
case 'o':
{
- offset = cvtnum(optarg);
+ offset = cvtnum("offset", optarg);
if (offset < 0) {
- error_report("Invalid offset specified");
return 1;
}
break;
@@ -4306,9 +4305,8 @@ static int img_bench(int argc, char **argv)
{
int64_t sval;
- sval = cvtnum(optarg);
+ sval = cvtnum("buffer size", optarg);
if (sval < 0) {
- error_report("Invalid buffer size specified");
return 1;
}
@@ -4319,9 +4317,8 @@ static int img_bench(int argc, char **argv)
{
int64_t sval;
- sval = cvtnum(optarg);
+ sval = cvtnum("step_size", optarg);
if (sval < 0) {
- error_report("Invalid step size specified");
return 1;
}
@@ -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.");
return 1;
}
in->bsz = out->bsz = res;
@@ -4506,10 +4505,9 @@ static int img_dd_count(const char *arg,
struct DdIo *in, struct DdIo *out,
struct DdInfo *dd)
{
- dd->count = cvtnum(arg);
+ dd->count = cvtnum("count", arg);
if (dd->count < 0) {
- error_report("invalid number: '%s'", arg);
return 1;
}
@@ -4538,10 +4536,9 @@ static int img_dd_skip(const char *arg,
struct DdIo *in, struct DdIo *out,
struct DdInfo *dd)
{
- in->offset = cvtnum(arg);
+ in->offset = cvtnum("skip", arg);
if (in->offset < 0) {
- error_report("invalid number: '%s'", arg);
return 1;
}
@@ -4923,16 +4920,8 @@ static int img_measure(int argc, char **argv)
{
int64_t sval;
- sval = cvtnum(optarg);
+ sval = cvtnum("image size", optarg);
if (sval < 0) {
- if (sval == -ERANGE) {
- error_report("Image size must be less than 8 EiB!");
- } else {
- error_report("Invalid image size specified! You may use "
- "k, M, G, T, P or E suffixes for ");
- error_report("kilobytes, megabytes, gigabytes, terabytes, "
- "petabytes and exabytes.");
- }
goto out;
}
img_size = (uint64_t)sval;
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index affa55b341..0ec2e6b82e 100644
--- a/tests/qemu-iotests/049.out
+++ 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!
qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2
qemu-img: TEST_DIR/t.qcow2: Value '-1024' is out of range for parameter 'size'
qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k
-qemu-img: Image size must be less than 8 EiB!
+qemu-img: Invalid image size specified! Must be less than 8 EiB!
qemu-img create -f qcow2 -o size=-1k TEST_DIR/t.qcow2
qemu-img: TEST_DIR/t.qcow2: Value '-1k' is out of range for parameter 'size'
--
2.17.2 (Apple Git-113)
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 <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
> 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.
> @@ -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.
> +++ 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?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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 <mark.kanda@oracle.com>
>> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
>> ---
>> 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.
© 2016 - 2025 Red Hat, Inc.