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

[Frediano Ziglio]

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