>
> Hi,
>
> On Thu, Jun 08, 2017 at 09:09:35AM -0400, Frediano Ziglio wrote:
> > ping
> >
> > >
> > > >
> > > > Hi,
> > > >
> > > > On Mon, May 22, 2017 at 11:41:02AM +0100, Frediano Ziglio wrote:
> > > > > In the past CursorItem structure was stored in
> > > > > RedCursorCmd::device_data field so the check was there to check if
> > > > > the
> > > > > structure fit into that field.
> > > > > Since 2ba69f9f8819daaa3d166c4c1c7e03b121b88a95
> > > >
> > > > A lit bit hard to follow, this one
> > > >
> > >
> > > Yes, the mentioned patch is quite big and the topic is not much related
> > > to this. However you can see that "device_data" access was removed.
> > >
> > > > > ("libspice: add surface 0 support") the structure is no more stored
> > > > > in
> > > > > the field so there's no reason for this check causing only confusion.
> > > > >
> > > > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > > > ---
> > > > > server/cursor-channel.c | 2 --
> > > > > 1 file changed, 2 deletions(-)
> > > > >
> > > > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > > > index 6aea64a..a876113 100644
> > > > > --- a/server/cursor-channel.c
> > > > > +++ b/server/cursor-channel.c
> > > > > @@ -34,8 +34,6 @@ typedef struct CursorItem {
> > > > > RedCursorCmd *red_cursor;
> > > > > } CursorItem;
> > > > >
> > > > > -G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
> > > > > -
> > > >
> > > > I don't see any other use for QXL_CURSUR_DEVICE_DATA_SIZE in my local
> > > > workspace besides:
> > > >
> > > > spice-protocol/spice/qxl_dev.h:335:
> > > > #define QXL_CURSUR_DEVICE_DATA_SIZE 128
> > > > spice-protocol/spice/qxl_dev.h:352:
> > > > uint8_t device_data[QXL_CURSUR_DEVICE_DATA_SIZE]; //todo: dynamic
> > > > size
> > > > from
> > > > rom
> > > >
> > > > 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];
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.
> > > 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@xxxxxxxxxx>
>
> Cheers,
> > >
> > > > > struct CursorChannel
> > > > > {
> > > > > CommonGraphicsChannel parent;
> > >
Thanks,
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel