[Spice-devel] [PATCH spice-protocol 1/2 v2] qxl_dev: Fix alignment for QXLReleaseInfo

[Snir Sheriber]

On 7/18/19 6:37 PM, Frediano Ziglio wrote:
>>
>> On 7/18/19 5:51 PM, Frediano Ziglio wrote:
>>>> Hi,
>>>>
>>>>
>>>> IIRC this was related to some compiler warning, no?
>>> Yes, recent compilers are reporting it, see below.
>>>
>>>> If it is I'd mentioning it , otherwise, ack.
>>>>
>>> Just this patch or the entire series?
>>
>> No, actually i started looking at the second one and wondered why
>> we are using #include end/start-packed.h
>>
>> It is mentioned in start-packed.h it's because "its not possible to put
>> pragmas into header files"
> I think instead of
>
> /* Ideally this should all have been macros in a common headers, but
>   * its not possible to put pragmas into header files, so we have
>   * to use include magic.
>
> should be
>
> /* Ideally this should all have been macros in a common headers, but
>   * its not possible to put pragmas into MACROS, so we have
>   * to use include magic.
>
> and with C99 you can use _Pragma instead, but for coherence this
> method is still fine.
>
>> and just after that we put pragma, into this header file.
>> What am i missing? or the comment is wrong?
>>
> Header code is fine, comment is surely misleading.
>
>>>> Snir.
>>>>
>>>>
>>>> On 7/4/19 4:56 PM, Frediano Ziglio wrote:
>>>>> Do not declare the structure as aligned.
>>>>> The start/end-packed.h headers affects structures without
>>>>> specification only using MingW or Microsoft compilers. For other
>>>>> platform SPICE_ATTR_PACKED macro should be used.  This way the
>>>>> definition are the same for all compiler.
>>>>> This structure is used in a lot of QXL structures which are not
>>>>> aligned causing to have an aligned structure to be potentially
>>>>> unaligned.
>>> What about changing this paragraph to:
>>>
>>> "This structure is used in a lot of QXL structures which are not
>>>    aligned causing to have an aligned structure to be potentially
>>>    unaligned. Some compilers report a warning for some usage."
>>
>> Not a big deal but maybe "Some compilers may report a warning"?
>>
>> Anyway, ack for the change.
>>
> Thanks.
>
> Just this patch or also the other one?


Yes, if not pushed yet, I'd also squash, but not necessary
anyway ack for both (squshed or not)

Snir.


>
>>>>> As this structure has no holes this change does not make any size
>>>>> change using any compiler.
>>>>> The change will only change the alignment from 4/8 to 1.
>>>>> This could affect structures containing this union however beside
>>>>> packed structure in qxl_dev.h (which are not affected) there are no
>>>>> other usages affecting ABI by spice-gtk, Qemu or spice-server.
>>>>>
>>>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>>>> ---
>>>>> Changes since v1:
>>>>> - update commit message
>>>>> ---
>>>>>     spice/qxl_dev.h | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/spice/qxl_dev.h b/spice/qxl_dev.h
>>>>> index a9cc4f4..659f930 100644
>>>>> --- a/spice/qxl_dev.h
>>>>> +++ b/spice/qxl_dev.h
>>>>> @@ -275,7 +275,7 @@ typedef struct SPICE_ATTR_ALIGNED(4)
>>>>> SPICE_ATTR_PACKED
>>>>> QXLRam {
>>>>>     
>>>>>     } QXLRam;
>>>>>     
>>>>> -typedef union QXLReleaseInfo {
>>>>> +typedef union SPICE_ATTR_PACKED QXLReleaseInfo {
>>>>>         uint64_t id;      // in
>>>>>         uint64_t next;    // out
>>>>>     } QXLReleaseInfo;
>>> Frediano