On Tue, Jun 27, 2017 at 04:34:22PM +0300, Manos Pitsidianakis wrote:
> On Tue, Jun 27, 2017 at 01:45:40PM +0100, Stefan Hajnoczi wrote:
> > On Mon, Jun 26, 2017 at 07:26:41PM +0300, Manos Pitsidianakis wrote:
> > > On Mon, Jun 26, 2017 at 03:30:55PM +0100, Stefan Hajnoczi wrote:
> > > > > +    bs->file = bdrv_open_child(NULL, options, "file",
> > > > > +                                    bs, &child_file, false, &local_err);
> > > > > +
> > > > > +    if (local_err) {
> > > > > +        error_propagate(errp, local_err);
> > > > > +        return -EINVAL;
> > > > > +    }
> > > > > +
> > > > > +    qdict_flatten(options);
> > > > > +    return throttle_configure_tgm(bs, tgm, options, errp);
> > > >
> > > > Who destroys bs->file on error?
> > > 
> > > It is reaped by bdrv_open_inherit() on failure, if I'm not mistaken.
> > > That's how other drivers handle this as well. Some (eg block/qcow2.c)
> > > check if bs->file is NULL instead of the error pointer they pass, so
> > > this is not not very consistent.
> > 
> > Maybe I'm missing it but I don't see relevant bs->file cleanup in
> > bdrv_open_inherit() or bdrv_open_common().
> > 
> > Please post the exact line where it happens.
> > 
> > Stefan
> 
> Relevant commit: de234897b60e034ba94b307fc289e2dc692c9251 block: Do not
> unref bs->file on error in BD's open
> 
> bdrv_open_inherit() does this on failure:
> 
> fail:
>    blk_unref(file);
>    if (bs->file != NULL) {
>        bdrv_unref_child(bs, bs->file);
>    }

Thanks, you are right.  I missed it.

> While looking into this I noticed bdrv_new_open_driver() doesn't handle
> bs->file on failure. It simply unrefs the bs but because its child's ref
> still remains, it is leaked.

That's a good candidate for a separate bug fix patch.