diff mbox series

[1/2] block/export: Free ignored Error

Message ID 20210422145335.65814-2-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series block/export: Fix crash on error after iothread conflict | expand

Commit Message

Max Reitz April 22, 2021, 2:53 p.m. UTC
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(+)

Comments

Vladimir Sementsov-Ogievskiy April 26, 2021, 9:44 a.m. UTC | #1
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. 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():

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.
Max Reitz April 26, 2021, 10:33 a.m. UTC | #2
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.
> 
>
Vladimir Sementsov-Ogievskiy April 26, 2021, 11:01 a.m. UTC | #3
26.04.2021 13:33, Max Reitz wrote:
> 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).)

Hm. I don't know. May be you are right.. Actually, I don't care too much, so, patch is OK as is:

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

> 
> 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.
> 

I remember we still have some functions not setting errp on some error paths.. bdrv_open_driver() has work-around for such bad .*open handlers of some drivers... So I decided to look through.

> 
>> 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.
>>
>>
>
diff mbox series

Patch

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;
         }
     }