On Tue, Jan 17, 2012 at 3:18 PM, Luiz Capitulino <lcapitul...@redhat.com> wrote: > On Fri, 13 Jan 2012 14:53:19 -0600 > Anthony Liguori <anth...@codemonkey.ws> wrote: > >> On 01/09/2012 05:24 AM, Luiz Capitulino wrote: >> > Signed-off-by: Luiz Capitulino<lcapitul...@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. > > Fixed. > >> > +# >> > +# @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. > > They were required, as it's necessary to use '?' for optionals in the > args_type entry: > >> > --- 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", > > The implementation also seems to imply they are required because there's > an explicit check for it (which can be dropped in the new version...). > > Can the author/block guys confirm these parameters are intended to be > required? For hmp, they are required, while for qmp, they are optional in theory. In order to two interfaces consistent, these parameters are all required. >
-- Regards, Zhi Yong Wu