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

Submitted by Frediano Ziglio on Sept. 1, 2017, 9:36 a.m.

Details

Message ID 20170901093659.24154-2-fziglio@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Sept. 1, 2017, 9:36 a.m.
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.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/red-worker.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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

Comments

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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 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@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 */
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
> 
> 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
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