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

Submitted by Frediano Ziglio on Oct. 29, 2015, 11:09 a.m.

Details

Message ID 1446116978-26548-2-git-send-email-fziglio@redhat.com
State New
Headers show
Series "Backported some patches from refactory branches (29th Oct)" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Oct. 29, 2015, 11:09 a.m.
From: Marc-André Lureau <marcandre.lureau@gmail.com>

---
 server/red_worker.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

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,

Comments

> 
> From: Marc-André Lureau <marcandre.lureau@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-protocol/commit/?id=361fd166b26b4450617b1f7175be9aaa7d8f6a7e
The "fix" would require a change (a definition) in spice-protocol spice/qxl_dev.h file and use the mnemonic instead.

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.

Frediano
On Thu, 2015-10-29 at 07:24 -0400, Frediano Ziglio wrote:
> > 
> > From: Marc-André Lureau <marcandre.lureau@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.