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

[Frediano Ziglio]

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

Frediano

> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-worker.h | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/red-worker.h b/server/red-worker.h
> > index 4f64b729..21a08c7b 100644
> > --- a/server/red-worker.h
> > +++ b/server/red-worker.h
> > @@ -44,16 +44,19 @@ typedef uint32_t RedWorkerMessage;
> >  
> >  /* Keep message order, only append new messages!
> >   * Replay code store enum values into save files.
> > + * To avoid missing this rule the constants ever used by replay are
> > + * explicitly numbered to cause a runtime error when duplicate values
> > + * are inserted into the dispatcher.
> >   */
> >  enum {
> >      RED_WORKER_MESSAGE_NOP,
> >  
> > -    RED_WORKER_MESSAGE_UPDATE,
> > -    RED_WORKER_MESSAGE_WAKEUP,
> > +    RED_WORKER_MESSAGE_UPDATE = 1, /* replay */
> > +    RED_WORKER_MESSAGE_WAKEUP = 2, /* replay */
> >      RED_WORKER_MESSAGE_OOM,
> >      RED_WORKER_MESSAGE_READY, /* unused */
> >  
> > -    RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> > +    RED_WORKER_MESSAGE_DISPLAY_CONNECT = 5, /* replay */
> >      RED_WORKER_MESSAGE_DISPLAY_DISCONNECT,
> >      RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> >      RED_WORKER_MESSAGE_START,
> > @@ -67,9 +70,9 @@ enum {
> >      RED_WORKER_MESSAGE_ADD_MEMSLOT,
> >      RED_WORKER_MESSAGE_DEL_MEMSLOT,
> >      RED_WORKER_MESSAGE_RESET_MEMSLOTS,
> > -    RED_WORKER_MESSAGE_DESTROY_SURFACES,
> > -    RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE,
> > -    RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE,
> > +    RED_WORKER_MESSAGE_DESTROY_SURFACES = 19, /* replay */
> > +    RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE = 20, /* replay */
> > +    RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE = 21, /* replay */
> >      RED_WORKER_MESSAGE_RESET_CURSOR,
> >      RED_WORKER_MESSAGE_RESET_IMAGE_CACHE,
> >      RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT,
> > @@ -78,7 +81,7 @@ enum {
> >      RED_WORKER_MESSAGE_UPDATE_ASYNC,
> >      RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC,
> >      RED_WORKER_MESSAGE_DESTROY_SURFACES_ASYNC,
> > -    RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC,
> > +    RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC = 29, /* replay */
> >      RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE_ASYNC,
> >      RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC,
> >      /* suspend/windows resolution change command */