Re: [Qemu-devel] [PATCH v2 1/2] block/drive-mirror: Check for NULL backing_hd

[Fam Zheng <famz@redhat.com>]

On 11/05/2013 08:35 AM, Max Reitz wrote:
> It should be possible to execute the QMP "drive-mirror" command in
> "none" sync mode and "absolute-paths" mode even for block devices
> lacking a backing file.
>
> "absolute-paths" does in fact not require a backing file to be present,
> as can be seen from the "top" sync mode code path. "top" basically
> states that the device should indeed have a backing file - however, the
> current code catches the case if it doesn't and then simply treats it as
> "full" sync mode, creating a target image without a backing file (in
> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
> file must indeed have a backing file.
>
> Therefore, the target file may be left unbacked in case of "none" sync
> mode as well, if the specified device is not backed either. Currently,
> qemu will crash trying to dereference the backing file pointer since it
> assumes that it will always be non-NULL in that case ("none" with
> "absolute-paths").
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   blockdev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index b260477..986e59d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2038,8 +2038,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>           case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
>               /* create new image with backing file */
>               bdrv_img_create(target, format,
> -                            source->filename,
> -                            source->drv->format_name,
> +                            source ? source->filename : NULL,
> +                            source ? source->drv->format_name : NULL,
>                               NULL, size, flags, &local_err, false);
>               break;
>           default:
The code around here is:

2029     if (sync == MIRROR_SYNC_MODE_FULL && mode != 
NEW_IMAGE_MODE_EXISTING) {
2030         /* create new image w/o backing file */
2031         assert(format && drv);
2032         bdrv_img_create(target, format,
2033                         NULL, NULL, NULL, size, flags, &local_err, 
false);
2034     } else {
2035         switch (mode) {
2036         case NEW_IMAGE_MODE_EXISTING:
2037             break;
2038         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
2039             /* create new image with backing file */
2040             bdrv_img_create(target, format,
2041                             source->filename,
2042                             source->drv->format_name,
2043                             NULL, size, flags, &local_err, false);
2044             break;
2045         default:
2046             abort();
2047         }
2048     }

Why not update the if condition and reuse the branch, I think this is a 
better branching? Either should be fine, but in your change you should 
also update the comment in line 2039.

Thanks,
Fam