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

[Uri Lublin]

On 06/08/2017 04:09 PM, Frediano Ziglio wrote:
> ping

Ack.

Uri.

> 
>>
>>>
>>> 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 at redhat.com>
>>>> ---
>>>>   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. 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.
>>
>>>>   struct CursorChannel
>>>>   {
>>>>       CommonGraphicsChannel parent;
>>
>> Frediano
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>