[PATCH] Third and Final (hopefully)
robert.mcqueen at collabora.co.uk
Tue Nov 8 18:57:34 PST 2005
John (J5) Palmieri wrote:
> Me ;-) but I can't approve my own patch. It seems as though most of the
> people who have a right to review patches for submission have dropped
> off the list or just don't check it. I was going to blog about people
> being able to review patches without having the right to say ok to
> commit. It makes it easier to assess if we should give a person that
> right and it lightens the load for all of us.
FWIW, I've read the patch through for a while, and although I wouldn't
claim to be familiar with the code (I just understood enough to get my
ReleaseName patch going) there's nothing in the implementation that
screams at me as being wrong, and the test case looks good. It applies,
compiles and tests correctly here too.
One minor improvement might be to remove the "@todo: this all seems
rather broken" in the doxgen comment, if we can agree on that being the
case. :) On many levels, what's more important is that the API and the
spec are correct, because a deviation from this in the implementation is
a bug which we can fix later in the release cycle. My thoughts on the
> Anyway, my take on this patch. I'm not sure of the use cases other than
> the text editor one you outlined. What I am sure of is it is logical,
> at least more so than the PROHIBIT_REPLACEMENT version which didn't seem
> to follow any strict rules. The way I see it is we either do it this
> way or revert back to only allowing one owner and no queue. Services
> can poll or listen for signals and race for ownership. I would sulk for
> a few seconds if we do revert because it does mean more coding and I had
> fun with this particular patch but really it would only be for a few
I agree: I don't see the use case either, but I do think that the
current behaviour is weird. It seems a far more sensible default to not
allow yourself to be bumped off the bus unless you explicitly
acknowledge this possibility at request time, release your name
explicitly, or disconnect/crash/exit.
> What is clear is we should pick one as the intermediate was inadequate.
> Perhaps we should change DO_NOT_QUEUE to ALLOW_QUEUE and have it behave
> as a no queue entity by default and keep the added functionality.
I agree again. On balance, I think having a queue is probably a win,
because it allows applications contending for a name to discover who
else is trying to provide that name, and choose whether to queue, time
out, etc, or not, so I'd prefer the proposed situation to just
abandoning the flags entirely.
I do however question the choice of joining the queue by default - an
application that's expecting to provide a service should probably have
to explicitly indicate it wants to stick around even if it can't provide
that service, without having to monitor any further signals to determine
what's going on or decide what to do.
To me, going with ALLOW_REPLACEMENT and switching DO_NOT_QUEUE to
ALLOW_QUEUE would make the behaviour the most intuitive. Baseline is:
when you get a name, it's yours, and nobody else can get it, and if they
try then they fail. If you want to deviate from this functionality, you
may choose to allow other people to usurp you, or you may choose to wait
for someone to move off a name, which indicates a commitment that you
will monitor the signals and behave appropriately in these cases.
> In this case it would be harder to add this functionality should we take
> it out now.
More information about the dbus