| Message ID | 9a93d47ddba5f86d36cbdf884d86e2fa6f5542ae.1485364872.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 */ >
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
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 */ >> >
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 */ >
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
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 */ > > > >
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 */
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(+)