| Message ID | 1335195589-13285-4-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Il 23/04/2012 17:39, Stefan Hajnoczi ha scritto: > + /* Only set speed when necessary to avoid NotSupported error */ > + if (speed != 0) { > + Error *local_err = NULL; > + > + block_job_set_speed(job, speed, &local_err); > + if (error_is_set(&local_err)) { > + bs->job = NULL; > + g_free(job); > + bdrv_set_in_use(bs, 0); > + error_propagate(errp, local_err); > + return NULL; > + } > + } Similarly, here I would instead modify the QAPI entry point, and add a call to qmp_block_job_set_speed there. In patch 2, qmp_block_job_set_speed can parse the return code to distinguish ENOTSUP from EINVAL. Paolo
On Mon, 23 Apr 2012 16:39:48 +0100 Stefan Hajnoczi <stefanha@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 <stefanha@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. > + Error *local_err = NULL; > + > + block_job_set_speed(job, speed, &local_err); > + if (error_is_set(&local_err)) { > + bs->job = NULL; > + g_free(job); > + bdrv_set_in_use(bs, 0); > + error_propagate(errp, local_err); > + return NULL; > + } > + } > return job; > } > > diff --git a/block/stream.c b/block/stream.c > index f0486a3..dc15fb6 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -281,13 +281,14 @@ static BlockJobType stream_job_type = { > }; > > void stream_start(BlockDriverState *bs, BlockDriverState *base, > - const char *base_id, BlockDriverCompletionFunc *cb, > + const char *base_id, int64_t speed, > + BlockDriverCompletionFunc *cb, > void *opaque, Error **errp) > { > StreamBlockJob *s; > Coroutine *co; > > - s = block_job_create(&stream_job_type, bs, cb, opaque, errp); > + s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp); > if (!s) { > return; /* bs must already be in use */ > } > diff --git a/block_int.h b/block_int.h > index 6015c27..bffca35 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -343,6 +343,7 @@ int is_windows_drive(const char *filename); > * block_job_create: > * @job_type: The class object for the newly-created job. > * @bs: The block > + * @speed: The maximum speed, in bytes per second, or 0 for unlimited. > * @cb: Completion function for the job. > * @opaque: Opaque pointer value passed to @cb. > * @errp: A location to return DeviceInUse. > @@ -357,8 +358,8 @@ int is_windows_drive(const char *filename); > * called from a wrapper that is specific to the job type. > */ > 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); > > /** > * block_job_complete: > @@ -417,6 +418,7 @@ void block_job_cancel_sync(BlockJob *job); > * flatten the whole backing file chain onto @bs. > * @base_id: The file name that will be written to @bs as the new > * backing file if the job completes. Ignored if @base is %NULL. > + * @speed: The maximum speed, in bytes per second, or 0 for unlimited. > * @cb: Completion function for the job. > * @opaque: Opaque pointer value passed to @cb. > * @errp: A location to return DeviceInUse. > @@ -428,7 +430,8 @@ void block_job_cancel_sync(BlockJob *job); > * @base_id in the written image and to @base in the live BlockDriverState. > */ > void stream_start(BlockDriverState *bs, BlockDriverState *base, > - const char *base_id, BlockDriverCompletionFunc *cb, > + const char *base_id, int64_t speed, > + BlockDriverCompletionFunc *cb, > void *opaque, Error **errp); > > #endif /* BLOCK_INT_H */ > diff --git a/blockdev.c b/blockdev.c > index c484259..f18af16 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1091,7 +1091,8 @@ static void block_stream_cb(void *opaque, int ret) > } > > void qmp_block_stream(const char *device, bool has_base, > - const char *base, Error **errp) > + const char *base, bool has_speed, > + int64_t speed, Error **errp) > { > BlockDriverState *bs; > BlockDriverState *base_bs = NULL; > @@ -1110,7 +1111,8 @@ void qmp_block_stream(const char *device, bool has_base, > } > } > > - stream_start(bs, base_bs, base, block_stream_cb, bs, errp); > + stream_start(bs, base_bs, base, has_speed ? speed : 0, > + block_stream_cb, bs, errp); > if (error_is_set(errp)) { > return; > } > diff --git a/hmp-commands.hx b/hmp-commands.hx > index a6f5a84..66b70e1 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -71,8 +71,8 @@ ETEXI > > { > .name = "block_stream", > - .args_type = "device:B,base:s?", > - .params = "device [base]", > + .args_type = "device:B,speed:o?,base:s?", > + .params = "device [speed] [base]", > .help = "copy data from a backing file into a block device", > .mhandler.cmd = hmp_block_stream, > }, > diff --git a/hmp.c b/hmp.c > index f3e5163..eb96618 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -835,8 +835,10 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) > Error *error = NULL; > const char *device = qdict_get_str(qdict, "device"); > const char *base = qdict_get_try_str(qdict, "base"); > + int64_t speed = qdict_get_try_int(qdict, "speed", 0); > > - qmp_block_stream(device, base != NULL, base, &error); > + qmp_block_stream(device, base != NULL, base, > + qdict_haskey(qdict, "speed"), speed, &error); > > hmp_handle_error(mon, &error); > } > diff --git a/qapi-schema.json b/qapi-schema.json > index d0d4f693..9950451 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1571,15 +1571,19 @@ > # > # @base: #optional the common backing file name > # > +# @speed: #optional the maximum speed, in bytes per second > +# > # Returns: Nothing on success > # If streaming is already active on this device, DeviceInUse > # If @device does not exist, DeviceNotFound > # If image streaming is not supported by this device, NotSupported > # If @base does not exist, BaseNotFound > +# If @speed is invalid, BlockJobSpeedInvalid > # > # Since: 1.1 > ## > -{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str' } } > +{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str', > + '*speed': 'int' } } > > ## > # @block-job-set-speed: > diff --git a/qmp-commands.hx b/qmp-commands.hx > index f972332..a5ed6d0 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -688,7 +688,7 @@ EQMP > > { > .name = "block-stream", > - .args_type = "device:B,base:s?", > + .args_type = "device:B,base:s?,speed:o?", > .mhandler.cmd_new = qmp_marshal_input_block_stream, > }, >
On Tue, Apr 24, 2012 at 8:31 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Mon, 23 Apr 2012 16:39:48 +0100 > Stefan Hajnoczi <stefanha@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 <stefanha@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. (The qmp marshaller generated code does indeed leave the int64_t uninitialized.) Stefan
On Wed, 25 Apr 2012 11:56:51 +0100 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Tue, Apr 24, 2012 at 8:31 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote: > > On Mon, 23 Apr 2012 16:39:48 +0100 > > Stefan Hajnoczi <stefanha@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 <stefanha@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.
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) { + Error *local_err = NULL; + + block_job_set_speed(job, speed, &local_err); + if (error_is_set(&local_err)) { + bs->job = NULL; + g_free(job); + bdrv_set_in_use(bs, 0); + error_propagate(errp, local_err); + return NULL; + } + } return job; } diff --git a/block/stream.c b/block/stream.c index f0486a3..dc15fb6 100644 --- a/block/stream.c +++ b/block/stream.c @@ -281,13 +281,14 @@ static BlockJobType stream_job_type = { }; void stream_start(BlockDriverState *bs, BlockDriverState *base, - const char *base_id, BlockDriverCompletionFunc *cb, + const char *base_id, int64_t speed, + BlockDriverCompletionFunc *cb, void *opaque, Error **errp) { StreamBlockJob *s; Coroutine *co; - s = block_job_create(&stream_job_type, bs, cb, opaque, errp); + s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp); if (!s) { return; /* bs must already be in use */ } diff --git a/block_int.h b/block_int.h index 6015c27..bffca35 100644 --- a/block_int.h +++ b/block_int.h @@ -343,6 +343,7 @@ int is_windows_drive(const char *filename); * block_job_create: * @job_type: The class object for the newly-created job. * @bs: The block + * @speed: The maximum speed, in bytes per second, or 0 for unlimited. * @cb: Completion function for the job. * @opaque: Opaque pointer value passed to @cb. * @errp: A location to return DeviceInUse. @@ -357,8 +358,8 @@ int is_windows_drive(const char *filename); * called from a wrapper that is specific to the job type. */ 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); /** * block_job_complete: @@ -417,6 +418,7 @@ void block_job_cancel_sync(BlockJob *job); * flatten the whole backing file chain onto @bs. * @base_id: The file name that will be written to @bs as the new * backing file if the job completes. Ignored if @base is %NULL. + * @speed: The maximum speed, in bytes per second, or 0 for unlimited. * @cb: Completion function for the job. * @opaque: Opaque pointer value passed to @cb. * @errp: A location to return DeviceInUse. @@ -428,7 +430,8 @@ void block_job_cancel_sync(BlockJob *job); * @base_id in the written image and to @base in the live BlockDriverState. */ void stream_start(BlockDriverState *bs, BlockDriverState *base, - const char *base_id, BlockDriverCompletionFunc *cb, + const char *base_id, int64_t speed, + BlockDriverCompletionFunc *cb, void *opaque, Error **errp); #endif /* BLOCK_INT_H */ diff --git a/blockdev.c b/blockdev.c index c484259..f18af16 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1091,7 +1091,8 @@ static void block_stream_cb(void *opaque, int ret) } void qmp_block_stream(const char *device, bool has_base, - const char *base, Error **errp) + const char *base, bool has_speed, + int64_t speed, Error **errp) { BlockDriverState *bs; BlockDriverState *base_bs = NULL; @@ -1110,7 +1111,8 @@ void qmp_block_stream(const char *device, bool has_base, } } - stream_start(bs, base_bs, base, block_stream_cb, bs, errp); + stream_start(bs, base_bs, base, has_speed ? speed : 0, + block_stream_cb, bs, errp); if (error_is_set(errp)) { return; } diff --git a/hmp-commands.hx b/hmp-commands.hx index a6f5a84..66b70e1 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -71,8 +71,8 @@ ETEXI { .name = "block_stream", - .args_type = "device:B,base:s?", - .params = "device [base]", + .args_type = "device:B,speed:o?,base:s?", + .params = "device [speed] [base]", .help = "copy data from a backing file into a block device", .mhandler.cmd = hmp_block_stream, }, diff --git a/hmp.c b/hmp.c index f3e5163..eb96618 100644 --- a/hmp.c +++ b/hmp.c @@ -835,8 +835,10 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) Error *error = NULL; const char *device = qdict_get_str(qdict, "device"); const char *base = qdict_get_try_str(qdict, "base"); + int64_t speed = qdict_get_try_int(qdict, "speed", 0); - qmp_block_stream(device, base != NULL, base, &error); + qmp_block_stream(device, base != NULL, base, + qdict_haskey(qdict, "speed"), speed, &error); hmp_handle_error(mon, &error); } diff --git a/qapi-schema.json b/qapi-schema.json index d0d4f693..9950451 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1571,15 +1571,19 @@ # # @base: #optional the common backing file name # +# @speed: #optional the maximum speed, in bytes per second +# # Returns: Nothing on success # If streaming is already active on this device, DeviceInUse # If @device does not exist, DeviceNotFound # If image streaming is not supported by this device, NotSupported # If @base does not exist, BaseNotFound +# If @speed is invalid, BlockJobSpeedInvalid # # Since: 1.1 ## -{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str' } } +{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str', + '*speed': 'int' } } ## # @block-job-set-speed: diff --git a/qmp-commands.hx b/qmp-commands.hx index f972332..a5ed6d0 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -688,7 +688,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?", + .args_type = "device:B,base:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
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 <stefanha@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(-)