[Spice-devel] [PATCH spice-server] cursor-channel: Remove obsolete size check

[Victor Toso]

Hi,

On Thu, Jun 08, 2017 at 09:56:52AM -0400, Frediano Ziglio wrote:
> >
> > Hi,
> >
> > On Thu, Jun 08, 2017 at 09:32:47AM -0400, Frediano Ziglio wrote:
> > > > > > > Is that useless now?
> > > > > >
> > > > > > Maybe but is not so easy to remove. Now, did some grep too and
> > > > > > basically
> > > > > > nobody uses device_data field. Is not surprising as was used to store
> > > > > > spice-server data in qxl card memory... once spice-server usage was
> > > > > > gone the fields stopped to be used.
> > > > > >
> > > > > > However this now is an ABI and the size of QXLCursorCmd is part of
> > > > > > the ABI.
> > > >
> > > > Would it work if we keep the struct size but removing the field?
> > > >
> > >
> > > Yes, we could for instance replace
> > >
> > >    uint8_t device_data[QXL_CURSUR_DEVICE_DATA_SIZE]; //todo: dynamic size
> > >    from rom
> > >
> > > with something like
> > >
> > >    /* this field is to keep ABI compatibility but is actually never used */
> > >    uint8_t unused[128];
> >
> > IMHO, that's a good approach.
> >
> > > and remove QXL_CURSUR_DEVICE_DATA_SIZE definition. Or add a 128+13 byte
> > > field inside the "u" union.
> > >
> > > CURSUR ? well... this would remove the typo too :-)
> >
> > :)
> >
> > > An option to remove the field would be to reduce structure size check
> > > on spice-server, wait some years to make sure that old disappears and
> > > then remove from the guest.
> >
> > Why not spice 1.0 / spice-gtk 1.0 and break compatibility entirely...
> > Imagine the possibilities!
> >
>
> Maybe a bit OT here, I was thinking some days ago about Spice protocol
> 1 (spice1.proto in spice-common and all generated stuff).
> Looks like the last change in the protocol was back in 2013 adding
> OPUS enumeration.
> Also version 2 file was introduced back in 2010 (end of May).

OT yes, but something worth to discuss/consider at some point although
I'm not sure if there are enough benefits for breaking compatibility,
etc.

>
> > >
> > > > > > Although this is the last field and QXLCursorCmd is not
> > > > > > included in any array/structures (but just pointed from the cursor
> > > > > > ring) this can cause problems. Let say for instance that the field is
> > > > > > removed. The QXL drivers will allocate less bytes and pass less
> > > > > > bytes.
> > > > > > Now if the spice-server you are using is using the old length it will
> > > > > > check for memory bounds using the old (bigger) size. If the cursor
> > > > > > commands was allocated at the end of QXL memory region (quite
> > > > > > unlikely
> > > > > > but not impossible) the red-parse-qxl.c code will detect an error and
> > > > > > discard the command. Not sure if this is the only issue of removing
> > > > > > the field but I think that this change is not strongly related to
> > > > > > this one and should be addresses in a follow up.
> > > > > >
> > > > > > > (hmm, checking qxl-wddm-dod)
> > > > > > > [0] https://gitlab.com/spice/qxl-wddm-dod
> > > > > > > 
> > > > > > > Okay, here is also not used and header is actually duplicated
> > > > > > > (shouldn't
> > > > > > > we use the header from spice-protocol here? see:
> > > > > > > 
> > > > > > >   $ qxl-wddm-dod (master 1a81d6d3) $ grepi "qxl_dev" *
> > > > > > >   qxldod/include/qxl_dev.h:32:#ifndef _H_QXL_DEV
> > > > > > >   qxldod/include/qxl_dev.h:33:#define _H_QXL_DEV
> > > > > > >   qxldod/include/qxl_dev.h:65:#define QXL_DEVICE_ID_STABLE 0x0100
> > > > > > >   qxldod/include/qxl_dev.h:74:#define QXL_DEVICE_ID_DEVEL 0x01ff
> > > > > > >   qxldod/include/qxl_dev.h:808:#endif /* _H_QXL_DEV */
> > > > > > >   qxldod/QxlDod.h:13:#include "qxl_dev.h"
> > > > > > > 
> > > > > > > Anyway, patch looks fine but we might want to do some follow ups.
> > > > > > > 
> > > > > > 
> > > > > > Yes, surely.
> > > > 
> > > > Sorry, should have been clear here that either way
> > > > Acked-by: Victor Toso <victortoso at redhat.com>
> > > > 
> > > > Cheers,
> > > > > > 
> > > > > > > >  struct CursorChannel
> > > > > > > >  {
> > > > > > > >      CommonGraphicsChannel parent;
> > > > > > 
> > > 
> > > Thanks,
> > >   Frediano
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/228c12e3/attachment.sig>