[Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/block.c b/block.c
index 1a965b2..00b6f21 100644
--- a/block.c
+++ b/block.c
@@ -603,10 +603,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 =
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[malc]

On Wed, 27 Oct 2010, Anthony Liguori wrote:

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/block.c b/block.c
> index 1a965b2..00b6f21 100644
> --- a/block.c
> +++ b/block.c
> @@ -603,10 +603,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);

Sigh..

> +        } else {
> +            pstrcpy(backing_filename, sizeof(backing_filename),
> +                    bs->backing_file);
> +        }
>  
>          /* backing files always opened read-only */
>          back_flags =
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Anthony Liguori]

On 10/27/2010 02:22 PM, malc wrote:
> On Wed, 27 Oct 2010, Anthony Liguori wrote:
>
>    
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/block.c b/block.c
>> index 1a965b2..00b6f21 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -603,10 +603,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);
>>      
> Sigh..
>    

Always good to see such clear and constructive comments...

I'll respin.

Regards,

Anthony Liguori

>    
>> +        } else {
>> +            pstrcpy(backing_filename, sizeof(backing_filename),
>> +                    bs->backing_file);
>> +        }
>>
>>           /* backing files always opened read-only */
>>           back_flags =
>>
>>      
>    

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Stefan Hajnoczi]

On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> diff --git a/block.c b/block.c
> index 1a965b2..00b6f21 100644
> --- a/block.c
> +++ b/block.c
> @@ -603,10 +603,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 =
> --
> 1.7.0.4

I think this makes sense.

Now it is possible to specify backing files that are relative to
QEMU's current working directory using file:filename.  I don't see
harm in this.

Stefan

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Kevin Wolf]

Am 28.10.2010 10:30, schrieb Stefan Hajnoczi:
> On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> diff --git a/block.c b/block.c
>> index 1a965b2..00b6f21 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -603,10 +603,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 =
>> --
>> 1.7.0.4
> 
> I think this makes sense.
> 
> Now it is possible to specify backing files that are relative to
> QEMU's current working directory using file:filename.  I don't see
> harm in this.

It would be more consistent if it was relative to the image, but there's
no meaningful way to do that for arbitrary protocols. It's definitely
not worse than before the change.

Kevin

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Daniel P. Berrange]

On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote:
> On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >
> > diff --git a/block.c b/block.c
> > index 1a965b2..00b6f21 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -603,10 +603,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 =
> > --
> > 1.7.0.4
> 
> I think this makes sense.
> 
> Now it is possible to specify backing files that are relative to
> QEMU's current working directory using file:filename.  I don't see
> harm in this.

Shouldn't a backing file be treated as relative to the image file pointing
to it, rather than the QEMU working directory. eg so you can do

  # qemu-img create backing.img
  # qemu-img create -o backing_file=file:backing.img main.img
  
And have main.img be able to resolve backing.img in its same directory,
no matter what directory QEMU itself is executing from

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Stefan Hajnoczi]

On Thu, Oct 28, 2010 at 10:35:02AM +0100, Daniel P. Berrange wrote:
> On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote:
> > On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > >
> > > diff --git a/block.c b/block.c
> > > index 1a965b2..00b6f21 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -603,10 +603,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 =
> > > --
> > > 1.7.0.4
> > 
> > I think this makes sense.
> > 
> > Now it is possible to specify backing files that are relative to
> > QEMU's current working directory using file:filename.  I don't see
> > harm in this.
> 
> Shouldn't a backing file be treated as relative to the image file pointing
> to it, rather than the QEMU working directory. eg so you can do
> 
>   # qemu-img create backing.img
>   # qemu-img create -o backing_file=file:backing.img main.img
>   
> And have main.img be able to resolve backing.img in its same directory,
> no matter what directory QEMU itself is executing from

This works relative to main.img:

# qemu-img create -o backing_file=backing.img main.img

but this works relative to QEMU's cwd:

# qemu-img create -o backing_file=file:backing.img main.img

As Kevin mentioned, special-casing the file: protocol isn't ideal.  We
shouldn't make assumptions about specific protocols.

Stefan

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Kevin Wolf]

Am 28.10.2010 11:35, schrieb Daniel P. Berrange:
> On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote:
>> On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>
>>> diff --git a/block.c b/block.c
>>> index 1a965b2..00b6f21 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -603,10 +603,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 =
>>> --
>>> 1.7.0.4
>>
>> I think this makes sense.
>>
>> Now it is possible to specify backing files that are relative to
>> QEMU's current working directory using file:filename.  I don't see
>> harm in this.
> 
> Shouldn't a backing file be treated as relative to the image file pointing
> to it, rather than the QEMU working directory. eg so you can do
> 
>   # qemu-img create backing.img
>   # qemu-img create -o backing_file=file:backing.img main.img
>   
> And have main.img be able to resolve backing.img in its same directory,
> no matter what directory QEMU itself is executing from

The problem is that this wouldn't work in the general case. It's rather
an exception that it makes sense for file: backing files with file:
images. Consider this:

  # qemu-img create -o backing_file=nbd:foo:1234 /tmp/main.img

Without this patch, you'll end up with /tmp/nbd:foo:1234, which is
probably not what you wanted. With a patch that would work for file: you
would get a hardly better path nbd:/tmp/foo:1234

Kevin

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Daniel P. Berrange]

On Thu, Oct 28, 2010 at 11:49:58AM +0200, Kevin Wolf wrote:
> Am 28.10.2010 11:35, schrieb Daniel P. Berrange:
> > On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote:
> >> On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index 1a965b2..00b6f21 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -603,10 +603,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 =
> >>> --
> >>> 1.7.0.4
> >>
> >> I think this makes sense.
> >>
> >> Now it is possible to specify backing files that are relative to
> >> QEMU's current working directory using file:filename.  I don't see
> >> harm in this.
> > 
> > Shouldn't a backing file be treated as relative to the image file pointing
> > to it, rather than the QEMU working directory. eg so you can do
> > 
> >   # qemu-img create backing.img
> >   # qemu-img create -o backing_file=file:backing.img main.img
> >   
> > And have main.img be able to resolve backing.img in its same directory,
> > no matter what directory QEMU itself is executing from
> 
> The problem is that this wouldn't work in the general case. It's rather
> an exception that it makes sense for file: backing files with file:
> images. Consider this:
> 
>   # qemu-img create -o backing_file=nbd:foo:1234 /tmp/main.img
> 
> Without this patch, you'll end up with /tmp/nbd:foo:1234, which is
> probably not what you wanted. With a patch that would work for file: you
> would get a hardly better path nbd:/tmp/foo:1234

Since we know the protocol, there could be a per-protocol function used
for resolving the backing store URI relative to the master URI. That
would avoid needing to special case file: in the shared generic code.

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Anthony Liguori]

On 10/28/2010 04:51 AM, Daniel P. Berrange wrote:
>> The problem is that this wouldn't work in the general case. It's rather
>> an exception that it makes sense for file: backing files with file:
>> images. Consider this:
>>
>>    # qemu-img create -o backing_file=nbd:foo:1234 /tmp/main.img
>>
>> Without this patch, you'll end up with /tmp/nbd:foo:1234, which is
>> probably not what you wanted. With a patch that would work for file: you
>> would get a hardly better path nbd:/tmp/foo:1234
>>      
> Since we know the protocol, there could be a per-protocol function used
> for resolving the backing store URI relative to the master URI. That
> would avoid needing to special case file: in the shared generic code.
>    

Relative resolution of a backing files makes me very nervous.  Any time 
a disk image can reasonably resolve to something other than what the 
user expected is potentially a very nasty security issue.

The less obvious the resolution, the worse the problem becomes.  I think 
resolution based on current path is probably the most obvious 
implementation.

Regards,

Anthony Liguori

> Daniel
>    

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Kevin Wolf]

Am 27.10.2010 20:19, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/block.c b/block.c
> index 1a965b2..00b6f21 100644
> --- a/block.c
> +++ b/block.c
> @@ -603,10 +603,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) {

If no protocol is specified, bdrv_find_protocol doesn't return NULL but
the file: driver.

This breaks all backing file related qemu-iotests cases because qcow2
backing files are opened as raw files now.

Kevin

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Anthony Liguori]

On 11/04/2010 07:54 AM, Kevin Wolf wrote:
> Am 27.10.2010 20:19, schrieb Anthony Liguori:
>    
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/block.c b/block.c
>> index 1a965b2..00b6f21 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -603,10 +603,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) {
>>      
> If no protocol is specified, bdrv_find_protocol doesn't return NULL but
> the file: driver.
>    

An ugly way to handle this would be to do if (strstr(bs->backing_file, 
":") == NULL) instead.

A deeper refactoring could return NULL in bdrv_find_protocol and fixup 
the callers to default to file: if none are specified.

What do you think?

Regards,

Anthony Liguori



> This breaks all backing file related qemu-iotests cases because qcow2
> backing files are opened as raw files now.
>
> Kevin
>    

Re: [Qemu-devel] [PATCH] block: fix the use of protocols in backing files

[Kevin Wolf]

Am 04.11.2010 14:14, schrieb Anthony Liguori:
> On 11/04/2010 07:54 AM, Kevin Wolf wrote:
>> Am 27.10.2010 20:19, schrieb Anthony Liguori:
>>    
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>
>>> diff --git a/block.c b/block.c
>>> index 1a965b2..00b6f21 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -603,10 +603,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) {
>>>      
>> If no protocol is specified, bdrv_find_protocol doesn't return NULL but
>> the file: driver.
>>    
> 
> An ugly way to handle this would be to do if (strstr(bs->backing_file, 
> ":") == NULL) instead.

Ugly indeed.

> A deeper refactoring could return NULL in bdrv_find_protocol and fixup 
> the callers to default to file: if none are specified.

NULL is already used for errors, so we'd have to have something like

  int bdrv_find_protocol(const char *filename, BlockDriver **drv)

in order to be able to distinguish "invalid protocol" from "no explicit
protocol". You could rename this function and retain a
bdrv_find_protocol with the old prototype as a wrapper that returns file
instead of NULL (there are several callers that expect this behaviour,
so probably it makes sense to have it in a central place).

Does that sound reasonable?

Kevin