| Message ID | 1504541267-36954-7-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | fsdev: qmp interface for io throttling | expand |
On Mon 04 Sep 2017 06:07:47 PM CEST, Pradeep Jagadeesh wrote: > +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) > +{ > + Error *err = NULL; > + IOThrottleList *fsdev_list, *info; > + fsdev_list = qmp_query_fsdev_io_throttle(&err); > + > + for (info = fsdev_list; info; info = info->next) { > + print_fsdev_throttle_config(mon, info->value); > + } > + qapi_free_IOThrottleList(fsdev_list); > +} You're passing an Error to qmp_query_fsdev_io_throttle() but then you don't handle it. Use hmp_handle_error() as I said in my previous e-mail. I know that with the current code qmp_query_fsdev_io_throttle() is never going to fail, but that's no reason to declare an Error and then ignore it. Berto
On 9/5/2017 9:53 AM, Alberto Garcia wrote: > On Mon 04 Sep 2017 06:07:47 PM CEST, Pradeep Jagadeesh wrote: > >> +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) >> +{ >> + Error *err = NULL; >> + IOThrottleList *fsdev_list, *info; >> + fsdev_list = qmp_query_fsdev_io_throttle(&err); >> + >> + for (info = fsdev_list; info; info = info->next) { >> + print_fsdev_throttle_config(mon, info->value); >> + } >> + qapi_free_IOThrottleList(fsdev_list); >> +} > > You're passing an Error to qmp_query_fsdev_io_throttle() but then you > don't handle it. Use hmp_handle_error() as I said in my previous e-mail. OK I will handle it. > > I know that with the current code qmp_query_fsdev_io_throttle() is never > going to fail, but that's no reason to declare an Error and then ignore > it. I need to pass err because the function qmp_query_fsdev_io_throttle(Error *) is created by the scripts. So, I have to declare and pass it. I do not is there any way to avoid this. -Pradeep > > Berto >
On Tue, 5 Sep 2017 10:28:02 +0200 Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote: > On 9/5/2017 9:53 AM, Alberto Garcia wrote: > > On Mon 04 Sep 2017 06:07:47 PM CEST, Pradeep Jagadeesh wrote: > > > >> +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) > >> +{ > >> + Error *err = NULL; > >> + IOThrottleList *fsdev_list, *info; > >> + fsdev_list = qmp_query_fsdev_io_throttle(&err); > >> + > >> + for (info = fsdev_list; info; info = info->next) { > >> + print_fsdev_throttle_config(mon, info->value); > >> + } > >> + qapi_free_IOThrottleList(fsdev_list); > >> +} > > > > You're passing an Error to qmp_query_fsdev_io_throttle() but then you > > don't handle it. Use hmp_handle_error() as I said in my previous e-mail. > OK I will handle it. > > > > I know that with the current code qmp_query_fsdev_io_throttle() is never > > going to fail, but that's no reason to declare an Error and then ignore > > it. > I need to pass err because the function > qmp_query_fsdev_io_throttle(Error *) is created by the scripts. So, I It isn't created by the scripts, it is introduced by patch 5 of this series. But yes, the generated code expects this function to have an Error ** (not Error *) argument. > have to declare and pass it. I do not is there any way to avoid this. > If you don't care for errors (because you know that the function can't fail), then you just need to pass NULL. But as Berto is saying, if you do pass an non-null Error ** then you should handle it. Cheers, -- Greg > -Pradeep > > > > Berto > > >
On Tue 05 Sep 2017 10:28:02 AM CEST, Pradeep Jagadeesh wrote: >>> +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) >>> +{ >>> + Error *err = NULL; >>> + IOThrottleList *fsdev_list, *info; >>> + fsdev_list = qmp_query_fsdev_io_throttle(&err); >>> + >>> + for (info = fsdev_list; info; info = info->next) { >>> + print_fsdev_throttle_config(mon, info->value); >>> + } >>> + qapi_free_IOThrottleList(fsdev_list); >>> +} >> >> You're passing an Error to qmp_query_fsdev_io_throttle() but then you >> don't handle it. Use hmp_handle_error() as I said in my previous e-mail. > OK I will handle it. >> >> I know that with the current code qmp_query_fsdev_io_throttle() is never >> going to fail, but that's no reason to declare an Error and then ignore >> it. > I need to pass err because the function > qmp_query_fsdev_io_throttle(Error *) is created by the scripts. So, I > have to declare and pass it. I do not is there any way to avoid this. When a function takes an Error ** like qmp_query_fsdev_io_throttle() and many others do, there are two alternatives: a) Declare an Error, pass it _and then handle it_ (if you don't handle it, you're leaking it): Error *err = NULL; some_function(&err); if (err) { /* handle err */ } b) Don't declare the Error and pass NULL instead: some_function(NULL); Note that (b) is perfectly valid, but if the function you're calling tries to set an Error then you won't get it. It's ok if you know what you're doing (which usually means: you have other way to determine if the function failed, _or_ you don't care if the function failed). Here you have no other way to know if qmp_query_fsdev_io_throttle() fails, so you should choose (a). I know that at the moment qmp_query_fsdev_io_throttle() can never fail so in practice this doesn't matter much _now_, but if in the future that function can fail then your code should be ready to handle it. Berto
On 9/5/2017 11:07 AM, Alberto Garcia wrote: > On Tue 05 Sep 2017 10:28:02 AM CEST, Pradeep Jagadeesh wrote: >>>> +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) >>>> +{ >>>> + Error *err = NULL; >>>> + IOThrottleList *fsdev_list, *info; >>>> + fsdev_list = qmp_query_fsdev_io_throttle(&err); >>>> + >>>> + for (info = fsdev_list; info; info = info->next) { >>>> + print_fsdev_throttle_config(mon, info->value); >>>> + } >>>> + qapi_free_IOThrottleList(fsdev_list); >>>> +} >>> >>> You're passing an Error to qmp_query_fsdev_io_throttle() but then you >>> don't handle it. Use hmp_handle_error() as I said in my previous e-mail. >> OK I will handle it. >>> >>> I know that with the current code qmp_query_fsdev_io_throttle() is never >>> going to fail, but that's no reason to declare an Error and then ignore >>> it. > >> I need to pass err because the function >> qmp_query_fsdev_io_throttle(Error *) is created by the scripts. So, I >> have to declare and pass it. I do not is there any way to avoid this. > > When a function takes an Error ** like qmp_query_fsdev_io_throttle() and > many others do, there are two alternatives: > > a) Declare an Error, pass it _and then handle it_ (if you don't handle > it, you're leaking it): > > Error *err = NULL; > some_function(&err); > if (err) { > /* handle err */ > } > > b) Don't declare the Error and pass NULL instead: > > some_function(NULL); > > Note that (b) is perfectly valid, but if the function you're calling > tries to set an Error then you won't get it. It's ok if you know what > you're doing (which usually means: you have other way to determine if > the function failed, _or_ you don't care if the function failed). > > Here you have no other way to know if qmp_query_fsdev_io_throttle() > fails, so you should choose (a). > > I know that at the moment qmp_query_fsdev_io_throttle() can never fail > so in practice this doesn't matter much _now_, but if in the future that > function can fail then your code should be ready to handle it. OK, I will pass NULL. -Pradeep > > Berto >
On Tue 05 Sep 2017 11:13:09 AM CEST, Pradeep Jagadeesh wrote: >> a) Declare an Error, pass it _and then handle it_ (if you don't >> handle it, you're leaking it): >> >> Here you have no other way to know if qmp_query_fsdev_io_throttle() >> fails, so you should choose (a). >> > OK, I will pass NULL. :-) (it's ok I guess, I don't see how that function can ever be made to fail in the future) Berto
On 9/5/2017 11:34 AM, Alberto Garcia wrote: > On Tue 05 Sep 2017 11:13:09 AM CEST, Pradeep Jagadeesh wrote: >>> a) Declare an Error, pass it _and then handle it_ (if you don't >>> handle it, you're leaking it): >>> >>> Here you have no other way to know if qmp_query_fsdev_io_throttle() >>> fails, so you should choose (a). >>> >> OK, I will pass NULL. > > :-) > > (it's ok I guess, I don't see how that function can ever be made to fail > in the future) :) Agreed. -Pradeep > > Berto >
* Pradeep Jagadeesh (pradeepkiruvale@gmail.com) wrote: > This patch introduces hmp interfaces for the fsdev > devices. > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> > --- > hmp-commands-info.hx | 18 ++++++++++++++++ > hmp-commands.hx | 19 +++++++++++++++++ > hmp.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > hmp.h | 4 ++++ > 4 files changed, 101 insertions(+) > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx > index d9df238..07ed338 100644 > --- a/hmp-commands-info.hx > +++ b/hmp-commands-info.hx > @@ -84,6 +84,24 @@ STEXI > Show block device statistics. > ETEXI > > +#if defined(CONFIG_VIRTFS) > + > + { > + .name = "fsdev_iothrottle", > + .args_type = "", > + .params = "", > + .help = "show fsdev iothrottle information", > + .cmd = hmp_info_fsdev_iothrottle, > + }, > + > +#endif > + > +STEXI > +@item info fsdev_iothrottle > +@findex fsdev_iothrottle > +Show fsdev device throttle info. > +ETEXI > + > { > .name = "block-jobs", > .args_type = "", > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 1941e19..aef9f79 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1680,6 +1680,25 @@ STEXI > Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} > ETEXI > > +#if defined(CONFIG_VIRTFS) > + > + { > + .name = "fsdev_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 fs devices", > + .cmd = hmp_fsdev_set_io_throttle, > + }, > + > +#endif > + > +STEXI > +@item fsdev_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} > +@findex fsdev_set_io_throttle > +Change I/O throttle limits for a fs devices to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} > +ETEXI I'm OK with this, and I see it's a copy of the equivalent block_set_io_throttle, but can you clarify the meaning - for example if a value is 0 what does it mean? Does it mean there's no throttle set for that variable? Also is the 'bps' bytes/second (as opposed to bits or blocks). Clarifying both of these in the text would be good. Dave > + > + > { > .name = "set_password", > .args_type = "protocol:s,password:s,connected:s?", > diff --git a/hmp.c b/hmp.c > index 2dbfb80..6e63cf1 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1783,6 +1783,66 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) > hmp_handle_error(mon, &err); > } > > +#ifdef CONFIG_VIRTFS > + > +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict) > +{ > + Error *err = NULL; > + IOThrottle throttle = { > + .has_id = true, > + .id = (char *) qdict_get_str(qdict, "device"), > + }; > + > + hmp_initialize_io_throttle(&throttle, qdict); > + qmp_fsdev_set_io_throttle(&throttle, &err); > + hmp_handle_error(mon, &err); > +} > + > +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg) > +{ > + monitor_printf(mon, "%s", fscfg->id); > + monitor_printf(mon, " I/O throttling:" > + " bps=%" PRId64 > + " bps_rd=%" PRId64 " bps_wr=%" PRId64 > + " bps_max=%" PRId64 > + " bps_rd_max=%" PRId64 > + " bps_wr_max=%" PRId64 > + " iops=%" PRId64 " iops_rd=%" PRId64 > + " iops_wr=%" PRId64 > + " iops_max=%" PRId64 > + " iops_rd_max=%" PRId64 > + " iops_wr_max=%" PRId64 > + " iops_size=%" PRId64 > + "\n", > + fscfg->bps, > + fscfg->bps_rd, > + fscfg->bps_wr, > + fscfg->bps_max, > + fscfg->bps_rd_max, > + fscfg->bps_wr_max, > + fscfg->iops, > + fscfg->iops_rd, > + fscfg->iops_wr, > + fscfg->iops_max, > + fscfg->iops_rd_max, > + fscfg->iops_wr_max, > + fscfg->iops_size); > +} > + > +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) > +{ > + Error *err = NULL; > + IOThrottleList *fsdev_list, *info; > + fsdev_list = qmp_query_fsdev_io_throttle(&err); > + > + for (info = fsdev_list; info; info = info->next) { > + print_fsdev_throttle_config(mon, info->value); > + } > + qapi_free_IOThrottleList(fsdev_list); > +} > + > +#endif > + > void hmp_block_stream(Monitor *mon, const QDict *qdict) > { > Error *error = NULL; > diff --git a/hmp.h b/hmp.h > index 1ff4552..d700d7d 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -81,6 +81,10 @@ 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); > +#ifdef CONFIG_VIRTFS > +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict); > +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict); > +#endif > void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict); > void hmp_block_stream(Monitor *mon, const QDict *qdict); > void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict); > -- > 1.8.3.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index d9df238..07ed338 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -84,6 +84,24 @@ STEXI Show block device statistics. ETEXI +#if defined(CONFIG_VIRTFS) + + { + .name = "fsdev_iothrottle", + .args_type = "", + .params = "", + .help = "show fsdev iothrottle information", + .cmd = hmp_info_fsdev_iothrottle, + }, + +#endif + +STEXI +@item info fsdev_iothrottle +@findex fsdev_iothrottle +Show fsdev device throttle info. +ETEXI + { .name = "block-jobs", .args_type = "", diff --git a/hmp-commands.hx b/hmp-commands.hx index 1941e19..aef9f79 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1680,6 +1680,25 @@ STEXI Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} ETEXI +#if defined(CONFIG_VIRTFS) + + { + .name = "fsdev_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 fs devices", + .cmd = hmp_fsdev_set_io_throttle, + }, + +#endif + +STEXI +@item fsdev_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} +@findex fsdev_set_io_throttle +Change I/O throttle limits for a fs devices to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} +ETEXI + + { .name = "set_password", .args_type = "protocol:s,password:s,connected:s?", diff --git a/hmp.c b/hmp.c index 2dbfb80..6e63cf1 100644 --- a/hmp.c +++ b/hmp.c @@ -1783,6 +1783,66 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +#ifdef CONFIG_VIRTFS + +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict) +{ + Error *err = NULL; + IOThrottle throttle = { + .has_id = true, + .id = (char *) qdict_get_str(qdict, "device"), + }; + + hmp_initialize_io_throttle(&throttle, qdict); + qmp_fsdev_set_io_throttle(&throttle, &err); + hmp_handle_error(mon, &err); +} + +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg) +{ + monitor_printf(mon, "%s", fscfg->id); + monitor_printf(mon, " I/O throttling:" + " bps=%" PRId64 + " bps_rd=%" PRId64 " bps_wr=%" PRId64 + " bps_max=%" PRId64 + " bps_rd_max=%" PRId64 + " bps_wr_max=%" PRId64 + " iops=%" PRId64 " iops_rd=%" PRId64 + " iops_wr=%" PRId64 + " iops_max=%" PRId64 + " iops_rd_max=%" PRId64 + " iops_wr_max=%" PRId64 + " iops_size=%" PRId64 + "\n", + fscfg->bps, + fscfg->bps_rd, + fscfg->bps_wr, + fscfg->bps_max, + fscfg->bps_rd_max, + fscfg->bps_wr_max, + fscfg->iops, + fscfg->iops_rd, + fscfg->iops_wr, + fscfg->iops_max, + fscfg->iops_rd_max, + fscfg->iops_wr_max, + fscfg->iops_size); +} + +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) +{ + Error *err = NULL; + IOThrottleList *fsdev_list, *info; + fsdev_list = qmp_query_fsdev_io_throttle(&err); + + for (info = fsdev_list; info; info = info->next) { + print_fsdev_throttle_config(mon, info->value); + } + qapi_free_IOThrottleList(fsdev_list); +} + +#endif + void hmp_block_stream(Monitor *mon, const QDict *qdict) { Error *error = NULL; diff --git a/hmp.h b/hmp.h index 1ff4552..d700d7d 100644 --- a/hmp.h +++ b/hmp.h @@ -81,6 +81,10 @@ 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); +#ifdef CONFIG_VIRTFS +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict); +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict); +#endif void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict); void hmp_block_stream(Monitor *mon, const QDict *qdict); void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
This patch introduces hmp interfaces for the fsdev devices. Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> --- hmp-commands-info.hx | 18 ++++++++++++++++ hmp-commands.hx | 19 +++++++++++++++++ hmp.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ hmp.h | 4 ++++ 4 files changed, 101 insertions(+)