Re: [Qemu-devel] [PATCH V10 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-3-28 17:54, Kevin Wolf 写道:
> Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben:
>>    Now image info will be retrieved as an embbed json object inside
>> BlockDeviceInfo, backing chain info and all related internal snapshot
>> info can be got in the enhanced recursive structure of ImageInfo.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c         |   39 ++++++++++++++++++++++++++--
>>   include/block/qapi.h |    4 ++-
>>   qapi-schema.json     |    5 +++-
>>   qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 109 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index df73f5b..9051947 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
>>       return 0;
>>   }
>>
>> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
>> +/* return 0 on success, and @p_info will be set only on success. */
>> +int bdrv_query_info(BlockDriverState *bs,
>> +                    BlockInfo **p_info,
>> +                    Error **errp)
>>   {
>>       BlockInfo *info = g_malloc0(sizeof(*info));
>> +    BlockDriverState *bs0;
>> +    ImageInfo **p_image_info;
>> +    int ret = 0;
>
> ret is never changed, so this function always returns 0. I would suggest
> to drop ret and make the function return type void.
>
   My bad, I forgot to set its value, the interface is intend to return
negative error number and errp both on fail.

>>       info->device = g_strdup(bs->device_name);
>>       info->type = g_strdup("unknown");
>>       info->locked = bdrv_dev_is_medium_locked(bs);
>> @@ -256,8 +262,29 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
>>               info->inserted->iops_wr =
>>                              bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
>>           }
>> +
>> +        bs0 = bs;
>> +        p_image_info = &info->inserted->image;
>> +        while (1) {
>> +            if (bdrv_query_image_info(bs0, p_image_info, errp)) {
>> +                goto err;
>> +            }
   Sorry ret is missing here, it should be:
             ret = bdrv_query_image_info(bs0, p_image_info, errp));
             if (ret) {
                 goto err;
             }
   I'll correct it.

>> +            if (bs0->drv && bs0->backing_hd) {
>> +                bs0 = bs0->backing_hd;
>> +                (*p_image_info)->has_backing_image = true;
>> +                p_image_info = &((*p_image_info)->backing_image);
>> +            } else {
>> +                break;
>> +            }
>> +        }
>>       }
>> -    return info;
>> +
>> +    *p_info = info;
>> +    return 0;
>> +
>> + err:
>> +    qapi_free_BlockInfo(info);
>> +    return ret;
>>   }
>>
>>   SnapshotInfoList *qmp_query_snapshots(Error **errp)
>> @@ -284,11 +311,17 @@ BlockInfoList *qmp_query_block(Error **errp)
>>
>>       while ((bs = bdrv_next(bs))) {
>>           BlockInfoList *info = g_malloc0(sizeof(*info));
>> -        info->value = bdrv_query_info(bs);
>> +        if (bdrv_query_info(bs, &info->value, errp)) {
>> +            goto err;
>> +        }
>
> Consequently, you've got the error handling wrong here, the if condition
> is never true. It should look more or less like this (the syntax details
> may be wrong):
>
> Error *local_err;
> bdrv_query_info(bs, &info->value, &local_err);
> if (error_is_set(local_err)) {
>      error_propagate(err, local_err);
>      goto err;
> }
>
> Kevin
>


-- 
Best Regards

Wenchao Xia