Re: [PATCH v7 03/14] block: check return value of bdrv_open_child and drop error propagation

[Vladimir Sementsov-Ogievskiy]

15.02.2021 23:04, Eric Blake wrote:

On 2/15/21 3:22 AM, Kevin Wolf wrote:

With this patch applied, 'check unit-test' fails with:

Running test test-replication
Unexpected error in bdrv_open_driver() at ../block.c:1481:
Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument
ERROR test-replication - missing test plan

The problem is this hunk:

diff --git a/block/qcow2.c b/block/qcow2.c
index 5d94f45be9..e8dd42d73b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1611,9 +1611,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
      /* Open external data file */
      s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
                                     &child_of_bds, BDRV_CHILD_DATA,
-                                   true, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                   true, errp);
+    if (!s->data_file) {
          ret = -EINVAL;
          goto fail;
      }

bdrv_open_child() can return NULL in non-error cases, when the child is
optional and it isn't given. The test doesn't use an external data file,
so this returns NULL without setting an error, which now gets turned
into -EINVAL instead.

This makes the most basic tests fail with qcow2 (iotests 001 is enough).

The other hunks in this patch don't suffer from the same problem because
they pass allow_none=false.

Thanks; that's enough to figure out how to repair the patch:

diff --git i/block/qcow2.c w/block/qcow2.c
index e8dd42d73b4c..38137ca30eb0 100644
--- i/block/qcow2.c
+++ w/block/qcow2.c
@@ -1292,6 +1292,7 @@ static int
validate_compression_type(BDRVQcow2State *s, Error **errp)
  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
                                        int flags, Error **errp)
  {
+    ERRP_GUARD();
      BDRVQcow2State *s = bs->opaque;
      unsigned int len, i;
      int ret = 0;
@@ -1612,7 +1613,7 @@ static int coroutine_fn
qcow2_do_open(BlockDriverState *bs, QDict *options,
      s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
                                     &child_of_bds, BDRV_CHILD_DATA,
                                     true, errp);
-    if (!s->data_file) {
+    if (*errp) {
          ret = -EINVAL;
          goto fail;
      }

Agree.. Or better refactor bdrv_open_child to follow more common (and 
recommended) semantics (i.e. NULL + errp on error, non-null on succsess).. But 
this will require more investigation.

I'm busy now with our internal deadline (this week), after this will return to 
handle upstream things.

--
Best regards,
Vladimir