On Wed, 25 Apr 2012 11:56:51 +0100
Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Tue, Apr 24, 2012 at 8:31 PM, Luiz Capitulino <lcapitul...@redhat.com>
> wrote:
> > On Mon, 23 Apr 2012 16:39:48 +0100
> > Stefan Hajnoczi <stefa...@linux.vnet.ibm.com> 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 <stefa...@linux.vnet.ibm.com>
> >> ---
> >> 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.
