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

[Max Reitz]

On 05.11.2013 04:32, Fam Zheng wrote:
>
> 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 <address@hidden>
>> ---
>>   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.

Okay, I'll go for updating the condition.

Max

> Thanks,
> Fam