Re: [PATCH] qemu-img: add support for rate limit in qemu-img commit

[Alberto Garcia <berto@igalia.com>]

On Sun 18 Oct 2020 08:33:59 AM CEST, Zhengui li wrote:

Hello,

> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index b89c019..ed55b76 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -34,9 +34,9 @@ SRST
>  ERST
>  
>  DEF("commit", img_commit,
> -    "commit [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
> +    "commit [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-b base] [-s speed] [-d] [-p] filename")
>  SRST
> -.. option:: commit [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [-t CACHE] [-b BASE] [-d] [-p] FILENAME
> +.. option:: commit [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [-t CACHE] [-b BASE] [-s SPEED] [-d] [-p] FILENAME
>  ERST

You should also update docs/tools/qemu-img.rst and explain what the new
parameter does.

> +        case 's': {
> +            unsigned long long sval;
> +            if (qemu_strtou64(optarg, NULL, 10, &sval)) {
> +                error_report("rate limit parse failed");
> +                return 1;
> +            }

You are using 'unsigned long long' here but qemu_strtou64() takes a
uint64_t.

> +            rate_limit = (int64_t)sval * 1024 * 1024;
> +        }   break;

And then you multiply that value by 1024*1024, which can overflow.

So I understand that the value received by 'qemu-img' is in megabytes?
Is there a reason for using that and not bytes?

qemu-img.c provides cvtnum() and cvtnum_full() which allow the user to
specify the units.

Berto