Re: [Qemu-devel] [PATCH 3/4] block: add 'speed' optional parameter to block-stream

[Luiz Capitulino]

On Wed, 25 Apr 2012 11:56:51 +0100
Stefan Hajnoczi <address@hidden> wrote:

> On Tue, Apr 24, 2012 at 8:31 PM, Luiz Capitulino <address@hidden> wrote:
> > On Mon, 23 Apr 2012 16:39:48 +0100
> > Stefan Hajnoczi <address@hidden> wrote:
> >
> >> Allow streaming operations to be started with an initial speed limit.
> >> This eliminates the window of time between starting streaming and
> >> issuing block-job-set-speed.  Users should use the new optional 'speed'
> >> parameter instead so that speed limits are in effect immediately when
> >> the job starts.
> >>
> >> Signed-off-by: Stefan Hajnoczi <address@hidden>
> >> ---
> >>  block.c          |   18 ++++++++++++++++--
> >>  block/stream.c   |    5 +++--
> >>  block_int.h      |    9 ++++++---
> >>  blockdev.c       |    6 ++++--
> >>  hmp-commands.hx  |    4 ++--
> >>  hmp.c            |    4 +++-
> >>  qapi-schema.json |    6 +++++-
> >>  qmp-commands.hx  |    2 +-
> >>  8 files changed, 40 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 7056d8c..e3c1483 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -4072,8 +4072,8 @@ out:
> >>  }
> >>
> >>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> >> -                       BlockDriverCompletionFunc *cb, void *opaque,
> >> -                       Error **errp)
> >> +                       int64_t speed, BlockDriverCompletionFunc *cb,
> >> +                       void *opaque, Error **errp)
> >>  {
> >>      BlockJob *job;
> >>
> >> @@ -4089,6 +4089,20 @@ void *block_job_create(const BlockJobType 
> >> *job_type, BlockDriverState *bs,
> >>      job->cb            = cb;
> >>      job->opaque        = opaque;
> >>      bs->job = job;
> >> +
> >> +    /* Only set speed when necessary to avoid NotSupported error */
> >> +    if (speed != 0) {
> >
> > Missed this small detail. Ideally, you should test against has_speed, but
> > I think that there are only two possibilities for a false negativehere:
> > 1. the client/user expects speed=0 to "work" 2. 'speed' is (probably
> > incorrectly) initialized to some value != 0.
> 
> By the time we get here we've already checked has_speed and set
> speed=0 when has_speed=false. 

Ah, true, but speed=0 is just ignored. In practice doesn't matter, in
theory would be good to be consistent.

> (The qmp marshaller generated code does
> indeed leave the int64_t uninitialized.)

I didn't know that. We could change that, but maybe it's a good idea to
force people to rely on the has_ bool.