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. Rich.
int ifindex = 0; int errcount = 0; int tmp = -1, rv, n, pollTo; @@ -1453,7 +1453,7 @@ virNWFilterDHCPSnoopThread(void *req0) n--;
rv = pcap_next_ex(pcapConf[i].handle, &hdr, - (const u_char **)&packet); + (const u_char **)&packetPtr);
And then into a pointer to a pointer to a pointer.
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 :|
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v