* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 17:22 [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames Jeff Cody
@ 2017-01-25 17:43 ` Jeff Cody
2017-01-25 18:24 ` Eric Blake
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Jeff Cody @ 2017-01-25 17:43 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mreitz, qemu-block
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 17:22 [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames Jeff Cody
2017-01-25 17:43 ` Jeff Cody
@ 2017-01-25 18:24 ` Eric Blake
2017-01-25 18:25 ` Max Reitz
2017-01-25 18:24 ` Max Reitz
2017-01-25 18:27 ` Max Reitz
3 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2017-01-25 18:24 UTC (permalink / raw)
To: Jeff Cody, qemu-devel; +Cc: kwolf, mreitz, qemu block
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 18:24 ` Eric Blake
@ 2017-01-25 18:25 ` Max Reitz
0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-01-25 18:25 UTC (permalink / raw)
To: Eric Blake, Jeff Cody, qemu-devel; +Cc: kwolf, qemu block
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 17:22 [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames Jeff Cody
2017-01-25 17:43 ` Jeff Cody
2017-01-25 18:24 ` Eric Blake
@ 2017-01-25 18:24 ` Max Reitz
2017-01-25 18:44 ` Jeff Cody
2017-01-25 18:27 ` Max Reitz
3 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2017-01-25 18:24 UTC (permalink / raw)
To: Jeff Cody, qemu-devel; +Cc: kwolf
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 18:24 ` Max Reitz
@ 2017-01-25 18:44 ` Jeff Cody
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Cody @ 2017-01-25 18:44 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, kwolf
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 17:22 [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames Jeff Cody
` (2 preceding siblings ...)
2017-01-25 18:24 ` Max Reitz
@ 2017-01-25 18:27 ` Max Reitz
2017-01-25 18:48 ` Jeff Cody
3 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2017-01-25 18:27 UTC (permalink / raw)
To: Jeff Cody, qemu-devel; +Cc: kwolf
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames
2017-01-25 18:27 ` Max Reitz
@ 2017-01-25 18:48 ` Jeff Cody
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Cody @ 2017-01-25 18:48 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, kwolf
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 */
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread