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

[Vladimir Sementsov-Ogievskiy]

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.


--
Best regards,
Vladimir