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 <kw...@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
