On Mon, Jan 21, 2019 at 03:41:06PM +0000, Richard W.M. Jones wrote:
> On Mon, Jan 21, 2019 at 03:23:59PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Jan 21, 2019 at 03:13:20PM +0000, Richard W.M. Jones wrote:
> > > GCC 9 complains:
> > >
> > > nwfilter/nwfilter_dhcpsnoop.c: In function 'virNWFilterDHCPSnoopThread':
> > > nwfilter/nwfilter_dhcpsnoop.c:1456:31: error: converting a packed 'virNWFilterSnoopEthHdrPtr' {aka 'struct _virNWFilterSnoopEthHdr *'} pointer (alignment 1) to 'const u_char *' {aka 'const unsigned char *'} (alignment 8) may result in an unaligned pointer value [-Werror=address-of-packed-member]
> > > 1456 | (const u_char **)&packet);
> > > | ^
> >
> > I tend to think this warning is bogus. We are not de-referencing
> > any packed fields within the sctruct when we call to pcap_next_ex()
> > with the cast. pcap_next_ex() is just going to fill the entire
> > memory region with a read off the wire, so it would not be triggering
> > unaligned access either. IOW, I don't think the compiler should be
> > warning there
> >
> > IIUC gcc X.0.0 versions are not in fact relases, but rather
> > pre-release snapshots.
> >
> > If so, I think this might be a bug that needs reporting against
> > the GCC pre-release.
> >
> >
> >
> > > nwfilter/nwfilter_dhcpsnoop.c:183:8: note: defined here
> > > 183 | struct _virNWFilterSnoopEthHdr {
> > > | ^~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > However it seems like there's more going on here than just an enhanced
> > > GCC warning. The function pcap_next_ex is documented as:
> > >
> > > the pointer pointed to by the
> > > pkt_data argument is set to point to the data in the packet
> > >
> > > We are passing a struct here rather than a pointer. I changed the
> > > code to pass a pointer instead.
> >
> > > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> > > index 58f0057c3f..45873a542c 100644
> > > --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> > > +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> > > @@ -1335,7 +1335,7 @@ virNWFilterDHCPSnoopThread(void *req0)
> > > {
> > > virNWFilterSnoopReqPtr req = req0;
> > > struct pcap_pkthdr *hdr;
> > > - virNWFilterSnoopEthHdrPtr packet;
> > > + const virNWFilterSnoopEthHdrPtr *packetPtr;
> >
> > virNWFilterSnoopEthHdrPtr is already a pointer to a virNWFilterSnoopEthHdr.
> >
> > So this change turns it into a pointer to a pointer....
>
> Duh you're right there.
>
> Yup as you say this patch is bogus and more likely indicates
> some bug in GCC.
It was indeed a bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88664
So we don't need to fix this in libvirt.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
