[Qemu-devel] [PATCH 0/2] block-commit fixes

[Qemu-devel] [PATCH 0/2] block-commit fixes

[Jeff Cody]

Eric reported some bugs with relative pathnames and block-commit.

These patches attempt to fix those issues, by changing how we do
some filename and path comparisons.  There is also some minor
simplication of error checking for block-commit as well.


Jeff Cody (2):
  block: make bdrv_find_backing_image compare canonical filenames
  block: in commit, determine base image from the top image

 block.c        | 38 ++++++++++++++++++++++++++++++++------
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 3 files changed, 43 insertions(+), 25 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody]

Currently, bdrv_find_backing_image compares bs->backing_file with
what is passed in as a backing_file name.  Mismatches may occur,
however, when bs->backing_file and backing_file are both not
absolute or relative.

Use path_combine() to make sure any relative backing filenames are
relative to the current image filename being searched, and then use
realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive,
but iterative.

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

diff --git a/block.c b/block.c
index e95f613..641b8fa 100644
--- a/block.c
+++ b/block.c
@@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/* backing_file can either be relative, or absolute.  If it is
+ * relative, it must be relative to the chain.  So, passing in
+ * bs->filename from a BDS as backing_file should not be done,
+ * as that may be relative to the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         const char *backing_file)
 {
-    if (!bs->drv) {
+    char filename_full[PATH_MAX];
+    char backing_file_full[PATH_MAX];
+    char filename_tmp[PATH_MAX];
+    BlockDriverState *curr_bs = NULL;
+
+    if (!bs || !bs->drv) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
-        } else {
-            return bdrv_find_backing_image(bs->backing_hd, backing_file);
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+        /* If not an absolute filename path, make it relative to the current
+         * image's filename path */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, backing_file);
+
+        /* We are going to compare absolute pathnames */
+        if (!realpath(filename_tmp, filename_full)) {
+            continue;
+        }
+
+        /* We need to make sure the backing filename we are comparing against
+         * is relative to the current image filename (or absolute) */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, curr_bs->backing_file);
+
+        if (!realpath(filename_tmp, backing_file_full)) {
+            continue;
+        }
+
+        if (strcmp(backing_file_full, filename_full) == 0) {
+            return curr_bs->backing_hd;
         }
     }
 
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Paolo Bonzini]

Il 10/10/2012 07:56, Jeff Cody ha scritto:
> Currently, bdrv_find_backing_image compares bs->backing_file with
> what is passed in as a backing_file name.  Mismatches may occur,
> however, when bs->backing_file and backing_file are both not
> absolute or relative.
> 
> Use path_combine() to make sure any relative backing filenames are
> relative to the current image filename being searched, and then use
> realpath() to make all comparisons based on absolute filenames.
> 
> This also changes bdrv_find_backing_image to no longer be recursive,
> but iterative.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e95f613..641b8fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> +/* backing_file can either be relative, or absolute.  If it is
> + * relative, it must be relative to the chain.  So, passing in
> + * bs->filename from a BDS as backing_file should not be done,
> + * as that may be relative to the CWD rather than the chain. */
>  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>          const char *backing_file)
>  {
> -    if (!bs->drv) {
> +    char filename_full[PATH_MAX];
> +    char backing_file_full[PATH_MAX];
> +    char filename_tmp[PATH_MAX];
> +    BlockDriverState *curr_bs = NULL;
> +
> +    if (!bs || !bs->drv) {
>          return NULL;
>      }
>  
> -    if (bs->backing_hd) {
> -        if (strcmp(bs->backing_file, backing_file) == 0) {
> -            return bs->backing_hd;
> -        } else {
> -            return bdrv_find_backing_image(bs->backing_hd, backing_file);
> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
> +        /* If not an absolute filename path, make it relative to the current
> +         * image's filename path */
> +        path_combine(filename_tmp, sizeof(filename_tmp),
> +                     curr_bs->filename, backing_file);
> +
> +        /* We are going to compare absolute pathnames */
> +        if (!realpath(filename_tmp, filename_full)) {
> +            continue;
> +        }
> +
> +        /* We need to make sure the backing filename we are comparing against
> +         * is relative to the current image filename (or absolute) */
> +        path_combine(filename_tmp, sizeof(filename_tmp),
> +                     curr_bs->filename, curr_bs->backing_file);
> +
> +        if (!realpath(filename_tmp, backing_file_full)) {

Unfortunately realpath does not exist on Win32.  But
http://lists.gnu.org/archive/html/grub-devel/2011-09/msg00034.html
suggests that there is a _fullpath and in fact already has all you need
to write a qemu_realpath function.

Paolo

> +            continue;
> +        }
> +
> +        if (strcmp(backing_file_full, filename_full) == 0) {
> +            return curr_bs->backing_hd;
>          }
>      }
>  
> 

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody]

On 10/10/2012 03:34 AM, Paolo Bonzini wrote:
> Il 10/10/2012 07:56, Jeff Cody ha scritto:
>> Currently, bdrv_find_backing_image compares bs->backing_file with
>> what is passed in as a backing_file name.  Mismatches may occur,
>> however, when bs->backing_file and backing_file are both not
>> absolute or relative.
>>
>> Use path_combine() to make sure any relative backing filenames are
>> relative to the current image filename being searched, and then use
>> realpath() to make all comparisons based on absolute filenames.
>>
>> This also changes bdrv_find_backing_image to no longer be recursive,
>> but iterative.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e95f613..641b8fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>      return -ENOTSUP;
>>  }
>>  
>> +/* backing_file can either be relative, or absolute.  If it is
>> + * relative, it must be relative to the chain.  So, passing in
>> + * bs->filename from a BDS as backing_file should not be done,
>> + * as that may be relative to the CWD rather than the chain. */
>>  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>          const char *backing_file)
>>  {
>> -    if (!bs->drv) {
>> +    char filename_full[PATH_MAX];
>> +    char backing_file_full[PATH_MAX];
>> +    char filename_tmp[PATH_MAX];
>> +    BlockDriverState *curr_bs = NULL;
>> +
>> +    if (!bs || !bs->drv) {
>>          return NULL;
>>      }
>>  
>> -    if (bs->backing_hd) {
>> -        if (strcmp(bs->backing_file, backing_file) == 0) {
>> -            return bs->backing_hd;
>> -        } else {
>> -            return bdrv_find_backing_image(bs->backing_hd, backing_file);
>> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
>> +        /* If not an absolute filename path, make it relative to the current
>> +         * image's filename path */
>> +        path_combine(filename_tmp, sizeof(filename_tmp),
>> +                     curr_bs->filename, backing_file);
>> +
>> +        /* We are going to compare absolute pathnames */
>> +        if (!realpath(filename_tmp, filename_full)) {
>> +            continue;
>> +        }
>> +
>> +        /* We need to make sure the backing filename we are comparing against
>> +         * is relative to the current image filename (or absolute) */
>> +        path_combine(filename_tmp, sizeof(filename_tmp),
>> +                     curr_bs->filename, curr_bs->backing_file);
>> +
>> +        if (!realpath(filename_tmp, backing_file_full)) {
> 
> Unfortunately realpath does not exist on Win32.  But
> http://lists.gnu.org/archive/html/grub-devel/2011-09/msg00034.html
> suggests that there is a _fullpath and in fact already has all you need
> to write a qemu_realpath function.
> 
> Paolo
> 

Doesn't block.c already rely on realpath(), in bdrv_open()?  Why would
this be any different using it here?

>> +            continue;
>> +        }
>> +
>> +        if (strcmp(backing_file_full, filename_full) == 0) {
>> +            return curr_bs->backing_hd;
>>          }
>>      }
>>  
>>
> 

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Paolo Bonzini]

Il 10/10/2012 09:59, Jeff Cody ha scritto:
>> Unfortunately realpath does not exist on Win32.  But
>> http://lists.gnu.org/archive/html/grub-devel/2011-09/msg00034.html
>> suggests that there is a _fullpath and in fact already has all you need
>> to write a qemu_realpath function.
> 
> Doesn't block.c already rely on realpath(), in bdrv_open()?  Why would
> this be any different using it here?

Uh, indeed there's already a _fullpath wrapper in qemu-common.h.  My
bad, I only grepped os*.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Eric Blake]

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

On 10/09/2012 11:56 PM, Jeff Cody wrote:
> Currently, bdrv_find_backing_image compares bs->backing_file with
> what is passed in as a backing_file name.  Mismatches may occur,
> however, when bs->backing_file and backing_file are both not
> absolute or relative.
> 
> Use path_combine() to make sure any relative backing filenames are
> relative to the current image filename being searched, and then use
> realpath() to make all comparisons based on absolute filenames.
> 
> This also changes bdrv_find_backing_image to no longer be recursive,
> but iterative.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e95f613..641b8fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> +/* backing_file can either be relative, or absolute.  If it is
> + * relative, it must be relative to the chain.  So, passing in
> + * bs->filename from a BDS as backing_file should not be done,
> + * as that may be relative to the CWD rather than the chain. */
>  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>          const char *backing_file)
>  {
> -    if (!bs->drv) {
> +    char filename_full[PATH_MAX];
> +    char backing_file_full[PATH_MAX];
> +    char filename_tmp[PATH_MAX];

That's a LOT of stack space, which risks stack overflow, will mostly be
unused, and still doesn't work if you have super-deep hierarchies larger
than PATH_MAX.  Would you be better off using realpath(,NULL) for its
allocating semantics, and then free()ing the results?

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody]

On 10/10/2012 02:13 PM, Eric Blake wrote:
> On 10/09/2012 11:56 PM, Jeff Cody wrote:
>> Currently, bdrv_find_backing_image compares bs->backing_file with
>> what is passed in as a backing_file name.  Mismatches may occur,
>> however, when bs->backing_file and backing_file are both not
>> absolute or relative.
>>
>> Use path_combine() to make sure any relative backing filenames are
>> relative to the current image filename being searched, and then use
>> realpath() to make all comparisons based on absolute filenames.
>>
>> This also changes bdrv_find_backing_image to no longer be recursive,
>> but iterative.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e95f613..641b8fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>      return -ENOTSUP;
>>  }
>>  
>> +/* backing_file can either be relative, or absolute.  If it is
>> + * relative, it must be relative to the chain.  So, passing in
>> + * bs->filename from a BDS as backing_file should not be done,
>> + * as that may be relative to the CWD rather than the chain. */
>>  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>          const char *backing_file)
>>  {
>> -    if (!bs->drv) {
>> +    char filename_full[PATH_MAX];
>> +    char backing_file_full[PATH_MAX];
>> +    char filename_tmp[PATH_MAX];
> 
> That's a LOT of stack space, which risks stack overflow, will mostly be
> unused, and still doesn't work if you have super-deep hierarchies larger
> than PATH_MAX.  Would you be better off using realpath(,NULL) for its
> allocating semantics, and then free()ing the results?
> 

That is the main reason I changed it from being a recursive function, to
an iterative one.

Do we know that realpath(,NULL) behaves the same on all platforms?

We had a thread back in April that touched on the use of realpath, and
concerns were raised then that realpath(,NULL) was not necessarily safe
across all OSes:

https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01417.html

That said, if there is concern over the stack usage, to be safe I can
manually g_malloc() each array.

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Eric Blake]

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

On 10/10/2012 12:29 PM, Jeff Cody wrote:

>> That's a LOT of stack space, which risks stack overflow, will mostly be
>> unused, and still doesn't work if you have super-deep hierarchies larger
>> than PATH_MAX.  Would you be better off using realpath(,NULL) for its
>> allocating semantics, and then free()ing the results?
>>
> 
> That is the main reason I changed it from being a recursive function, to
> an iterative one.
> 
> Do we know that realpath(,NULL) behaves the same on all platforms?

Gnulib lists the following platforms as mis-handling NULL:
Mac OS X 10.5, FreeBSD 6.4, OpenBSD 4.4, Solaris 10.

> 
> We had a thread back in April that touched on the use of realpath, and
> concerns were raised then that realpath(,NULL) was not necessarily safe
> across all OSes:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01417.html

In fact, that message points out an even more insidious portability bug
in your algorithm: on Solaris 10, realpath("relative", buffer) leaves
buffer containing "relative" rather than an absolute name, but your
algorithm depends on matching absolute names.  I don't know if we port
qemu to Solaris 10, but it's worth considering my question back in that
thread - does glib provide us a more portable function for converting a
relative name into a canonical path that is guaranteed to work everywhere?

> 
> That said, if there is concern over the stack usage, to be safe I can
> manually g_malloc() each array.

g_malloc() would solve the stack size concern, but not the Solaris 10
relative name bug.

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody]

On 10/10/2012 02:34 PM, Eric Blake wrote:
> On 10/10/2012 12:29 PM, Jeff Cody wrote:
> 
>>> That's a LOT of stack space, which risks stack overflow, will mostly be
>>> unused, and still doesn't work if you have super-deep hierarchies larger
>>> than PATH_MAX.  Would you be better off using realpath(,NULL) for its
>>> allocating semantics, and then free()ing the results?
>>>
>>
>> That is the main reason I changed it from being a recursive function, to
>> an iterative one.
>>
>> Do we know that realpath(,NULL) behaves the same on all platforms?
> 
> Gnulib lists the following platforms as mis-handling NULL:
> Mac OS X 10.5, FreeBSD 6.4, OpenBSD 4.4, Solaris 10.
>
>>
>> We had a thread back in April that touched on the use of realpath, and
>> concerns were raised then that realpath(,NULL) was not necessarily safe
>> across all OSes:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01417.html
> 
> In fact, that message points out an even more insidious portability bug
> in your algorithm: on Solaris 10, realpath("relative", buffer) leaves
> buffer containing "relative" rather than an absolute name, but your
> algorithm depends on matching absolute names.  I don't know if we port
> qemu to Solaris 10, but it's worth considering my question back in that
> thread - does glib provide us a more portable function for converting a
> relative name into a canonical path that is guaranteed to work everywhere?
> 

We are already relying on realpath() in block.c.  So rather than making
this patch series much more complicated for a minor fix, I think that if
there are concerns about realpath() on other OSes such as Solaris 10,
then those concerns should be addressed in a separate patch series.

>>
>> That said, if there is concern over the stack usage, to be safe I can
>> manually g_malloc() each array.
> 
> g_malloc() would solve the stack size concern, but not the Solaris 10
> relative name bug.
> 

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Eric Blake]

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

On 10/10/2012 12:57 PM, Jeff Cody wrote:
> 
> We are already relying on realpath() in block.c.  So rather than making
> this patch series much more complicated for a minor fix, I think that if
> there are concerns about realpath() on other OSes such as Solaris 10,
> then those concerns should be addressed in a separate patch series.

I can agree to that.  Therefore,

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

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Eric Blake]

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

On 10/09/2012 11:56 PM, Jeff Cody wrote:
> Currently, bdrv_find_backing_image compares bs->backing_file with
> what is passed in as a backing_file name.  Mismatches may occur,
> however, when bs->backing_file and backing_file are both not
> absolute or relative.
> 
> Use path_combine() to make sure any relative backing filenames are
> relative to the current image filename being searched, and then use
> realpath() to make all comparisons based on absolute filenames.
> 
> This also changes bdrv_find_backing_image to no longer be recursive,
> but iterative.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 

> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
> +        /* If not an absolute filename path, make it relative to the current
> +         * image's filename path */
> +        path_combine(filename_tmp, sizeof(filename_tmp),
> +                     curr_bs->filename, backing_file);

I just realized that it is possible to set up a qcow2 file that wraps a
network protocol as its backing source, such as 'nbd:...'.  In this
case, what does path_combine() do to that user string?

> +
> +        /* We are going to compare absolute pathnames */
> +        if (!realpath(filename_tmp, filename_full)) {
> +            continue;
> +        }

and realpath() certainly won't like it (most likely, it won't exist in
the file system, but on the off chance that it does, that file is much
different than the real protocol that we are using as the backing source).

I'm afraid you may need a followup patch that handles the case of a
non-file backing protocol, and insist on an exact match in that case
without trying any normalization.

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody]

On 10/12/2012 05:52 PM, Eric Blake wrote:
> On 10/09/2012 11:56 PM, Jeff Cody wrote:
>> Currently, bdrv_find_backing_image compares bs->backing_file with
>> what is passed in as a backing_file name.  Mismatches may occur,
>> however, when bs->backing_file and backing_file are both not
>> absolute or relative.
>>
>> Use path_combine() to make sure any relative backing filenames are
>> relative to the current image filename being searched, and then use
>> realpath() to make all comparisons based on absolute filenames.
>>
>> This also changes bdrv_find_backing_image to no longer be recursive,
>> but iterative.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
> 
>> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
>> +        /* If not an absolute filename path, make it relative to the current
>> +         * image's filename path */
>> +        path_combine(filename_tmp, sizeof(filename_tmp),
>> +                     curr_bs->filename, backing_file);
> 
> I just realized that it is possible to set up a qcow2 file that wraps a
> network protocol as its backing source, such as 'nbd:...'.  In this
> case, what does path_combine() do to that user string?
> 
>> +
>> +        /* We are going to compare absolute pathnames */
>> +        if (!realpath(filename_tmp, filename_full)) {
>> +            continue;
>> +        }
> 
> and realpath() certainly won't like it (most likely, it won't exist in
> the file system, but on the off chance that it does, that file is much
> different than the real protocol that we are using as the backing source).
> 
> I'm afraid you may need a followup patch that handles the case of a
> non-file backing protocol, and insist on an exact match in that case
> without trying any normalization.
> 

Yes, it will need a follow-up patch.  Check for protocol
(path_has_protocol()), and if true just compare unmodified
backing_file names, other wise use path_combine + realpath().

Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Kevin Wolf]

Am 13.10.2012 17:25, schrieb Jeff Cody:
> On 10/12/2012 05:52 PM, Eric Blake wrote:
>> On 10/09/2012 11:56 PM, Jeff Cody wrote:
>>> Currently, bdrv_find_backing_image compares bs->backing_file with
>>> what is passed in as a backing_file name.  Mismatches may occur,
>>> however, when bs->backing_file and backing_file are both not
>>> absolute or relative.
>>>
>>> Use path_combine() to make sure any relative backing filenames are
>>> relative to the current image filename being searched, and then use
>>> realpath() to make all comparisons based on absolute filenames.
>>>
>>> This also changes bdrv_find_backing_image to no longer be recursive,
>>> but iterative.
>>>
>>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>> ---
>>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>>
>>
>>> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
>>> +        /* If not an absolute filename path, make it relative to the current
>>> +         * image's filename path */
>>> +        path_combine(filename_tmp, sizeof(filename_tmp),
>>> +                     curr_bs->filename, backing_file);
>>
>> I just realized that it is possible to set up a qcow2 file that wraps a
>> network protocol as its backing source, such as 'nbd:...'.  In this
>> case, what does path_combine() do to that user string?
>>
>>> +
>>> +        /* We are going to compare absolute pathnames */
>>> +        if (!realpath(filename_tmp, filename_full)) {
>>> +            continue;
>>> +        }
>>
>> and realpath() certainly won't like it (most likely, it won't exist in
>> the file system, but on the off chance that it does, that file is much
>> different than the real protocol that we are using as the backing source).
>>
>> I'm afraid you may need a followup patch that handles the case of a
>> non-file backing protocol, and insist on an exact match in that case
>> without trying any normalization.
>>
> 
> Yes, it will need a follow-up patch.  Check for protocol
> (path_has_protocol()), and if true just compare unmodified
> backing_file names, other wise use path_combine + realpath().

These cases would regress, so it doesn't just need a follow-up patch,
but a respin of this series.

Kevin

[Qemu-devel] [PATCH 2/2] block: in commit, determine base image from the top image

[Jeff Cody]

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames,
or filenames relative to the chain.  In the QMP message handler for
block commit, when looking up the base do so from the determined top
image, so we know it is reachable from top.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 733c914..13d9e82 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -211,15 +211,6 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    /* top and base may be valid, but let's make sure that base is reachable
-     * from top */
-    if (bdrv_find_backing_image(top, base->filename) != base) {
-        error_setg(errp,
-                   "Base (%s) is not reachable from top (%s)",
-                   base->filename, top->filename);
-        return;
-    }
-
     overlay_bs = bdrv_find_overlay(bs, top);
 
     if (overlay_bs == NULL) {
diff --git a/blockdev.c b/blockdev.c
index 99828ad..7052287 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1157,16 +1157,6 @@ void qmp_block_commit(const char *device,
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
-    if (base && has_base) {
-        base_bs = bdrv_find_backing_image(bs, base);
-    } else {
-        base_bs = bdrv_find_base(bs);
-    }
-
-    if (base_bs == NULL) {
-        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
-        return;
-    }
 
     /* default top_bs is the active layer */
     top_bs = bs;
@@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (base && has_base) {
+        base_bs = bdrv_find_backing_image(top_bs, base);
+    } else {
+        base_bs = bdrv_find_base(top_bs);
+    }
+
+    if (base_bs == NULL) {
+        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
+        return;
+    }
+
     commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
                 &local_err);
     if (local_err != NULL) {
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 2/2] block: in commit, determine base image from the top image

[Paolo Bonzini]

Il 10/10/2012 07:56, Jeff Cody ha scritto:
> This simplifies some code and error checking, and also fixes a bug.
> 
> bdrv_find_backing_image() should only be passed absolute filenames,
> or filenames relative to the chain.  In the QMP message handler for
> block commit, when looking up the base do so from the determined top
> image, so we know it is reachable from top.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/commit.c |  9 ---------
>  blockdev.c     | 21 +++++++++++----------
>  2 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 733c914..13d9e82 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -211,15 +211,6 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>          return;
>      }
>  
> -    /* top and base may be valid, but let's make sure that base is reachable
> -     * from top */
> -    if (bdrv_find_backing_image(top, base->filename) != base) {
> -        error_setg(errp,
> -                   "Base (%s) is not reachable from top (%s)",
> -                   base->filename, top->filename);
> -        return;
> -    }
> -
>      overlay_bs = bdrv_find_overlay(bs, top);
>  
>      if (overlay_bs == NULL) {
> diff --git a/blockdev.c b/blockdev.c
> index 99828ad..7052287 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1157,16 +1157,6 @@ void qmp_block_commit(const char *device,
>          error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>          return;
>      }
> -    if (base && has_base) {
> -        base_bs = bdrv_find_backing_image(bs, base);
> -    } else {
> -        base_bs = bdrv_find_base(bs);
> -    }
> -
> -    if (base_bs == NULL) {
> -        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
> -        return;
> -    }
>  
>      /* default top_bs is the active layer */
>      top_bs = bs;
> @@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    if (base && has_base) {
> +        base_bs = bdrv_find_backing_image(top_bs, base);
> +    } else {
> +        base_bs = bdrv_find_base(top_bs);
> +    }
> +
> +    if (base_bs == NULL) {
> +        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
> +        return;
> +    }
> +
>      commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
>                  &local_err);
>      if (local_err != NULL) {
> 

This one looks good,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 2/2] block: in commit, determine base image from the top image

[Eric Blake]

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

On 10/09/2012 11:56 PM, Jeff Cody wrote:
> This simplifies some code and error checking, and also fixes a bug.
> 
> bdrv_find_backing_image() should only be passed absolute filenames,
> or filenames relative to the chain.  In the QMP message handler for
> block commit, when looking up the base do so from the determined top
> image, so we know it is reachable from top.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/commit.c |  9 ---------
>  blockdev.c     | 21 +++++++++++----------
>  2 files changed, 11 insertions(+), 19 deletions(-)
> 

> @@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    if (base && has_base) {
> +        base_bs = bdrv_find_backing_image(top_bs, base);

In particular, if I have the chain:

/dir1/base <- /dir1/snap1(base) <- /dir2/base(/dir1/snap1) <-
/dir2/snap2(base)

and request a commit of "top":"/dir1/snap1", "base":"base", then this
new code will now look up base relative to /dir1/snap1 and find
/dir1/base and proceed with the commit; whereas the old code would look
up base relative to /dir2/snap2 and find /dir2/base and complain that
the arguments were swapped.

I can live with those semantics.

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

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 2/2] block: in commit, determine base image from the top image

[Eric Blake]

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

On 10/09/2012 11:56 PM, Jeff Cody wrote:
> This simplifies some code and error checking, and also fixes a bug.
> 
> bdrv_find_backing_image() should only be passed absolute filenames,
> or filenames relative to the chain.  In the QMP message handler for
> block commit, when looking up the base do so from the determined top
> image, so we know it is reachable from top.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---

> +++ b/blockdev.c

> @@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    if (base && has_base) {

Code motion, but won't this order trigger valgrind warnings about
jumping based on a read of uninitialized memory?  That is, the canonical
ordering is 'has_base && base' (although the end result is the same,
with the difference only observable in valgrind).

-- 
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: 617 bytes --]