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

[Frediano Ziglio]

> 
> 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
> 

I already regret my suggestion :-)

Frediano