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

[Christophe Fergeau]

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

> 
> 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 */
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel