[spice-protocol,1/2,v2] qxl_dev: Fix alignment for QXLReleaseInfo

Submitted by Frediano Ziglio on July 4, 2019, 1:56 p.m.

Details

Message ID 20190704135610.15771-1-fziglio@redhat.com
State Accepted
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 4, 2019, 1:56 p.m.
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.
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@redhat.com>
---
Changes since v1:
- update commit message
---
 spice/qxl_dev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

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;

Comments

ping series

> 
> 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.
> 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@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;
Hi,


IIRC this was related to some compiler warning, no?
If it is I'd mentioning it , otherwise, ack.

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.
> 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@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;
> 
> 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?

> 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."

> > 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@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
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"
and just after that we put pragma, into this header file.
What am i missing? or the comment is wrong?


>
>> 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.

>
>>> 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@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
> 
> 
> 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?

> >
> >>> 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@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
>
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@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