Re: [Qemu-devel] [PATCH 10/10] qapi: Convert block_set_io_throttle

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/09/2012 05:24 AM, Luiz Capitulino wrote:
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   blockdev.c       |   47 ++++++++++++++---------------------------------
>   blockdev.h       |    2 --
>   hmp-commands.hx  |    3 +--
>   hmp.c            |   14 ++++++++++++++
>   hmp.h            |    1 +
>   qapi-schema.json |   29 +++++++++++++++++++++++++++++
>   qmp-commands.hx  |    5 +----
>   7 files changed, 60 insertions(+), 41 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 8df78ce..5d16137 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -775,46 +775,29 @@ void qmp_change_blockdev(const char *device, const char *filename,
>   }
>
>   /* throttling disk I/O limits */
> -int do_block_set_io_throttle(Monitor *mon,
> -                       const QDict *qdict, QObject **ret_data)
> +void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> +                               int64_t bps_wr, int64_t iops, int64_t iops_rd,
> +                               int64_t iops_wr, Error **errp)
>   {
>       BlockIOLimit io_limits;
> -    const char *devname = qdict_get_str(qdict, "device");
>       BlockDriverState *bs;
>
> -    io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
> -                        = qdict_get_try_int(qdict, "bps", -1);
> -    io_limits.bps[BLOCK_IO_LIMIT_READ]
> -                        = qdict_get_try_int(qdict, "bps_rd", -1);
> -    io_limits.bps[BLOCK_IO_LIMIT_WRITE]
> -                        = qdict_get_try_int(qdict, "bps_wr", -1);
> -    io_limits.iops[BLOCK_IO_LIMIT_TOTAL]
> -                        = qdict_get_try_int(qdict, "iops", -1);
> -    io_limits.iops[BLOCK_IO_LIMIT_READ]
> -                        = qdict_get_try_int(qdict, "iops_rd", -1);
> -    io_limits.iops[BLOCK_IO_LIMIT_WRITE]
> -                        = qdict_get_try_int(qdict, "iops_wr", -1);
> -
> -    bs = bdrv_find(devname);
> +    bs = bdrv_find(device);
>       if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, devname);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
>       }
>
> -    if ((io_limits.bps[BLOCK_IO_LIMIT_TOTAL] == -1)
> -        || (io_limits.bps[BLOCK_IO_LIMIT_READ] == -1)
> -        || (io_limits.bps[BLOCK_IO_LIMIT_WRITE] == -1)
> -        || (io_limits.iops[BLOCK_IO_LIMIT_TOTAL] == -1)
> -        || (io_limits.iops[BLOCK_IO_LIMIT_READ] == -1)
> -        || (io_limits.iops[BLOCK_IO_LIMIT_WRITE] == -1)) {
> -        qerror_report(QERR_MISSING_PARAMETER,
> -                      "bps/bps_rd/bps_wr/iops/iops_rd/iops_wr");
> -        return -1;
> -    }
> +    io_limits.bps[BLOCK_IO_LIMIT_TOTAL] = bps;
> +    io_limits.bps[BLOCK_IO_LIMIT_READ]  = bps_rd;
> +    io_limits.bps[BLOCK_IO_LIMIT_WRITE] = bps_wr;
> +    io_limits.iops[BLOCK_IO_LIMIT_TOTAL]= iops;
> +    io_limits.iops[BLOCK_IO_LIMIT_READ] = iops_rd;
> +    io_limits.iops[BLOCK_IO_LIMIT_WRITE]= iops_wr;
>
>       if (!do_check_io_limits(&io_limits)) {
> -        qerror_report(QERR_INVALID_PARAMETER_COMBINATION);
> -        return -1;
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +        return;
>       }
>
>       bs->io_limits = io_limits;
> @@ -829,8 +812,6 @@ int do_block_set_io_throttle(Monitor *mon,
>               qemu_mod_timer(bs->block_timer, qemu_get_clock_ns(vm_clock));
>           }
>       }
> -
> -    return 0;
>   }
>
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/blockdev.h b/blockdev.h
> index b077449..260e16b 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -62,6 +62,4 @@ void qmp_change_blockdev(const char *device, const char *filename,
>                            bool has_format, const char *format, Error **errp);
>   void do_commit(Monitor *mon, const QDict *qdict);
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> -int do_block_set_io_throttle(Monitor *mon,
> -                             const QDict *qdict, QObject **ret_data);
>   #endif
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9f44690..a5d8d33 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1204,8 +1204,7 @@ ETEXI
>           .args_type  = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
>           .params     = "device bps bps_rd bps_wr iops iops_rd iops_wr",
>           .help       = "change I/O throttle limits for a block drive",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_block_set_io_throttle,
> +        .mhandler.cmd = hmp_block_set_io_throttle,
>       },
>
>   STEXI
> diff --git a/hmp.c b/hmp.c
> index 89fa8e7..c88c93e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -767,3 +767,17 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>       }
>       hmp_handle_error(mon,&err);
>   }
> +
> +void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    qmp_block_set_io_throttle(qdict_get_str(qdict, "device"),
> +                              qdict_get_int(qdict, "bps"),
> +                              qdict_get_int(qdict, "bps_rd"),
> +                              qdict_get_int(qdict, "bps_wr"),
> +                              qdict_get_int(qdict, "iops"),
> +                              qdict_get_int(qdict, "iops_rd"),
> +                              qdict_get_int(qdict, "iops_wr"),&err);
> +    hmp_handle_error(mon,&err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 621bdc2..aab0b1f 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -53,5 +53,6 @@ void hmp_set_password(Monitor *mon, const QDict *qdict);
>   void hmp_expire_password(Monitor *mon, const QDict *qdict);
>   void hmp_eject(Monitor *mon, const QDict *qdict);
>   void hmp_change(Monitor *mon, const QDict *qdict);
> +void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
>
>   #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 36fd156..1e1f128 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1403,3 +1403,32 @@
>   ##
>   { 'command': 'change',
>     'data': {'device': 'str', 'target': 'str', '*arg': 'str'} }
> +
> +##
> +# @block_set_io_throttle:
> +#
> +# Change I/O throttle limits for a block drive.
> +#
> +# @device: The name of the device
> +#
> +# @bps: total throughput limit in bytes per second
> +#
> +# @bps_rd: read throughput limit in bytes per second
> +#
> +# @bps_wr: read throughput limit in bytes per second

write throughput.

> +#
> +# @iops: total I/O operations per second
> +#
> +# @ops_rd: read I/O operations per second
> +#
> +# @iops_wr: write I/O operations per second
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If the argument combination is invalid, InvalidParameterCombination
> +#
> +# Since: 1.1


Do all of these fields have to be specified?  It looks like they were all 
optional in the previous form of the command so they should probably be optional 
here too.

Regards,

Anthony Liguori

> +##
> +{ 'command': 'block_set_io_throttle',
> +  'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> +            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index bfae81f..799e655 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -807,10 +807,7 @@ EQMP
>       {
>           .name       = "block_set_io_throttle",
>           .args_type  = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
> -        .params     = "device bps bps_rd bps_wr iops iops_rd iops_wr",
> -        .help       = "change I/O throttle limits for a block drive",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_block_set_io_throttle,
> +        .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,
>       },
>
>   SQMP