|
From: | Max Reitz |
Subject: | Re: [PATCH 1/2] block/export: Free ignored Error |
Date: | Mon, 26 Apr 2021 12:33:49 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 26.04.21 11:44, Vladimir Sementsov-Ogievskiy wrote:
22.04.2021 17:53, 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 the error from bdrv_try_set_aio_context() must be freed when it is ignored. Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef ("block/export: add iothread and fixed-iothread options") Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/export/export.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/block/export/export.c b/block/export/export.c index fec7d9f738..ce5dd3e59b 100644 --- a/block/export/export.c +++ b/block/export/export.c@@ -68,6 +68,7 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) { + ERRP_GUARD();bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread;const BlockExportDriver *drv; BlockExport *exp = NULL;@@ -127,6 +128,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)ctx = new_ctx; } else if (fixed_iothread) { goto fail; + } else { + error_free(*errp); + *errp = NULL; } }I don't think ERRP_GUARD is needed in this case: we don't need to handle errp somehow except for just free if it was set.
Perhaps not, but style-wise, I prefer not special-casing the errp == NULL case.(It can be argued that ERRP_GUARD similarly special-cases it, but that’s hidden from my view. Also, the errp == NULL case actually doesn’t even happen, so ERRP_GUARD is effectively a no-op and it won’t cost performance (not that it really matters).)
Of course we could also do this: ret = bdrv_try_set_aio_context(bs, new_ctx, fixed_iothread ? errp : NULL); Would be even shorter.
So we can simply do: } else if (errp) { error_free(*errp); *errp = NULL; }Let's only check that errp is really set on failure path of bdrv_try_set_aio_context():
OK, but out of interest, why? error_free() doesn’t care. I mean it might be a problem if blk_exp_add() returns an error without setting *errp, but that’d’ve been pre-existing.
Max
bdrv_try_set_aio_context() fails iff bdrv_can_set_aio_context() fails, which in turn may fail iff bdrv_parent_can_set_aio_context() or bdrv_child_can_set_aio_context() fails.bdrv_parent_can_set_aio_context() has two failure path, on first it set errp by hand, and on second it has assertion that errp is set.bdrv_child_can_set_aio_context() may fail only if nested call to bdrv_can_set_aio_context() fails, so recursion is closed.
[Prev in Thread] | Current Thread | [Next in Thread] |