[Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

Hi,

Patch 1 is the fix for the bug reported by Max here:

   https://lists.gnu.org/archive/html/qemu-block/2019-04/msg00293.html

Patch 2 fixes a different (but slightly related) issue that I found
while preparing the first patch.

Regards,

Berto

Alberto Garcia (2):
  block: Fix check for default backing files in bdrv_reopen_prepare()
  block: Clear the backing_file fields on detach

 block.c                | 6 +++++-
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

Hi,

Patch 1 is the fix for the bug reported by Max here:

   https://lists.gnu.org/archive/html/qemu-block/2019-04/msg00293.html

Patch 2 fixes a different (but slightly related) issue that I found
while preparing the first patch.

Regards,

Berto

Alberto Garcia (2):
  block: Fix check for default backing files in bdrv_reopen_prepare()
  block: Clear the backing_file fields on detach

 block.c                | 6 +++++-
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH for-4.1 1/2] block: Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

x-blockdev-reopen requires that the 'backing' parameter is specified
when an image has a backing file attached or when there is a default
backing file in the image metadata.

The latter can be checked by reading bs->auto_backing_file, but
bdrv_reopen_prepare() is using bs->backing_file.

This bug should be detected by iotest 245, but unfortunately the test
expectation is wrong so it must be corrected as well.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reported-by: Max Reitz <mreitz@redhat.com>
---
 block.c                | 3 ++-
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 16615bc876..e496999e2f 100644
--- a/block.c
+++ b/block.c
@@ -3634,7 +3634,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
      * its metadata. Otherwise the 'backing' option can be omitted.
      */
     if (drv->supports_backing && reopen_state->backing_missing &&
-        (backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) {
+        (backing_bs(reopen_state->bs) ||
+         reopen_state->bs->auto_backing_file[0])) {
         error_setg(errp, "backing is missing for '%s'",
                    reopen_state->bs->node_name);
         ret = -EINVAL;
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 7891a210c1..9784ca3ced 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -716,7 +716,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
         # Detach hd2 from hd0.
         self.reopen(opts, {'backing': None})
-        self.reopen(opts, {}, "backing is missing for 'hd0'")
+        self.reopen(opts)
 
         # Remove both hd0 and hd2
         result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
-- 
2.11.0

[Qemu-devel] [PATCH for-4.1 1/2] block: Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

x-blockdev-reopen requires that the 'backing' parameter is specified
when an image has a backing file attached or when there is a default
backing file in the image metadata.

The latter can be checked by reading bs->auto_backing_file, but
bdrv_reopen_prepare() is using bs->backing_file.

This bug should be detected by iotest 245, but unfortunately the test
expectation is wrong so it must be corrected as well.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reported-by: Max Reitz <mreitz@redhat.com>
---
 block.c                | 3 ++-
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 16615bc876..e496999e2f 100644
--- a/block.c
+++ b/block.c
@@ -3634,7 +3634,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
      * its metadata. Otherwise the 'backing' option can be omitted.
      */
     if (drv->supports_backing && reopen_state->backing_missing &&
-        (backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) {
+        (backing_bs(reopen_state->bs) ||
+         reopen_state->bs->auto_backing_file[0])) {
         error_setg(errp, "backing is missing for '%s'",
                    reopen_state->bs->node_name);
         ret = -EINVAL;
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 7891a210c1..9784ca3ced 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -716,7 +716,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
         # Detach hd2 from hd0.
         self.reopen(opts, {'backing': None})
-        self.reopen(opts, {}, "backing is missing for 'hd0'")
+        self.reopen(opts)
 
         # Remove both hd0 and hd2
         result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
-- 
2.11.0

[Qemu-devel] [PATCH for-4.1 2/2] block: Clear the backing_file fields on detach

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block.c b/block.c
index e496999e2f..e9c0c9e2ad 100644
--- a/block.c
+++ b/block.c
@@ -1085,6 +1085,9 @@ static void bdrv_backing_detach(BdrvChild *c)
     error_free(parent->backing_blocker);
     parent->backing_blocker = NULL;
 
+    parent->backing_file[0] = '\0';
+    parent->backing_format[0] = '\0';
+
     bdrv_child_cb_detach(c);
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH for-4.1 2/2] block: Clear the backing_file fields on detach

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block.c b/block.c
index e496999e2f..e9c0c9e2ad 100644
--- a/block.c
+++ b/block.c
@@ -1085,6 +1085,9 @@ static void bdrv_backing_detach(BdrvChild *c)
     error_free(parent->backing_blocker);
     parent->backing_blocker = NULL;
 
+    parent->backing_file[0] = '\0';
+    parent->backing_format[0] = '\0';
+
     bdrv_child_cb_detach(c);
 }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Max Reitz]

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

On 11.04.19 16:23, Alberto Garcia wrote:
> Hi,
> 
> Patch 1 is the fix for the bug reported by Max here:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2019-04/msg00293.html
> 
> Patch 2 fixes a different (but slightly related) issue that I found
> while preparing the first patch.

I think the real problem is that bs->backing_file is not a cache for
bs->backing->bs->filename.

In fact, every user of bs->backing_file expects it to be something
different.  Some expect it to be a cache for bs->backing->bs->filename,
some expect it to be what the image header says (i.e., it if’s a
relative path, it’s relative to the overlay), some expect it to be what
the image header says, but relative paths to be translated so they are
relative to qemu’s CWD.

All of this should be cleaned up and this is what patch 7 in my "block:
Deal with filters" series does:

http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Max Reitz]

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

On 11.04.19 16:23, Alberto Garcia wrote:
> Hi,
> 
> Patch 1 is the fix for the bug reported by Max here:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2019-04/msg00293.html
> 
> Patch 2 fixes a different (but slightly related) issue that I found
> while preparing the first patch.

I think the real problem is that bs->backing_file is not a cache for
bs->backing->bs->filename.

In fact, every user of bs->backing_file expects it to be something
different.  Some expect it to be a cache for bs->backing->bs->filename,
some expect it to be what the image header says (i.e., it if’s a
relative path, it’s relative to the overlay), some expect it to be what
the image header says, but relative paths to be translated so they are
relative to qemu’s CWD.

All of this should be cleaned up and this is what patch 7 in my "block:
Deal with filters" series does:

http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

On Sat 13 Apr 2019 02:56:57 AM CEST, Max Reitz wrote:
>> Patch 2 fixes a different (but slightly related) issue that I found
>> while preparing the first patch.
>
> I think the real problem is that bs->backing_file is not a cache for
> bs->backing->bs->filename.
>
> In fact, every user of bs->backing_file expects it to be something
> different.  Some expect it to be a cache for
> bs->backing->bs->filename, some expect it to be what the image header
> says (i.e., it if’s a relative path, it’s relative to the overlay),
> some expect it to be what the image header says, but relative paths to
> be translated so they are relative to qemu’s CWD.
>
> All of this should be cleaned up and this is what patch 7 in my "block:
> Deal with filters" series does:
>
> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

Ok, you can leave out the second patch then. The first one should still
be correct, right?

Berto

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

On Sat 13 Apr 2019 02:56:57 AM CEST, Max Reitz wrote:
>> Patch 2 fixes a different (but slightly related) issue that I found
>> while preparing the first patch.
>
> I think the real problem is that bs->backing_file is not a cache for
> bs->backing->bs->filename.
>
> In fact, every user of bs->backing_file expects it to be something
> different.  Some expect it to be a cache for
> bs->backing->bs->filename, some expect it to be what the image header
> says (i.e., it if’s a relative path, it’s relative to the overlay),
> some expect it to be what the image header says, but relative paths to
> be translated so they are relative to qemu’s CWD.
>
> All of this should be cleaned up and this is what patch 7 in my "block:
> Deal with filters" series does:
>
> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

Ok, you can leave out the second patch then. The first one should still
be correct, right?

Berto

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Max Reitz]

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

On 13.04.19 10:46, Alberto Garcia wrote:
> On Sat 13 Apr 2019 02:56:57 AM CEST, Max Reitz wrote:
>>> Patch 2 fixes a different (but slightly related) issue that I found
>>> while preparing the first patch.
>>
>> I think the real problem is that bs->backing_file is not a cache for
>> bs->backing->bs->filename.
>>
>> In fact, every user of bs->backing_file expects it to be something
>> different.  Some expect it to be a cache for
>> bs->backing->bs->filename, some expect it to be what the image header
>> says (i.e., it if’s a relative path, it’s relative to the overlay),
>> some expect it to be what the image header says, but relative paths to
>> be translated so they are relative to qemu’s CWD.
>>
>> All of this should be cleaned up and this is what patch 7 in my "block:
>> Deal with filters" series does:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html
> 
> Ok, you can leave out the second patch then. The first one should still
> be correct, right?

I just think it’s unnecessary because as of my series both backing_file
and auto_backing_file serve the purpose.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Max Reitz]

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

On 13.04.19 10:46, Alberto Garcia wrote:
> On Sat 13 Apr 2019 02:56:57 AM CEST, Max Reitz wrote:
>>> Patch 2 fixes a different (but slightly related) issue that I found
>>> while preparing the first patch.
>>
>> I think the real problem is that bs->backing_file is not a cache for
>> bs->backing->bs->filename.
>>
>> In fact, every user of bs->backing_file expects it to be something
>> different.  Some expect it to be a cache for
>> bs->backing->bs->filename, some expect it to be what the image header
>> says (i.e., it if’s a relative path, it’s relative to the overlay),
>> some expect it to be what the image header says, but relative paths to
>> be translated so they are relative to qemu’s CWD.
>>
>> All of this should be cleaned up and this is what patch 7 in my "block:
>> Deal with filters" series does:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html
> 
> Ok, you can leave out the second patch then. The first one should still
> be correct, right?

I just think it’s unnecessary because as of my series both backing_file
and auto_backing_file serve the purpose.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

On Sat 13 Apr 2019 05:48:11 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>> Ok, you can leave out the second patch then. The first one should
>> still be correct, right?
>
> I just think it’s unnecessary because as of my series both
> backing_file and auto_backing_file serve the purpose.

Ok then!

Berto

Re: [Qemu-devel] [PATCH for-4.1 0/2] Fix check for default backing files in bdrv_reopen_prepare()

[Alberto Garcia]

On Sat 13 Apr 2019 05:48:11 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>> Ok, you can leave out the second patch then. The first one should
>> still be correct, right?
>
> I just think it’s unnecessary because as of my series both
> backing_file and auto_backing_file serve the purpose.

Ok then!

Berto