Re: [PATCH 28/28] qemu-img: extend cvtnum() and use it in more places

2024-02-26 Thread Michael Tokarev

22.02.2024 00:16, Michael Tokarev wrote:


-static int64_t cvtnum_full(const char *name, const char *value, int64_t min,
-   int64_t max)
+static int64_t cvtnum_full(const char *name, const char *value,
+   bool issize, int64_t min, int64_t max)
  {
  int err;
  uint64_t res;
  
-err = qemu_strtosz(value, NULL, );

+err = issize ? qemu_strtosz(value, NULL, ) :
+   qemu_strtou64(value, NULL, 0, );
  if (err < 0 && err != -ERANGE) {
-error_report("Invalid %s specified. You may use "
- "k, M, G, T, P or E suffixes for", name);
-error_report("kilobytes, megabytes, gigabytes, terabytes, "
- "petabytes and exabytes.");
+if (issize) {
+error_report("Invalid %s specified. You may use "
+ "k, M, G, T, P or E suffixes for", name);
+error_report("kilobytes, megabytes, gigabytes, terabytes, "
+ "petabytes and exabytes.");
+} else {
+error_report("Invalid %s specified.", name);
+}


I've added actual value supplied to these error messages now.
And I think the list of possible suffixes makes little sense here.



@@ -5090,7 +5060,7 @@ static int img_bitmap(const img_cmd_t *ccmd, int argc, 
char **argv)
  src_fmt = optarg;
  break;
  case 'g':
-granularity = cvtnum("granularity", optarg);
+granularity = cvtnum("granularity", optarg, false);


Here, this is a size, so last arg should be true.  In the tests (190),
we already use -g 2M.  I didn't really knew what a granularity is while
converting it.

/mjt



Re: [PATCH 28/28] qemu-img: extend cvtnum() and use it in more places

2024-02-26 Thread Daniel P . Berrangé
On Thu, Feb 22, 2024 at 12:16:09AM +0300, Michael Tokarev wrote:
> cvtnum() expects input string to specify some sort of size
> (optionally with KMG... suffix).  However, there are a lot
> of other number conversions in there (using qemu_strtol ),
> also, not all conversions which use cvtnum, actually expects
> size, - like dd count=nn.
> 
> Add bool issize argument to cvtnum() to specify if it should
> treat the argument as a size or something else, - this changes
> conversion routine in use and error text.
> 
> Use the new cvtnum() in more places (like where strtol were used),
> since it never return negative number in successful conversion.
> When it makes sense, also specify upper or lower bounds at the
> same time.  This simplifies option processing in multiple places,
> removing the need of local temporary variables and longer error
> reporting code.
> 
> While at it, fix errors, like depth in measure must be >= 1,
> while the previous code allowed it to be 0.
> 
> In a few places, change unsigned variables (like of type size_t)
> to be signed instead, - to avoid the need of temporary conversion
> variable.  All these variables are okay to be signed, we never
> assign <0 value to them except of the cases of conversion error,
> where we return immediately.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 118 -
>  1 file changed, 44 insertions(+), 74 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|




[PATCH 28/28] qemu-img: extend cvtnum() and use it in more places

2024-02-21 Thread Michael Tokarev
cvtnum() expects input string to specify some sort of size
(optionally with KMG... suffix).  However, there are a lot
of other number conversions in there (using qemu_strtol ),
also, not all conversions which use cvtnum, actually expects
size, - like dd count=nn.

Add bool issize argument to cvtnum() to specify if it should
treat the argument as a size or something else, - this changes
conversion routine in use and error text.

Use the new cvtnum() in more places (like where strtol were used),
since it never return negative number in successful conversion.
When it makes sense, also specify upper or lower bounds at the
same time.  This simplifies option processing in multiple places,
removing the need of local temporary variables and longer error
reporting code.

While at it, fix errors, like depth in measure must be >= 1,
while the previous code allowed it to be 0.

In a few places, change unsigned variables (like of type size_t)
to be signed instead, - to avoid the need of temporary conversion
variable.  All these variables are okay to be signed, we never
assign <0 value to them except of the cases of conversion error,
where we return immediately.

Signed-off-by: Michael Tokarev 
---
 qemu-img.c | 118 -
 1 file changed, 44 insertions(+), 74 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 299e34e470..a066c4cfc4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -397,18 +397,23 @@ static int add_old_style_options(const char *fmt, 
QemuOpts *opts,
 return 0;
 }
 
-static int64_t cvtnum_full(const char *name, const char *value, int64_t min,
-   int64_t max)
+static int64_t cvtnum_full(const char *name, const char *value,
+   bool issize, int64_t min, int64_t max)
 {
 int err;
 uint64_t res;
 
-err = qemu_strtosz(value, NULL, );
+err = issize ? qemu_strtosz(value, NULL, ) :
+   qemu_strtou64(value, NULL, 0, );
 if (err < 0 && err != -ERANGE) {
-error_report("Invalid %s specified. You may use "
- "k, M, G, T, P or E suffixes for", name);
-error_report("kilobytes, megabytes, gigabytes, terabytes, "
- "petabytes and exabytes.");
+if (issize) {
+error_report("Invalid %s specified. You may use "
+ "k, M, G, T, P or E suffixes for", name);
+error_report("kilobytes, megabytes, gigabytes, terabytes, "
+ "petabytes and exabytes.");
+} else {
+error_report("Invalid %s specified.", name);
+}
 return err;
 }
 if (err == -ERANGE || res > max || res < min) {
@@ -419,9 +424,9 @@ static int64_t cvtnum_full(const char *name, const char 
*value, int64_t min,
 return res;
 }
 
-static int64_t cvtnum(const char *name, const char *value)
+static int64_t cvtnum(const char *name, const char *value, bool issize)
 {
-return cvtnum_full(name, value, 0, INT64_MAX);
+return cvtnum_full(name, value, issize, 0, INT64_MAX);
 }
 
 static int img_create(const img_cmd_t *ccmd, int argc, char **argv)
@@ -525,7 +530,7 @@ static int img_create(const img_cmd_t *ccmd, int argc, char 
**argv)
 
 /* Get image size, if specified */
 if (optind < argc) {
-img_size = cvtnum("image size", argv[optind++]);
+img_size = cvtnum("image size", argv[optind++], true);
 if (img_size < 0) {
 goto fail;
 }
@@ -987,7 +992,7 @@ static int img_commit(const img_cmd_t *ccmd, int argc, char 
**argv)
 quiet = true;
 break;
 case 'r':
-rate_limit = cvtnum("rate limit", optarg);
+rate_limit = cvtnum("rate limit", optarg, true);
 if (rate_limit < 0) {
 return 1;
 }
@@ -2412,7 +2417,7 @@ static int img_convert(const img_cmd_t *ccmd, int argc, 
char **argv)
 {
 int64_t sval;
 
-sval = cvtnum("buffer size for sparse output", optarg);
+sval = cvtnum("buffer size for sparse output", optarg, true);
 if (sval < 0) {
 goto fail_getopt;
 } else if (!QEMU_IS_ALIGNED(sval, BDRV_SECTOR_SIZE) ||
@@ -2444,10 +2449,9 @@ static int img_convert(const img_cmd_t *ccmd, int argc, 
char **argv)
 skip_create = true;
 break;
 case 'm':
-if (qemu_strtol(optarg, NULL, 0, _coroutines) ||
-s.num_coroutines < 1 || s.num_coroutines > MAX_COROUTINES) {
-error_report("Invalid number of coroutines. Allowed number of"
- " coroutines is between 1 and %d", 
MAX_COROUTINES);
+s.num_coroutines = cvtnum_full("number of coroutines", optarg,
+   false, 1, MAX_COROUTINES);
+if (s.num_coroutines < 0) {
 goto fail_getopt;
 }
 break;
@@ -2458,7 +2462,7 @@