Re: [Qemu-block] [PATCH 2/6] block: allow block jobs in any arbitrary node

[Alberto Garcia]

On Wed 15 Apr 2015 05:22:13 PM CEST, Max Reitz <address@hidden> wrote:

>> diff --git a/block/mirror.c b/block/mirror.c
>> index 4056164..189e8f8 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -607,8 +607,9 @@ static void mirror_complete(BlockJob *job, Error **errp)
>>           return;
>>       }
>>       if (!s->synced) {
>> -        error_set(errp, QERR_BLOCK_JOB_NOT_READY,
>> -                  bdrv_get_device_name(job->bs));
>> +        error_setg(errp,
>> +                   "The active block job for device '%s' cannot be 
>> completed",
>> +                   bdrv_get_device_name(job->bs));
>
> I know there is no way right now that bdrv_get_device_name() returns
> an empty string, but somehow I'd feel more consistent for this to be
> bdrv_get_device_or_node_name() and the string saying "node" instead of
> "device". Your choice, though, since it's completely correct for now.

Yeah, I decided to leave it like that while the mirror operation can
only work on root nodes. In general in all these patches I'm only using
bdrv_get_device_or_node_name() in the cases where it's actually possible
that the device name is empty.

>> +/* Get the block job for a given device or node name
>> + * and acquire its AioContext */
>> +static BlockJob *find_block_job(const char *device_or_node,
>> +                                AioContext **aio_context,
>>                                   Error **errp)
>>   {
>> -    BlockBackend *blk;
>>       BlockDriverState *bs;
>>   
>> -    blk = blk_by_name(device);
>> -    if (!blk) {
>> +    bs = bdrv_lookup_bs(device_or_node, device_or_node, NULL);
>> +    if (!bs) {
>>           goto notfound;
>
> It would be possible to pass errp to bdrv_lookup_bs() and move the
> error_set() under notfound to the if (!bs->job) block (I'd find the
> error message confusing if there just is no node with that name; but
> on the other hand, it wouldn't be a regression).

Sounds reasonable, I'll change that.

>> diff --git a/blockjob.c b/blockjob.c
>> index 562362b..b2b6cc9 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -52,7 +52,7 @@ void *block_job_create(const BlockJobDriver *driver, 
>> BlockDriverState *bs,
>>       BlockJob *job;
>>   
>>       if (bs->job) {
>> -        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>> +        error_set(errp, QERR_DEVICE_IN_USE, 
>> bdrv_get_device_or_node_name(bs));
>
> Hm, the error message only mentions "device" now... Not too nice.

Errr... I overlooked that, I'll fix it.

>> @@ -1895,7 +1895,7 @@
>>   #
>>   # @type: job type
>>   #
>> -# @device: device name
>> +# @device: device name, or node name if not present
>>   #
>>   # @len: maximum progress value
>>   #
>
> I think you need to apply the same treatment for the comment of 
> BlockJobInfo, too.

You're right again :)

Berto