[systemd-devel] [PATCH] domain: fix a BUG_ON() when kdbus_domain_new() fails

[Djalal Harouni]

On Tue, Jun 03, 2014 at 12:55:54PM +0200, Kay Sievers wrote:
> On Mon, Jun 2, 2014 at 5:57 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
> > Currently just running: test/test-kdbus will trigger the BUG_ON()
> > appended at the bottom.
> >
> > This is due to the test in check_domain_make() where we try to register
> > the same domain twice line: 297, hence kdbus_domain_new() fails with
> > -EEXIST at line domain.c:289
> >
> > Later on error path we clear the non-finalized domain:
> > kdbus_domain_unref()
> >  => __kdbus_domain_free()
> >    => BUG_ON(!domain->disconnected)
> 
> Oh, hmm, I should probably run our own tests from time to time, not
> just the full system as a test. :)
:-)


> > After a closer look, it seems we will hit this BUG_ON() on every time
> > kdbus_domain_new() fails. domain was not finalized so
> > kdbus_domain_disconnect() is never called, and domain->disconnect can't
> > be true.
> >
> > To fix this, I just set 'domain->disconnect = true' at the beginning
> > which is perfectly true since that domain is not finalized hence not
> > connected, and before we return success set it again to 'false' in other
> > words: connected.
> >
> > I just took this path since it seems logic, and having a single exit
> > node "kdbus_domain_unref()" on success/errors wich passes all these
> > BUG_ON() makes the code robust.
> >
> > In other places: bus, endpoints we do not follow this and we duplicate
> > the unref() logic, for endpoints it would be easy to convert! I'll
> > probably follow with patches for that.
> 
> It was that way earlier, it will probably not work out for the others.
> We needed to open-code it because the objects were not fully
> initialized and the rollback was not possible without doing weird
> things.
Ok.

> I actually think adding the needed 3 free() calls here, like for the
> other objects, is shorter, needs no explanation and is easier to
> follow.
Ok, I'll do that, and it seems we do not clean the chrdev and idr too,
will send a patch.

Thanks

> Kay

-- 
Djalal Harouni
http://opendz.org