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

[Christophe Fergeau]

On Wed, Sep 06, 2017 at 10:48:40AM -0400, Frediano Ziglio wrote:
> > 
> > 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 :-)

For now I think I'd be fine with compile-time assertions even if this is
not perfect ;)

Christophe