Re: [Qemu-devel] [PATCH 11/14] hw/vfio/ccw: avoid taking address members in packed structs

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 2 Apr 2019 17:11:29 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Apr 02, 2019 at 06:00:33PM +0200, Cornelia Huck wrote:
> > On Fri, 29 Mar 2019 11:11:01 +0000
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > The GCC 9 compiler complains about many places in s390 code
> > > that take the address of members of the 'struct SCHIB' which
> > > is marked packed:
> > > 
> > > hw/vfio/ccw.c: In function ‘vfio_ccw_io_notifier_handler’:
> > > hw/vfio/ccw.c:133:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer value \
> > > [-Waddress-of-packed-member]
> > >   133 |     SCSW *s = &sch->curr_status.scsw;
> > >       |               ^~~~~~~~~~~~~~~~~~~~~~
> > > hw/vfio/ccw.c:134:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer value \
> > > [-Waddress-of-packed-member]
> > >   134 |     PMCW *p = &sch->curr_status.pmcw;
> > >       |               ^~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > ...snip many more...
> > > 
> > > Almost all of these are just done for convenience to avoid
> > > typing out long variable/field names when referencing struct
> > > members. We can get most of this convenience by taking the
> > > address of the 'struct SCHIB' instead, avoiding triggering
> > > the compiler warnings.
> > > 
> > > In a couple of places we copy via a local variable which is
> > > a technique already applied elsewhere in s390 code for this
> > > problem.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
> > >  1 file changed, 22 insertions(+), 20 deletions(-)  
> > 
> > I'm currently in the process of queuing this and the other three s390x
> > fixes, but I'm inclined to do so for 4.1 (it feels a bit late in the
> > cycle for 4.0.)
> > 
> > Other opinions?  
> 
> It would be nice to be warning free for 4.0, but I agree that it feels
> kind of late to be making these changes. They're not fixing real world
> bugs, and even if you queue the s390 bits we're unlikely to get all the
> others merged, especially the usb-mtp one is a nasty mess. So we'll
> not be 100% warning free.

Yeah, but OTOH, the s390x changes are straightforward and have been
reviewed by several people.

So I changed my mind and queued them to s390-fixes :)