[PATCH v2 0/2] block/export: Conditionally ignore set-context error

[PATCH v2 0/2] block/export: Conditionally ignore set-context error

[Max Reitz]

Hi,

I had this v2 lying around for quite some time but for some reason never
sent it.  I probably just forgot.  Sorry.

v1:
https://lists.nongnu.org/archive/html/qemu-block/2021-04/msg00584.html

Changes in v2:
- Letting `bdrv_try_set_aio_context()` return an error and then just
  discarding it conditionally is kind of not right.
  If we want to ignore the error, decide so from the start: Depending on
  `fixed_iothread`, pass either `errp` or `NULL`.

- Patch 2: Reference output has changed because of 30ebb9aa920.


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0009] [FC] 'block/export: Conditionally ignore set-context error'
002/2:[0002] [FC] 'iotests/307: Test iothread conflict for exports'


Max Reitz (2):
  block/export: Conditionally ignore set-context error
  iotests/307: Test iothread conflict for exports

 block/export/export.c      |  5 ++++-
 tests/qemu-iotests/307     | 15 +++++++++++++++
 tests/qemu-iotests/307.out |  8 ++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH v2 1/2] block/export: Conditionally ignore set-context error

[Max Reitz]

When invoking block-export-add with some iothread and
fixed-iothread=false, and changing the node's iothread fails, the error
is supposed to be ignored.

However, it is still stored in *errp, which is wrong.  If a second error
occurs, the "*errp must be NULL" assertion in error_setv() fails:

  qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion
  `*errp == NULL' failed.

So if fixed-iothread=false, we should ignore the error by passing NULL
to bdrv_try_set_aio_context().

Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef
       ("block/export: add iothread and fixed-iothread options")
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/export/export.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/export/export.c b/block/export/export.c
index fec7d9f738..6d3b9964c8 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -111,6 +111,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     if (export->has_iothread) {
         IOThread *iothread;
         AioContext *new_ctx;
+        Error **set_context_errp;
 
         iothread = iothread_by_id(export->iothread);
         if (!iothread) {
@@ -120,7 +121,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
 
         new_ctx = iothread_get_aio_context(iothread);
 
-        ret = bdrv_try_set_aio_context(bs, new_ctx, errp);
+        /* Ignore errors with fixed-iothread=false */
+        set_context_errp = fixed_iothread ? errp : NULL;
+        ret = bdrv_try_set_aio_context(bs, new_ctx, set_context_errp);
         if (ret == 0) {
             aio_context_release(ctx);
             aio_context_acquire(new_ctx);
-- 
2.31.1

[PATCH v2 2/2] iotests/307: Test iothread conflict for exports

[Max Reitz]

Passing fixed-iothread=true should make iothread conflicts fatal,
whereas fixed-iothread=false should not.

Combine the second case with an error condition that is checked after
the iothread is handled, to verify that qemu does not crash if there is
such an error after changing the iothread failed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/307     | 15 +++++++++++++++
 tests/qemu-iotests/307.out |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/tests/qemu-iotests/307 b/tests/qemu-iotests/307
index c7685347bc..b429b5aa50 100755
--- a/tests/qemu-iotests/307
+++ b/tests/qemu-iotests/307
@@ -41,9 +41,11 @@ with iotests.FilePath('image') as img, \
     iotests.log('=== Launch VM ===')
 
     vm.add_object('iothread,id=iothread0')
+    vm.add_object('iothread,id=iothread1')
     vm.add_blockdev(f'file,filename={img},node-name=file')
     vm.add_blockdev(f'{iotests.imgfmt},file=file,node-name=fmt')
     vm.add_blockdev('raw,file=file,node-name=ro,read-only=on')
+    vm.add_blockdev('null-co,node-name=null')
     vm.add_device(f'id=scsi0,driver=virtio-scsi,iothread=iothread0')
     vm.launch()
 
@@ -74,6 +76,19 @@ with iotests.FilePath('image') as img, \
     vm.qmp_log('query-block-exports')
     iotests.qemu_nbd_list_log('-k', socket)
 
+    iotests.log('\n=== Add export with conflicting iothread ===')
+
+    vm.qmp_log('device_add', id='sdb', driver='scsi-hd', drive='null')
+
+    # Should fail because of fixed-iothread
+    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='null',
+               iothread='iothread1', fixed_iothread=True, writable=True)
+
+    # Should ignore the iothread conflict, but then fail because of the
+    # permission conflict (and not crash)
+    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='null',
+               iothread='iothread1', fixed_iothread=False, writable=True)
+
     iotests.log('\n=== Add a writable export ===')
 
     # This fails because share-rw=off
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index 4b0c7e155a..ec8d2be0e0 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -51,6 +51,14 @@ exports available: 1
    base:allocation
 
 
+=== Add export with conflicting iothread ===
+{"execute": "device_add", "arguments": {"drive": "null", "driver": "scsi-hd", "id": "sdb"}}
+{"return": {}}
+{"execute": "block-export-add", "arguments": {"fixed-iothread": true, "id": "export1", "iothread": "iothread1", "node-name": "null", "type": "nbd", "writable": true}}
+{"error": {"class": "GenericError", "desc": "Cannot change iothread of active block backend"}}
+{"execute": "block-export-add", "arguments": {"fixed-iothread": false, "id": "export1", "iothread": "iothread1", "node-name": "null", "type": "nbd", "writable": true}}
+{"error": {"class": "GenericError", "desc": "Permission conflict on node 'null': permissions 'write' are both required by an unnamed block device (uses node 'null' as 'root' child) and unshared by block device 'sdb' (uses node 'null' as 'root' child)."}}
+
 === Add a writable export ===
 {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
 {"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': permissions 'write' are both required by an unnamed block device (uses node 'fmt' as 'root' child) and unshared by block device 'sda' (uses node 'fmt' as 'root' child)."}}
-- 
2.31.1

Re: [PATCH v2 1/2] block/export: Conditionally ignore set-context error

[Vladimir Sementsov-Ogievskiy]

24.06.2021 11:38, Max Reitz wrote:
> When invoking block-export-add with some iothread and
> fixed-iothread=false, and changing the node's iothread fails, the error
> is supposed to be ignored.
> 
> However, it is still stored in *errp, which is wrong.  If a second error
> occurs, the "*errp must be NULL" assertion in error_setv() fails:
> 
>    qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion
>    `*errp == NULL' failed.
> 
> So if fixed-iothread=false, we should ignore the error by passing NULL
> to bdrv_try_set_aio_context().
> 
> Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef
>         ("block/export: add iothread and fixed-iothread options")
> Signed-off-by: Max Reitz<mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/2] iotests/307: Test iothread conflict for exports

[Vladimir Sementsov-Ogievskiy]

24.06.2021 11:38, Max Reitz wrote:
> Passing fixed-iothread=true should make iothread conflicts fatal,
> whereas fixed-iothread=false should not.
> 
> Combine the second case with an error condition that is checked after
> the iothread is handled, to verify that qemu does not crash if there is
> such an error after changing the iothread failed.
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 0/2] block/export: Conditionally ignore set-context error

[Kevin Wolf]

Am 24.06.2021 um 10:38 hat Max Reitz geschrieben:
> Max Reitz (2):
>   block/export: Conditionally ignore set-context error
>   iotests/307: Test iothread conflict for exports

Thanks, applied to the block branch.

Kevin