Re: [Qemu-devel] [PATCH v2 3/6] block: change block-job-set-speed argument from 'value' to 'speed'

[Eric Blake]

On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block.c          |    6 +++---
>  block/stream.c   |    8 ++++----
>  block_int.h      |    4 ++--
>  blockdev.c       |    4 ++--
>  hmp-commands.hx  |    4 ++--
>  qapi-schema.json |    4 ++--
>  qmp-commands.hx  |    2 +-
>  7 files changed, 16 insertions(+), 16 deletions(-)
> 

>  
> -static void stream_set_speed(BlockJob *job, int64_t value, Error **errp)
> +static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
>  {
>      StreamBlockJob *s = container_of(job, StreamBlockJob, common);
>  
> -    if (value < 0) {
> -        error_set(errp, QERR_INVALID_PARAMETER, "value");
> +    if (speed < 0) {
> +        error_set(errp, QERR_INVALID_PARAMETER, "speed");
>          return;
>      }
> -    ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
> +    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE);

Unrelated to this patch, but as long as I'm noticing it:

What happens if speed is < BDRV_SECTOR_SIZE?  Should we be rounding this
division up, rather than truncating downwards?  On the other hand,
libvirt always passes speed in 1MiB multiples, so unless you have a 2
megabyte sector size, libvirt won't trigger the original question.

> @@ -79,7 +79,7 @@ typedef struct BlockJobType {
>      const char *job_type;
>  
>      /** Optional callback for job types that support setting a speed limit */
> -    void (*set_speed)(BlockJob *job, int64_t value, Error **errp);
> +    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);

As long as we are touching the interface to get it right...

Would it be any simpler to convert speed to uint64_t and not have to
deal with invalid negative speed values?

> +++ b/hmp-commands.hx
> @@ -85,8 +85,8 @@ ETEXI
>  
>      {
>          .name       = "block_job_set_speed",
> -        .args_type  = "device:B,value:o",
> -        .params     = "device value",
> +        .args_type  = "device:B,speed:o",
> +        .params     = "device speed",

For that matter, can the :o type _ever_ usefully allow negative values,
or is it always an unsigned calculation?

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature