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! > > > > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel