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

[Frediano Ziglio]

> 
> On 5/13/19 12:45 PM, Frediano Ziglio wrote:
> > Do not declare the structure as aligned.
> > The start/end-packed.h headers affects only MingW or Microsoft
> > compilers.
> 
> They also define/undef SPICE_ATTR_PACKED which affects Linux.
>

May be:

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.
 
> BTW, if we define PACKED as ALIGNED(1) does that work for Windows
> too (I mean instead of pragma pack) ?
> 

More than BTW looks OT to me. No, aligned is used to specify that
the total structure is aligned while pack/packed affects all fields.
Actually I thing that pragma pack(push ,1) for compatibility should
work on all compilers we support.

> > To have unaligned structure with GCC compiler you have
> > to use SPICE_ATTR_PACKED. 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.
> 
> For this reason, this change looks safe to me.
> 

It depends, if a structure that defines an ABI is like

struct foo {
   int x;
   QXLReleaseInfo release_info;
}

this patch would change ABI.

> > 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 as such by spice-gtk, Qemu or spice-server.
> 
> It is used in Qemu, in hw/display/qxl.h, struct PCIQXLDevice.
> Also it's used in spice-server and likely QXL drivers too.
> 

Yes, I was talking about structures which have ABI requirements,
so not internal Qemu ones.

Maybe:

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.

> Uri.
> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >   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