Re: [Qemu-devel] Re: [PATCH 2/3] block-nbd: fix use of protocols in backing files and nbd probing

[Kevin Wolf]

Am 16.09.2010 17:40, schrieb Anthony Liguori:
> On 09/15/2010 11:06 AM, Juan Quintela wrote:
>> Anthony Liguori<address@hidden>  wrote:
>>    
>>> The use of protocols in backing_files is currently broken because of some
>>> checks for adjusting relative pathnames.
>>>
>>> Additionally, there's a spurious read when using an nbd protocol that can be
>>> quite destructive when using copy-on-read.  Potentially, this can lead to
>>> probing an image file over top of NBD but this is completely wrong as NBD
>>> devices are not growable.
>>>
>>> Signed-off-by: Anthony Liguori<address@hidden>
>>> ---
>>> NB: this is absolutely not ideal.  A more elegant suggestion would be
>>> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
>>> stands today.
>>>      
>> Bad, bad boy, you fixed two things in a single patch.
>>    
> 
> You're not supposed to notice that :-)  It was an RFC series, I was 
> really just looking to start a conversation.
> 
>>> @@ -603,10 +610,16 @@ int bdrv_open(BlockDriverState *bs, const char 
>>> *filename, int flags,
>>>           BlockDriver *back_drv = NULL;
>>>
>>>           bs->backing_hd = bdrv_new("");
>>> -        path_combine(backing_filename, sizeof(backing_filename),
>>> -                     filename, bs->backing_file);
>>> -        if (bs->backing_format[0] != '\0')
>>> -            back_drv = bdrv_find_format(bs->backing_format);
>>> +        back_drv = bdrv_find_protocol(bs->backing_file);
>>> +        if (!back_drv) {
>>> +            path_combine(backing_filename, sizeof(backing_filename),
>>> +                         filename, bs->backing_file);
>>> +            if (bs->backing_format[0] != '\0')
>>> +                back_drv = bdrv_find_format(bs->backing_format);
>>> +        } else {
>>> +            pstrcpy(backing_filename, sizeof(backing_filename),
>>> +                    bs->backing_file);
>>> +        }
>>>
>>>           /* backing files always opened read-only */
>>>           back_flags =
>>>      
>> But this one breaks my setup, I have to backout this patch to be able to
>> launch guests with qcow2 file images.
>>    
> 
> Kevin, do you have an opinion about the best way to resolve this?
> 
> The relative/absolute path is broken for non-file URIs.  I think we 
> could mark a protocol as having a file URI type or we could push the 
> absolute/relative conversion down to the file/phys_dev protocol.
> 
> I think the former approach is probably the least invasive.

I agree. It gets a bit hard for things like blkdebug or blkverify which
actually get two file names, but I guess they usually won't be used as a
backing file - and if they do, it's probably reasonable to require
absolute paths or to resolve them relative to the working directory
instead of the image. They are only for debugging anyway.

Kevin