Re: [libvirt] [PATCH 1/2] nwfilter: Fix pointer.

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.