Re: [Qemu-devel] [PATCH 13/18] blockdev: Rename I/O throttling options for QMP

[Kevin Wolf <kwolf@redhat.com>]

Am 26.07.2013 um 18:10 hat Eric Blake geschrieben:
> On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> > In QMP, we want to use dashes instead of underscores in QMP argument
> > names, and use nested options for throttling. Convert them for -drive
> > before calling into the code that will be shared between -drive and
> > blockdev-add.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> > 
> 
> This patch will probably conflict with Benoît's work on leaky bucket
> throttling; can the two of you decide which one should go in first?  Are
> we trying to target both this series and leaky bucket throttling for 1.6?

If you complete the review before I leave today, I might still send a
pull request, but as I'm going to disable blockdev-add and the new
options once again for 1.6, it doesn't really matter that much.

Benoît's series is for 1.7 as well, if I understood Stefan correctly. He
said he was going to merge a bug fix part of it for 1.6 and leave the
rest for 1.7. (I haven't been following the throttling series myself,
that's why I can't comment in much more detail.)

> > @@ -485,17 +486,17 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> >  
> >      /* disk I/O throttling */
> >      io_limits.bps[BLOCK_IO_LIMIT_TOTAL]  =
> > -                           qemu_opt_get_number(opts, "bps", 0);
> > +        qemu_opt_get_number(opts, "throttling.bps-total", 0);
> 
> I like the rename to bps-total, to make it more obvious why it is
> provided in addition to bps-{read,write}.
> 
> >      io_limits.bps[BLOCK_IO_LIMIT_READ]   =
> > -                           qemu_opt_get_number(opts, "bps_rd", 0);
> > +        qemu_opt_get_number(opts, "throttling.bps-read", 0);
> >      io_limits.bps[BLOCK_IO_LIMIT_WRITE]  =
> > -                           qemu_opt_get_number(opts, "bps_wr", 0);
> > +        qemu_opt_get_number(opts, "throttling.bps-write", 0);
> 
> Thank you for spelling out the names; QMP doesn't need the abbreviations
> that the command line has.
> 
> > +DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> > +{
> > +    /* Change legacy command line options into QMP ones */
> > +    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> > +    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
> > +    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");
> 
> Of course, the intent here is to preserve back-compat (the old spelling
> continues to work, even if it differs from the QMP spelling).  But will
> this patch also allow the command line to learn support for the new
> spellings?  If so, is that worth mentioning as an intentional side
> effect in the commit message?

Yes, of course the command line will accept the new spellings as well.
I'll update the commit message.

Kevin