Re: [Qemu-devel] [PATCH v5 04/11] blockdev: drop redundant proto_drv check

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

于 2013-5-30 20:34, Stefan Hajnoczi 写道:
> It is not necessary to check that we can find a protocol block driver
> since we create or open the image file.  This produces the error that we
> need anyway.
> 
> Besides, the QERR_INVALID_BLOCK_FORMAT is inappropriate since the
> protocol is incorrect rather than the format.
> 
> Also drop an empty line between bdrv_open() and checking its return
> value.  This may be due to copy-pasting from earlier code that performed
> other operations before handling errors.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c | 15 ---------------
>   1 file changed, 15 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b9b2d10..01db519 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -818,7 +818,6 @@ typedef struct ExternalSnapshotStates {
>   static void external_snapshot_prepare(BlkTransactionStates *common,
>                                         Error **errp)
>   {
> -    BlockDriver *proto_drv;
>       BlockDriver *drv;
>       int flags, ret;
>       Error *local_err = NULL;
> @@ -874,12 +873,6 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
> 
>       flags = states->old_bs->open_flags;
> 
> -    proto_drv = bdrv_find_protocol(new_image_file);
> -    if (!proto_drv) {
> -        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> -        return;
> -    }
> -
>       /* create new image w/backing file */
>       if (mode != NEW_IMAGE_MODE_EXISTING) {
>           bdrv_img_create(new_image_file, format,
> @@ -1368,7 +1361,6 @@ void qmp_drive_mirror(const char *device, const char *target,
>   {
>       BlockDriverState *bs;
>       BlockDriverState *source, *target_bs;
> -    BlockDriver *proto_drv;
>       BlockDriver *drv = NULL;
>       Error *local_err = NULL;
>       int flags;
> @@ -1436,12 +1428,6 @@ void qmp_drive_mirror(const char *device, const char *target,
>           sync = MIRROR_SYNC_MODE_FULL;
>       }
> 
> -    proto_drv = bdrv_find_protocol(target);
> -    if (!proto_drv) {
> -        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> -        return;
> -    }
> -
>       bdrv_get_geometry(bs, &size);
>       size *= 512;
>       if (sync == MIRROR_SYNC_MODE_FULL && mode != NEW_IMAGE_MODE_EXISTING) {
> @@ -1476,7 +1462,6 @@ void qmp_drive_mirror(const char *device, const char *target,
>        */
>       target_bs = bdrv_new("");
>       ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv);
> -
>       if (ret < 0) {
>           bdrv_delete(target_bs);
>           error_set(errp, QERR_OPEN_FILE_FAILED, target);
> 
  Although it is not merged yet, I'll use it as pre-patches in my
series which add internal snapshot support in qmp_transaction.

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

-- 
Best Regards

Wenchao Xia