[Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Jeff Cody]

In bdrv_find_backing_image(), if we are searching an image for a backing
file that contains a protocol, we currently only compare unmodified
paths.

However, some management software will change the backing filename to be
a relative filename in a path.  QEMU is able to handle this fine,
because internally it will use path_combine to put together the full
protocol URI.

However, this can lead to an inability to match an image during a QAPI
command that needs to use bdrv_find_backing_image() to find the image,
when it is searched by the full URI.

When searching for a protocol filename, if the straight comparison
fails, this patch will also compare against the full backing filename to
see if that is a match.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block.c b/block.c
index 39ddea3..a173afc 100644
--- a/block.c
+++ b/block.c
@@ -3145,6 +3145,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     int is_protocol = 0;
     BlockDriverState *curr_bs = NULL;
     BlockDriverState *retval = NULL;
+    Error *local_error = NULL;
 
     if (!bs || !bs->drv || !backing_file) {
         return NULL;
@@ -3165,6 +3166,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
                 retval = curr_bs->backing->bs;
                 break;
             }
+            /* Also check against the full backing filename for the image */
+            bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
+                                           &local_error);
+            if (local_error == NULL) {
+                if (strcmp(backing_file, backing_file_full) == 0) {
+                    retval = curr_bs->backing->bs;
+                    break;
+                }
+            } else {
+                error_free(local_error);
+            }
         } else {
             /* If not an absolute filename path, make it relative to the current
              * image's filename path */
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Jeff Cody]

Forgot to cc qemu-block, added.

On Wed, Jan 25, 2017 at 12:22:02PM -0500, Jeff Cody wrote:
> In bdrv_find_backing_image(), if we are searching an image for a backing
> file that contains a protocol, we currently only compare unmodified
> paths.
> 
> However, some management software will change the backing filename to be
> a relative filename in a path.  QEMU is able to handle this fine,
> because internally it will use path_combine to put together the full
> protocol URI.
> 
> However, this can lead to an inability to match an image during a QAPI
> command that needs to use bdrv_find_backing_image() to find the image,
> when it is searched by the full URI.
> 
> When searching for a protocol filename, if the straight comparison
> fails, this patch will also compare against the full backing filename to
> see if that is a match.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 39ddea3..a173afc 100644
> --- a/block.c
> +++ b/block.c
> @@ -3145,6 +3145,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>      int is_protocol = 0;
>      BlockDriverState *curr_bs = NULL;
>      BlockDriverState *retval = NULL;
> +    Error *local_error = NULL;
>  
>      if (!bs || !bs->drv || !backing_file) {
>          return NULL;
> @@ -3165,6 +3166,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>                  retval = curr_bs->backing->bs;
>                  break;
>              }
> +            /* Also check against the full backing filename for the image */
> +            bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
> +                                           &local_error);
> +            if (local_error == NULL) {
> +                if (strcmp(backing_file, backing_file_full) == 0) {
> +                    retval = curr_bs->backing->bs;
> +                    break;
> +                }
> +            } else {
> +                error_free(local_error);
> +            }
>          } else {
>              /* If not an absolute filename path, make it relative to the current
>               * image's filename path */
> -- 
> 2.9.3
> 

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On 01/25/2017 11:22 AM, Jeff Cody wrote:
> In bdrv_find_backing_image(), if we are searching an image for a backing
> file that contains a protocol, we currently only compare unmodified
> paths.
> 
> However, some management software will change the backing filename to be
> a relative filename in a path.  QEMU is able to handle this fine,
> because internally it will use path_combine to put together the full
> protocol URI.
> 
> However, this can lead to an inability to match an image during a QAPI
> command that needs to use bdrv_find_backing_image() to find the image,
> when it is searched by the full URI.
> 
> When searching for a protocol filename, if the straight comparison
> fails, this patch will also compare against the full backing filename to
> see if that is a match.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Does this overlap with Max' work on 'block: Fix some filename generation
issues'?  But in isolation, it looks okay to me,

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block.c b/block.c
> index 39ddea3..a173afc 100644
> --- a/block.c
> +++ b/block.c
> @@ -3145,6 +3145,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>      int is_protocol = 0;
>      BlockDriverState *curr_bs = NULL;
>      BlockDriverState *retval = NULL;
> +    Error *local_error = NULL;

I might have scoped this...

>  
>      if (!bs || !bs->drv || !backing_file) {
>          return NULL;
> @@ -3165,6 +3166,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>                  retval = curr_bs->backing->bs;
>                  break;
>              }

...as the first line within the if that uses it. But no big deal.

> +            /* Also check against the full backing filename for the image */
> +            bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
> +                                           &local_error);
> +            if (local_error == NULL) {
> +                if (strcmp(backing_file, backing_file_full) == 0) {
> +                    retval = curr_bs->backing->bs;
> +                    break;
> +                }
> +            } else {
> +                error_free(local_error);
> +            }
>          } else {
>              /* If not an absolute filename path, make it relative to the current
>               * image's filename path */
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On 25.01.2017 18:22, Jeff Cody wrote:
> In bdrv_find_backing_image(), if we are searching an image for a backing
> file that contains a protocol, we currently only compare unmodified
> paths.
> 
> However, some management software will change the backing filename to be
> a relative filename in a path.  QEMU is able to handle this fine,
> because internally it will use path_combine to put together the full
> protocol URI.
> 
> However, this can lead to an inability to match an image during a QAPI
> command that needs to use bdrv_find_backing_image() to find the image,
> when it is searched by the full URI.
> 
> When searching for a protocol filename, if the straight comparison
> fails, this patch will also compare against the full backing filename to
> see if that is a match.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Thanks, applied to my block tree:

https://github.com/XanClic/qemu/commits/block


How much would you mind writing an iotest?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

On 25.01.2017 19:24, Eric Blake wrote:
> On 01/25/2017 11:22 AM, Jeff Cody wrote:
>> In bdrv_find_backing_image(), if we are searching an image for a backing
>> file that contains a protocol, we currently only compare unmodified
>> paths.
>>
>> However, some management software will change the backing filename to be
>> a relative filename in a path.  QEMU is able to handle this fine,
>> because internally it will use path_combine to put together the full
>> protocol URI.
>>
>> However, this can lead to an inability to match an image during a QAPI
>> command that needs to use bdrv_find_backing_image() to find the image,
>> when it is searched by the full URI.
>>
>> When searching for a protocol filename, if the straight comparison
>> fails, this patch will also compare against the full backing filename to
>> see if that is a match.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
> 
> Does this overlap with Max' work on 'block: Fix some filename generation
> issues'?  But in isolation, it looks okay to me,

Yes, it does, but it's not too hard to fix up in my series. Better get
this in first and then do the trivial change on my side.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Noted :-)

Max

>>
>> diff --git a/block.c b/block.c
>> index 39ddea3..a173afc 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3145,6 +3145,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>      int is_protocol = 0;
>>      BlockDriverState *curr_bs = NULL;
>>      BlockDriverState *retval = NULL;
>> +    Error *local_error = NULL;
> 
> I might have scoped this...
> 
>>  
>>      if (!bs || !bs->drv || !backing_file) {
>>          return NULL;
>> @@ -3165,6 +3166,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>                  retval = curr_bs->backing->bs;
>>                  break;
>>              }
> 
> ...as the first line within the if that uses it. But no big deal.
> 
>> +            /* Also check against the full backing filename for the image */
>> +            bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
>> +                                           &local_error);
>> +            if (local_error == NULL) {
>> +                if (strcmp(backing_file, backing_file_full) == 0) {
>> +                    retval = curr_bs->backing->bs;
>> +                    break;
>> +                }
>> +            } else {
>> +                error_free(local_error);
>> +            }
>>          } else {
>>              /* If not an absolute filename path, make it relative to the current
>>               * image's filename path */
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]

On 25.01.2017 18:22, Jeff Cody wrote:
> In bdrv_find_backing_image(), if we are searching an image for a backing
> file that contains a protocol, we currently only compare unmodified
> paths.
> 
> However, some management software will change the backing filename to be
> a relative filename in a path.  QEMU is able to handle this fine,
> because internally it will use path_combine to put together the full
> protocol URI.
> 
> However, this can lead to an inability to match an image during a QAPI
> command that needs to use bdrv_find_backing_image() to find the image,
> when it is searched by the full URI.
> 
> When searching for a protocol filename, if the straight comparison
> fails, this patch will also compare against the full backing filename to
> see if that is a match.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 39ddea3..a173afc 100644
> --- a/block.c
> +++ b/block.c
> @@ -3145,6 +3145,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>      int is_protocol = 0;
>      BlockDriverState *curr_bs = NULL;
>      BlockDriverState *retval = NULL;
> +    Error *local_error = NULL;
>  
>      if (!bs || !bs->drv || !backing_file) {
>          return NULL;
> @@ -3165,6 +3166,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>                  retval = curr_bs->backing->bs;
>                  break;
>              }
> +            /* Also check against the full backing filename for the image */
> +            bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
> +                                           &local_error);
> +            if (local_error == NULL) {
> +                if (strcmp(backing_file, backing_file_full) == 0) {
> +                    retval = curr_bs->backing->bs;
> +                    break;
> +                }
> +            } else {
> +                error_free(local_error);

Oops, I was a bit too quick there. You should either follow Eric's
remark about the scope, or you have to reset local_error to NULL here.

Max

> +            }
>          } else {
>              /* If not an absolute filename path, make it relative to the current
>               * image's filename path */
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Jeff Cody]

On Wed, Jan 25, 2017 at 07:24:48PM +0100, Max Reitz wrote:
> On 25.01.2017 18:22, Jeff Cody wrote:
> > In bdrv_find_backing_image(), if we are searching an image for a backing
> > file that contains a protocol, we currently only compare unmodified
> > paths.
> > 
> > However, some management software will change the backing filename to be
> > a relative filename in a path.  QEMU is able to handle this fine,
> > because internally it will use path_combine to put together the full
> > protocol URI.
> > 
> > However, this can lead to an inability to match an image during a QAPI
> > command that needs to use bdrv_find_backing_image() to find the image,
> > when it is searched by the full URI.
> > 
> > When searching for a protocol filename, if the straight comparison
> > fails, this patch will also compare against the full backing filename to
> > see if that is a match.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> 
> Thanks, applied to my block tree:
> 
> https://github.com/XanClic/qemu/commits/block
>

Thanks!

> 
> How much would you mind writing an iotest?

I don't mind, I can do that.

-Jeff

Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Jeff Cody]

On Wed, Jan 25, 2017 at 07:27:07PM +0100, Max Reitz wrote:
> On 25.01.2017 18:22, Jeff Cody wrote:
> > In bdrv_find_backing_image(), if we are searching an image for a backing
> > file that contains a protocol, we currently only compare unmodified
> > paths.
> > 
> > However, some management software will change the backing filename to be
> > a relative filename in a path.  QEMU is able to handle this fine,
> > because internally it will use path_combine to put together the full
> > protocol URI.
> > 
> > However, this can lead to an inability to match an image during a QAPI
> > command that needs to use bdrv_find_backing_image() to find the image,
> > when it is searched by the full URI.
> > 
> > When searching for a protocol filename, if the straight comparison
> > fails, this patch will also compare against the full backing filename to
> > see if that is a match.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 39ddea3..a173afc 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -3145,6 +3145,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
> >      int is_protocol = 0;
> >      BlockDriverState *curr_bs = NULL;
> >      BlockDriverState *retval = NULL;
> > +    Error *local_error = NULL;
> >  
> >      if (!bs || !bs->drv || !backing_file) {
> >          return NULL;
> > @@ -3165,6 +3166,17 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
> >                  retval = curr_bs->backing->bs;
> >                  break;
> >              }
> > +            /* Also check against the full backing filename for the image */
> > +            bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
> > +                                           &local_error);
> > +            if (local_error == NULL) {
> > +                if (strcmp(backing_file, backing_file_full) == 0) {
> > +                    retval = curr_bs->backing->bs;
> > +                    break;
> > +                }
> > +            } else {
> > +                error_free(local_error);
> 
> Oops, I was a bit too quick there. You should either follow Eric's
> remark about the scope, or you have to reset local_error to NULL here.
>

OK. I'll submit a v2, and add an iotest to that as well.

> 
> > +            }
> >          } else {
> >              /* If not an absolute filename path, make it relative to the current
> >               * image's filename path */
> > 
> 
>