[Spice-devel] [PATCH 01/11] FIXME hardcoded 58?

[Jonathon Jongsma]

ACK


On Thu, 2015-10-29 at 13:32 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, 2015-10-29 at 07:24 -0400, Frediano Ziglio wrote:
> > > > 
> > > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > 
> > > > ---
> > > >  server/red_worker.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > index c027fde..3aa05d8 100644
> > > > --- a/server/red_worker.c
> > > > +++ b/server/red_worker.c
> > > > @@ -9802,7 +9802,7 @@ static void
> > > > guest_set_client_capabilities(RedWorker
> > > > *worker)
> > > >      DisplayChannelClient *dcc;
> > > >      RedChannelClient *rcc;
> > > >      RingItem *link, *next;
> > > > -    uint8_t caps[58] = { 0 };
> > > > +    uint8_t caps[58] = { 0 }; /* FIXME: 58?? */
> > > >      int caps_available[] = {
> > > >          SPICE_DISPLAY_CAP_SIZED_STREAM,
> > > >          SPICE_DISPLAY_CAP_MONITORS_CONFIG,
> > > > --
> > > > 2.4.3
> > > > 
> > > 
> > > I think the reply came from 
> > > http://cgit.freedesktop.org/spice/spice-p
> > > rotocol/commit/?id=361fd166b26b4450617b1f7175be9aaa7d8f6a7e
> > > The "fix" would require a change (a definition) in spice-protocol
> > > spice/qxl_dev.h file and use the mnemonic instead.
> > 
> > I guess the spice-protocol structure definition is the root cause
> > of
> > this number, but the direct reason that we're allocating this size
> > array here is probably because of the definition of
> > QxlInterface::set_client_capabilities() in server/spice-qxl.h.
> > There is
> > also a "magic" 58 in that function declaration. So if we're going
> > to
> > address the one in red_worker.c, we should probably address them
> > both.
> > 
> > > 
> > > Or we could use instead a sizeof(((QXLRom*)0)
> > > ->client_capabilities).
> > > 
> > > Personally I would prefer the second, we just released a spice
> > > -protocol release so doing the change,
> > > bumping version and update version on spice-server is quite long.
> > 
> > 
> > This seems OK to me.
> > 
> > Another option is to do both as an transitional measure. For
> > example,
> > introduce a SPICE_CAPABILITIES_SIZE #define in spice-protocol, and
> > then
> > in spice server do something like
> > 
> > #ifndef SPICE_CAPABILITIES_SIZE
> >  #define SPICE_CAPABILITIES_SIZE sizeof(...)
> > #endif
> > 
> > But I'm not sure it's worth the effort.
> > 
> 
> I think this patch can go
> 
> [PATCH] worker: avoid to use constant directly for capability size
> 
> ---
>  server/red_worker.c | 2 +-
>  server/spice-qxl.h  | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 96c0f14..2b23ffd 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9802,7 +9802,7 @@ static void
> guest_set_client_capabilities(RedWorker *worker)
>      DisplayChannelClient *dcc;
>      RedChannelClient *rcc;
>      RingItem *link, *next;
> -    uint8_t caps[58] = { 0 };
> +    uint8_t caps[SPICE_CAPABILITIES_SIZE] = { 0 };
>      int caps_available[] = {
>          SPICE_DISPLAY_CAP_SIZED_STREAM,
>          SPICE_DISPLAY_CAP_MONITORS_CONFIG,
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index dd49a86..e1f14e7 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -24,6 +24,10 @@
>  
>  #include "spice-core.h"
>  
> +#ifndef SPICE_CAPABILITIES_SIZE
> +#define SPICE_CAPABILITIES_SIZE (sizeof(((QXLRom*)0)
> ->client_capabilities))
> +#endif
> +
>  /* qxl interface */
>  
>  #define SPICE_INTERFACE_QXL "qxl"
> @@ -175,7 +179,7 @@ struct QXLInterface {
>                                   uint32_t num_updated_rects);
>      void (*set_client_capabilities)(QXLInstance *qin,
>                                      uint8_t client_present,
> -                                    uint8_t caps[58]);
> +                                    uint8_t
> caps[SPICE_CAPABILITIES_SIZE]);
>      /* returns 1 if the interface is supported, 0 otherwise.
>       * if monitors_config is NULL nothing is done except reporting
> the
>       * return code. */