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

[Stefan Hajnoczi]

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.  (The qmp marshaller generated code does
indeed leave the int64_t uninitialized.)

Stefan