Re: [Qemu-devel] [PATCH v2 1/6] block: use Error mechanism instead of -errno for block_job_create()
On Tue, Apr 24, 2012 at 7:35 PM, Luiz Capitulino wrote:
> On Tue, 24 Apr 2012 14:53:55 +0100
> Stefan Hajnoczi wrote:
>
>> The block job API uses -errno return values internally and we convert
>> these to Error in the QMP functions. This is ugly because the Error
>> should be created at the point where we still have all the relevant
>> information. More importantly, it is hard to add new error cases to
>> this case since we quickly run out of -errno values without losing
>> information.
>>
>> Go ahead an use Error directly and don't convert later.
>>
>> Signed-off-by: Stefan Hajnoczi
>> ---
>> block.c | 4 +++-
>> block/stream.c | 11 +--
>> block_int.h | 11 +++
>> blockdev.c | 16 +---
>> 4 files changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index fe74ddd..2b72a0f 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4083,11 +4083,13 @@ out:
>> }
>>
>> void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
>> - BlockDriverCompletionFunc *cb, void *opaque)
>> + BlockDriverCompletionFunc *cb, void *opaque,
>> + Error **errp)
>> {
>> BlockJob *job;
>>
>> if (bs->job || bdrv_in_use(bs)) {
>> + error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>> return NULL;
>> }
>> bdrv_set_in_use(bs, 1);
>> diff --git a/block/stream.c b/block/stream.c
>> index 0efe1ad..7002dc8 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -280,16 +280,16 @@ static BlockJobType stream_job_type = {
>> .set_speed = stream_set_speed,
>> };
>>
>> -int stream_start(BlockDriverState *bs, BlockDriverState *base,
>> - const char *base_id, BlockDriverCompletionFunc *cb,
>> - void *opaque)
>> +void stream_start(BlockDriverState *bs, BlockDriverState *base,
>> + const char *base_id, BlockDriverCompletionFunc *cb,
>> + void *opaque, Error **errp)
>> {
>> StreamBlockJob *s;
>> Coroutine *co;
>>
>> - s = block_job_create(&stream_job_type, bs, cb, opaque);
>> + s = block_job_create(&stream_job_type, bs, cb, opaque, errp);
>> if (!s) {
>> - return -EBUSY; /* bs must already be in use */
>> + return;
>> }
>>
>> s->base = base;
>> @@ -300,5 +300,4 @@ int stream_start(BlockDriverState *bs, BlockDriverState
>> *base,
>> co = qemu_coroutine_create(stream_run);
>> trace_stream_start(bs, base, s, co, opaque);
>> qemu_coroutine_enter(co, s);
>> - return 0;
>> }
>> diff --git a/block_int.h b/block_int.h
>> index 0acb49f..8cf6ce9 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -346,6 +346,7 @@ int is_windows_drive(const char *filename);
>> * @bs: The block
>> * @cb: Completion function for the job.
>> * @opaque: Opaque pointer value passed to @cb.
>> + * @errp: A location to return DeviceInUse.
>
> Quite minor, but this is not a good description. I'd say just "Error object".
I followed the style of the glib documentation for GError** arguments.
I'm happy to change to just "Error object".
>
>> *
>> * Create a new long-running block device job and return it. The job
>> * will call @cb asynchronously when the job completes. Note that
>> @@ -357,7 +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);
>> + BlockDriverCompletionFunc *cb, void *opaque,
>> + Error **errp);
>>
>> /**
>> * block_job_complete:
>> @@ -417,6 +419,7 @@ void block_job_cancel_sync(BlockJob *job);
>> * backing file if the job completes. Ignored if @base is %NULL.
>> * @cb: Completion function for the job.
>> * @opaque: Opaque pointer value passed to @cb.
>> + * @errp: A location to return DeviceInUse.
>> *
>> * Start a streaming operation on @bs. Clusters that are unallocated
>> * in @bs, but allocated in any image between @base and @bs (both
>> @@ -424,8 +427,8 @@ void block_job_cancel_sync(BlockJob *job);
>> * streaming job, the backing file of @bs will be changed to
>> * @base_id in the written image and to @base in the live BlockDriverState.
>> */
>> -int stream_start(BlockDriverState *bs, BlockDriverState *base,
>> - const char *base_id, BlockDriverCompletionFunc *cb,
>> - void *opaque);
>> +void stream_start(BlockDriverState *bs, BlockDriverState *base,
>> + const char *base_id, BlockDriverCompletionFunc *cb,
>> + void *opaque, Error **errp);
>>
>> #endif /* BLOCK_INT_H */
>> diff --git a/blockdev.c b/blockdev.c
>> index 0c2440e..a411477 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1095,7 +1095,7 @@ void qmp_block_stream(const char *de
Re: [Qemu-devel] [PATCH v2 1/6] block: use Error mechanism instead of -errno for block_job_create()
On Tue, 24 Apr 2012 14:53:55 +0100
Stefan Hajnoczi wrote:
> The block job API uses -errno return values internally and we convert
> these to Error in the QMP functions. This is ugly because the Error
> should be created at the point where we still have all the relevant
> information. More importantly, it is hard to add new error cases to
> this case since we quickly run out of -errno values without losing
> information.
>
> Go ahead an use Error directly and don't convert later.
>
> Signed-off-by: Stefan Hajnoczi
> ---
> block.c|4 +++-
> block/stream.c | 11 +--
> block_int.h| 11 +++
> blockdev.c | 16 +---
> 4 files changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/block.c b/block.c
> index fe74ddd..2b72a0f 100644
> --- a/block.c
> +++ b/block.c
> @@ -4083,11 +4083,13 @@ out:
> }
>
> void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> - BlockDriverCompletionFunc *cb, void *opaque)
> + BlockDriverCompletionFunc *cb, void *opaque,
> + Error **errp)
> {
> BlockJob *job;
>
> if (bs->job || bdrv_in_use(bs)) {
> +error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> return NULL;
> }
> bdrv_set_in_use(bs, 1);
> diff --git a/block/stream.c b/block/stream.c
> index 0efe1ad..7002dc8 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -280,16 +280,16 @@ static BlockJobType stream_job_type = {
> .set_speed = stream_set_speed,
> };
>
> -int stream_start(BlockDriverState *bs, BlockDriverState *base,
> - const char *base_id, BlockDriverCompletionFunc *cb,
> - void *opaque)
> +void stream_start(BlockDriverState *bs, BlockDriverState *base,
> + const char *base_id, BlockDriverCompletionFunc *cb,
> + void *opaque, Error **errp)
> {
> StreamBlockJob *s;
> Coroutine *co;
>
> -s = block_job_create(&stream_job_type, bs, cb, opaque);
> +s = block_job_create(&stream_job_type, bs, cb, opaque, errp);
> if (!s) {
> -return -EBUSY; /* bs must already be in use */
> +return;
> }
>
> s->base = base;
> @@ -300,5 +300,4 @@ int stream_start(BlockDriverState *bs, BlockDriverState
> *base,
> co = qemu_coroutine_create(stream_run);
> trace_stream_start(bs, base, s, co, opaque);
> qemu_coroutine_enter(co, s);
> -return 0;
> }
> diff --git a/block_int.h b/block_int.h
> index 0acb49f..8cf6ce9 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -346,6 +346,7 @@ int is_windows_drive(const char *filename);
> * @bs: The block
> * @cb: Completion function for the job.
> * @opaque: Opaque pointer value passed to @cb.
> + * @errp: A location to return DeviceInUse.
Quite minor, but this is not a good description. I'd say just "Error object".
> *
> * Create a new long-running block device job and return it. The job
> * will call @cb asynchronously when the job completes. Note that
> @@ -357,7 +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);
> + BlockDriverCompletionFunc *cb, void *opaque,
> + Error **errp);
>
> /**
> * block_job_complete:
> @@ -417,6 +419,7 @@ void block_job_cancel_sync(BlockJob *job);
> * backing file if the job completes. Ignored if @base is %NULL.
> * @cb: Completion function for the job.
> * @opaque: Opaque pointer value passed to @cb.
> + * @errp: A location to return DeviceInUse.
> *
> * Start a streaming operation on @bs. Clusters that are unallocated
> * in @bs, but allocated in any image between @base and @bs (both
> @@ -424,8 +427,8 @@ void block_job_cancel_sync(BlockJob *job);
> * streaming job, the backing file of @bs will be changed to
> * @base_id in the written image and to @base in the live BlockDriverState.
> */
> -int stream_start(BlockDriverState *bs, BlockDriverState *base,
> - const char *base_id, BlockDriverCompletionFunc *cb,
> - void *opaque);
> +void stream_start(BlockDriverState *bs, BlockDriverState *base,
> + const char *base_id, BlockDriverCompletionFunc *cb,
> + void *opaque, Error **errp);
>
> #endif /* BLOCK_INT_H */
> diff --git a/blockdev.c b/blockdev.c
> index 0c2440e..a411477 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1095,7 +1095,7 @@ void qmp_block_stream(const char *device, bool has_base,
> {
> BlockDriverState *bs;
> BlockDriverState *base_bs = NULL;
> -int ret;
> +Error *local_err = NULL;
>
> bs = bdrv_find(device);
> if (!bs) {
> @@ -,16 +,10 @@ void qmp_block_stream(const char *device, bool
> has_base,
Re: [Qemu-devel] [PATCH v2 1/6] block: use Error mechanism instead of -errno for block_job_create()
On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote: > The block job API uses -errno return values internally and we convert > these to Error in the QMP functions. This is ugly because the Error > should be created at the point where we still have all the relevant > information. More importantly, it is hard to add new error cases to > this case since we quickly run out of -errno values without losing > information. > > Go ahead an use Error directly and don't convert later. s/an /and / -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
