[Qemu-devel] [PATCH v2 0/4] block-commit fixes

[Qemu-devel] [PATCH v2 0/4] block-commit fixes

[Jeff Cody]

v1 -> v2 differences:

* Updated changes to be protocol friendly
* Updated block-commit tests to account for different error message
  wording
* Updated block-commit tests to add relative backing filename tests
* In bdrv_find_backing_image(), use g_malloc() instead of char arrays


Jeff Cody (4):
  block: make bdrv_find_backing_image compare canonical filenames
  block: in commit, determine base image from the top image
  qemu-iotests: qemu-io tests update for block-commit (040)
  qemu-iotests: add relative backing file tests for block-commit (040)

 block.c                    |  64 ++++++++++++++++++++++++---
 block/commit.c             |   9 ----
 blockdev.c                 |  21 ++++-----
 tests/qemu-iotests/040     | 106 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/040.out |   4 +-
 5 files changed, 175 insertions(+), 29 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH v2 1/4] 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 6 deletions(-)

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)
-- 
1.7.11.4

[Qemu-devel] [PATCH v2 2/4] 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

[Qemu-devel] [PATCH v2 3/4] qemu-iotests: qemu-io tests update for block-commit (040)

[Jeff Cody]

Some of the error messages put out by block-commit have changed
slightly, which causes 2 tests cases for block-commit to fail.
This patch updates the test cases to look for the correct error
output.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/040 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 258e7ea..0fa6441 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -111,7 +111,7 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_no_active_commit()
         result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % backing_img)
         self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', 'Invalid files for merge: top and base are the same')
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % backing_img)
 
     def test_top_invalid(self):
         self.assert_no_active_commit()
@@ -135,7 +135,7 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_no_active_commit()
         result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img)
         self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', 'Base (%(1)s) is not reachable from top (%(2)s)' % {"1" : mid_img, "2" : backing_img})
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
 
     def test_top_omitted(self):
         self.assert_no_active_commit()
-- 
1.7.11.4

[Qemu-devel] [PATCH v2 4/4] qemu-iotests: add relative backing file tests for block-commit (040)

[Jeff Cody]

The previous block commit used absolute filenames for all block-commit
images and commands; this adds relative filenames for the same tests.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/040     | 102 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |   4 +-
 2 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 0fa6441..aad535a 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -26,6 +26,7 @@ import os
 import iotests
 from iotests import qemu_img, qemu_io
 import struct
+import errno
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -143,6 +144,107 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing")
 
+class TestRelativePaths(ImageCommitTestCase):
+    image_len = 1 * 1024 * 1024
+    test_len = 1 * 1024 * 256
+
+    dir1 = "dir1"
+    dir2 = "dir2/"
+    dir3 = "dir2/dir3/"
+
+    test_img = os.path.join(iotests.test_dir, dir3, 'test.img')
+    mid_img = "../mid.img"
+    backing_img = "../dir1/backing.img"
+
+    backing_img_abs = os.path.join(iotests.test_dir, dir1, 'backing.img')
+    mid_img_abs = os.path.join(iotests.test_dir, dir2, 'mid.img')
+
+    def setUp(self):
+        try:
+            os.mkdir(os.path.join(iotests.test_dir, self.dir1))
+            os.mkdir(os.path.join(iotests.test_dir, self.dir2))
+            os.mkdir(os.path.join(iotests.test_dir, self.dir3))
+        except OSError as exception:
+            if exception.errno != errno.EEXIST:
+                raise
+        self.create_image(self.backing_img_abs, TestRelativePaths.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.backing_img_abs, self.mid_img_abs)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.mid_img_abs, self.test_img)
+        qemu_img('rebase', '-u', '-b', self.backing_img, self.mid_img_abs)
+        qemu_img('rebase', '-u', '-b', self.mid_img, self.test_img)
+        qemu_io('-c', 'write -P 0xab 0 524288', self.backing_img_abs)
+        qemu_io('-c', 'write -P 0xef 524288 524288', self.mid_img_abs)
+        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(self.test_img)
+        os.remove(self.mid_img_abs)
+        os.remove(self.backing_img_abs)
+        try:
+            os.rmdir(os.path.join(iotests.test_dir, self.dir1))
+            os.rmdir(os.path.join(iotests.test_dir, self.dir3))
+            os.rmdir(os.path.join(iotests.test_dir, self.dir2))
+        except OSError as exception:
+            if exception.errno != errno.EEXIST and exception.errno != errno.ENOTEMPTY:
+                raise
+
+    def test_commit(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img)
+        self.assert_qmp(result, 'return', {})
+
+        completed = False
+        while not completed:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp(event, 'data/type', 'commit')
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp(event, 'data/offset', self.image_len)
+                    self.assert_qmp(event, 'data/len', self.image_len)
+                    completed = True
+
+        self.assert_no_active_commit()
+        self.vm.shutdown()
+
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
+
+    def test_device_not_found(self):
+        result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % self.mid_img)
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_top_same_base(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img, base='%s' % self.mid_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % self.mid_img)
+
+    def test_top_invalid(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='badfile', base='%s' % self.backing_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Top image file badfile not found')
+
+    def test_base_invalid(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img, base='badfile')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
+
+    def test_top_is_active(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.test_img, base='%s' % self.backing_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Top image as the active layer is currently unsupported')
+
+    def test_top_and_base_reversed(self):
+        self.assert_no_active_commit()
+        result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.backing_img, base='%s' % self.mid_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % self.mid_img)
+
 
 class TestSetSpeed(ImageCommitTestCase):
     image_len = 80 * 1024 * 1024 # MB
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index dae404e..b6f2576 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-.........
+................
 ----------------------------------------------------------------------
-Ran 9 tests
+Ran 16 tests
 
 OK
-- 
1.7.11.4

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

[Eric Blake]

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

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?

-- 
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 v2 2/4] block: in commit, determine base image from the top image

[Eric Blake]

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

On 10/16/2012 08:44 AM, 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(-)

As long as you are touching this code:

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

please swap this to 'has_base && base' to avoid any potential of
valgrind warnings about conditional jump based on uninitialized memory.

Also, I raised another bug[1] about a bad error message regarding
top_bs, if the user passes a different spelling than the canonical name
of the active image.  Is that worth fixing in this series, or is it okay
to leave it until you actually add support for committing the top image?

[1] https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02067.html
-- 
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 v2 2/4] block: in commit, determine base image from the top image

[Jeff Cody]

On 10/16/2012 11:22 AM, Eric Blake wrote:
> On 10/16/2012 08:44 AM, 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(-)
> 
> As long as you are touching this code:
> 
>> @@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
>>          return;
>>      }
>>  
>> +    if (base && has_base) {
> 
> please swap this to 'has_base && base' to avoid any potential of
> valgrind warnings about conditional jump based on uninitialized memory.
>

OK.

> Also, I raised another bug[1] about a bad error message regarding
> top_bs, if the user passes a different spelling than the canonical name
> of the active image.  Is that worth fixing in this series, or is it okay
> to leave it until you actually add support for committing the top image?
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02067.html
> 

Since this doesn't pose an actual issue now, I was planning on just
addressing that with the stage-2 patches, since that will likely touch
other related areas of the code anyway.  If you have major heartburn
over this, I'll change it now.

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

[Eric Blake]

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

On 10/16/2012 09:31 AM, Jeff Cody wrote:
>> Also, I raised another bug[1] about a bad error message regarding
>> top_bs, if the user passes a different spelling than the canonical name
>> of the active image.  Is that worth fixing in this series, or is it okay
>> to leave it until you actually add support for committing the top image?
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02067.html
>>
> 
> Since this doesn't pose an actual issue now, I was planning on just
> addressing that with the stage-2 patches, since that will likely touch
> other related areas of the code anyway.  If you have major heartburn
> over this, I'll change it now.

Nah, waiting is fine.  As I said in the original bug report, it is only
a poor quality error message at the moment, and fixing it may be easier
once you have additional pieces in place for committing the active
image.  I just wanted to make sure the report wasn't lost.

-- 
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 v2 1/4] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody]

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