[Spice-devel] [PATCH spice-server 2/3] red-worker: Avoid changing message numbers used by replay utility

[Christophe Fergeau]

On Mon, Sep 04, 2017 at 10:47:45AM -0400, Frediano Ziglio wrote:
> > 
> > On Fri, Sep 01, 2017 at 10:36:58AM +0100, Frediano Ziglio wrote:
> > > These constants are saved into record files so their values should not
> > > be changed in order to be able to use old record files.
> > > Recently 2 different patches has attempted to change these values
> > > without noticing the command above the enumeration.
> > > A runtime error will occur registering duplicate message numbers as
> > > soon as a display is created inside the VM.
> > 
> > I would add a big /* INSERT NEW MESSAGES HERE */ right before
> > RED_WORKER_MESSAGE_COUNT, I *think* I'm much more likely to
> > be looking at the end of the enum rather than at its beginning when
> > looking where to add a new member.
> > 
> > Why not use
> > verify(RED_WORKER_MESSAGE_DISPLAY_CONNECT == 5);
> > verify(RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC == 29);
> > to get compile-time failures if these get moved by mistake?
> > 
> > Christophe
> > 
> 
> Would make sense. However I realized that this is not enough.
> 
> Let say that I record the event 20 which is not handled (now)
> by the replay. I though: well, I can reuse this value.
> But consider if I want to try the same file with a new version.
> Maybe now the event 20, which does another thing, is now handled
> by the replay so the old file may stop working. Which is
> something I don't want as I want to continue to use the file
> for testing. So I would have to fix all the numbers, used or
> not by the replay. Which is what the comment said, but not
> what this patch is doing.
> On the other hand recording is not the main aim of these messages.
> And having holes in the enumeration in the long run would
> make the handler array bigger while maybe for cache efficiency
> would be better to have it contiguous.
> But maybe here I'm a bit too paranoid.
> We could save names instead of numbers or define different
> fixed enumeration for record/replay.

Yes, this sounds better. The enum used for these dispatcher messages is
imo an internal implementation detail, the recording format is kind of
turning it into an ABI of some sort, which I don't think is good.

Christophe