On Wed 15 Apr 2015 05:22:13 PM CEST, Max Reitz <[email protected]> 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
