[PATCH wayland] server: Make serial number 0 mean invalid

[Derek Foreman]

On 30/09/15 03:23 PM, Jasper St. Pierre wrote:
> I don't necessarily like this. The absence of a serial can have
> radical meanings on behavior. Being able to pass 0 to mean "no serial"
> anywhere we currently rely on a serial seems like poor design to me,
> and can easily be done by mistake.

Eek, this wasn't my intention at all.  I figured I'd have to add errors
to some existing protocol in the case that they were passed "invalid serial"

There are cases in weston where it would be quite nice to have a
sentinel value to use instead of having to have a bool for "this serial
number is legit" too.

> In cases where we have two behaviors for serial-aware and
> non-serial-aware operations, I would rather have two different client
> requests.

Fair enough.

(there's still some text further down...)

> On Wed, Sep 30, 2015 at 12:47 PM, Daniel Stone <daniel at fooishbar.org> wrote:
>> Hi,
>>
>> On 30 September 2015 at 16:59, Derek Foreman <derekf at osg.samsung.com> wrote:
>>> Having an invalid serial number is quite useful - for example, we can
>>> have protocol requests that optionally take a serial number or 0
>>> instead of having two similar requests.
>>>
>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>> ---
>>>
>>> Well, let's see where this goes ;)
>>>
>>> I've seen a few times now when having an invalid serial number would be
>>> helpful, is it too late to do this?  Are we going to break anything?
>>>
>>> Anything currently doing math on serial numbers can be wrong by one if one
>>> of the serial numbers has wrapped around - this probably doesn't matter?
>>>
>>> I do wonder if we should have a wl_serial_compare() as part of wayland
>>> but the semantics are tricky (a big serial and a small serial may actually
>>> have happened quite close to eachother if the small happened after a
>>> wrap around).
>>>
>>> I think the usual comparisons of interest are equality and "is A less than B
>>> within (arbitrary distance to compensate for wrap-around)".  I think strcmp()
>>> like semantics won't necessarily be good here because we don't usually need
>>> something to tell us "b happened before a", more "these almost certainly
>>> happened in the order specified in the function call".
>>>
>>> Where do we go from here?
>>
>> From the IRC discussion way back when:
>> 20:27 < krh> if we do need to make a new serial, I'd much prefer to
>> have it be 1 << 31
>> 20:27 < krh> and then just restrict normal serial numbers space to 0 -
>> (1<<31 - 1)
>> 20:28 < krh> so that serial number increment is just serial = (serial
>> + 1) & 0x7fffffff
>>
>> I have no relevant opinion on the matter, but I guess that 0x80000000
>> (or 0xffffffff) is a much more obviously invalid serial number than 0,
>> which could also just be the victim of something uninitialised.

Yeah... I prefer 0 for just that reason - if someone's passing an
uninitialized serial they're buggy and it'd be nice to know about it (at
least in a log file or something - I'm not necessarily advocating a kill)

Also, we get 0 for free from any of our zalloc helpers, which is nice if
we're using the invalid serial to mean "haven't received a valid serial
yet" or similar in weston.

If we are going to make half the space invalid, we do have the option of
having an "invalid serial" and a "no serial" value - but this is
starting to make the serial feel a little too opaque/confusing I think?

So, I guess now we have two problems:

Do we even really want an "invalid serial"?

What should it be?


I'm fine with the non-zero invalid serial if the first question can be
resolved... :)

>> A helper would be nice, though like you say I think it has to be
>> directional/multi-return (older inc. wraparound? identical?
>> newer/error? invalid?). It'd probably be best to do that inside an
>> active user and only then shuffle it into Wayland.
>>
>> So, with the change to a different serial, and a note in documentation
>> somewhere that any extension relying on the invalid-serial behaviour
>> must itself explicitly document that:
>> Acked-by: Daniel Stone <daniels at collabora.com>
>>
>> Cheers,
>> Daniel
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> 
>