Patchwork [v2,1/4] block: make bdrv_find_backing_image compare canonical filenames

login
register
mail settings
Submitter Jeff Cody
Date Oct. 16, 2012, 2:44 p.m.
Message ID <f1c0516355e0a60439760f2f39588e31092aa0a5.1350398238.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/191806/
State New
Headers show

Comments

Jeff Cody - Oct. 16, 2012, 2:44 p.m.
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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 6 deletions(-)
Eric Blake - Oct. 16, 2012, 3:12 p.m.
On 10/16/2012 08:44 AM, 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

Reads better as s/both not/not both/.

> 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 58 insertions(+), 6 deletions(-)

> -    if (!bs->drv) {
> +    char *filename_full = NULL;
> +    char *backing_file_full = NULL;
> +    char *filename_tmp = NULL;
> +    int is_protocol = 0;

Any reason you didn't use bool here?

> +    BlockDriverState *curr_bs = NULL;
> +    BlockDriverState *retval = NULL;
> +
> +    if (!bs || !bs->drv || !backing_file) {
>          return NULL;
>      }
>  
> -    if (bs->backing_hd) {
> -        if (strcmp(bs->backing_file, backing_file) == 0) {
> -            return bs->backing_hd;
> +    filename_full     = g_malloc(sizeof(char) * PATH_MAX);

sizeof(char) is guaranteed to be 1; this can be simplified to
g_malloc(PATH_MAX).

> +    backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
> +    filename_tmp      = g_malloc(sizeof(char) * PATH_MAX);
> +    if (!filename_full || !backing_file_full || !filename_tmp) {
> +        goto error;
> +    }

Dead 'if', since g_malloc() is guaranteed to succeed.

> +
> +    is_protocol = path_has_protocol(backing_file);
> +
> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
> +
> +        /* If either of the filename paths is actually a protocol, then
> +         * compare unmodified paths; otherwise make paths relative */
> +        if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
> +            if (strcmp(backing_file, curr_bs->backing_file) == 0) {

I guess we are guaranteed that if is_protocol and path_has_protocol()
give different answers, then the strcmp() will fail?
Jeff Cody - Oct. 16, 2012, 3:40 p.m.
On 10/16/2012 11:12 AM, Eric Blake wrote:
> On 10/16/2012 08:44 AM, 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
> 
> Reads better as s/both not/not both/.
> 
>> 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 58 insertions(+), 6 deletions(-)
> 
>> -    if (!bs->drv) {
>> +    char *filename_full = NULL;
>> +    char *backing_file_full = NULL;
>> +    char *filename_tmp = NULL;
>> +    int is_protocol = 0;
> 
> Any reason you didn't use bool here?
> 

path_has_protocol() returns an int, so I preferred to just match that.

>> +    BlockDriverState *curr_bs = NULL;
>> +    BlockDriverState *retval = NULL;
>> +
>> +    if (!bs || !bs->drv || !backing_file) {
>>          return NULL;
>>      }
>>  
>> -    if (bs->backing_hd) {
>> -        if (strcmp(bs->backing_file, backing_file) == 0) {
>> -            return bs->backing_hd;
>> +    filename_full     = g_malloc(sizeof(char) * PATH_MAX);
> 
> sizeof(char) is guaranteed to be 1; this can be simplified to
> g_malloc(PATH_MAX).
> 
>> +    backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
>> +    filename_tmp      = g_malloc(sizeof(char) * PATH_MAX);
>> +    if (!filename_full || !backing_file_full || !filename_tmp) {
>> +        goto error;
>> +    }
> 
> Dead 'if', since g_malloc() is guaranteed to succeed.
> 

I'll go ahead and simplify both of the above with the next spin.

>> +
>> +    is_protocol = path_has_protocol(backing_file);
>> +
>> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
>> +
>> +        /* If either of the filename paths is actually a protocol, then
>> +         * compare unmodified paths; otherwise make paths relative */
>> +        if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
>> +            if (strcmp(backing_file, curr_bs->backing_file) == 0) {
> 
> I guess we are guaranteed that if is_protocol and path_has_protocol()
> give different answers, then the strcmp() will fail?
> 

Yes, since is_protocol is determined via path_has_protocol() - if
they are different, then by definition strcmp() should fail.

Jeff

Patch

diff --git a/block.c b/block.c
index e95f613..165905a 100644
--- a/block.c
+++ b/block.c
@@ -3123,22 +3123,74 @@  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/* backing_file can either be relative, or absolute, or a protocol.  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 = NULL;
+    char *backing_file_full = NULL;
+    char *filename_tmp = NULL;
+    int is_protocol = 0;
+    BlockDriverState *curr_bs = NULL;
+    BlockDriverState *retval = NULL;
+
+    if (!bs || !bs->drv || !backing_file) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
+    filename_full     = g_malloc(sizeof(char) * PATH_MAX);
+    backing_file_full = g_malloc(sizeof(char) * PATH_MAX);
+    filename_tmp      = g_malloc(sizeof(char) * PATH_MAX);
+    if (!filename_full || !backing_file_full || !filename_tmp) {
+        goto error;
+    }
+
+    is_protocol = path_has_protocol(backing_file);
+
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+
+        /* If either of the filename paths is actually a protocol, then
+         * compare unmodified paths; otherwise make paths relative */
+        if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
+            if (strcmp(backing_file, curr_bs->backing_file) == 0) {
+                retval = curr_bs->backing_hd;
+                break;
+            }
         } else {
-            return bdrv_find_backing_image(bs->backing_hd, backing_file);
+            /* If not an absolute filename path, make it relative to the current
+             * image's filename path */
+            path_combine(filename_tmp, PATH_MAX, 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, PATH_MAX, curr_bs->filename,
+                         curr_bs->backing_file);
+
+            if (!realpath(filename_tmp, backing_file_full)) {
+                continue;
+            }
+
+            if (strcmp(backing_file_full, filename_full) == 0) {
+                retval = curr_bs->backing_hd;
+                break;
+            }
         }
     }
 
-    return NULL;
+error:
+    g_free(filename_full);
+    g_free(backing_file_full);
+    g_free(filename_tmp);
+    return retval;
 }
 
 int bdrv_get_backing_file_depth(BlockDriverState *bs)