Re: [libvirt] [PATCH 5/7] Disable cast-align warnings in various places

On 04/03/2013 09:06 AM, Daniel P. Berrange wrote: Sheesh - this is some hairy code. nwf is type virNWFilterRuleDefPtr, and att[idx].dataIdx is populated to be the offset of a field within virNWFilterRuleDef. I looked for all instances of dataIdx initializations, and it loos like all of those fields are indeed typed nwItemDesc, so the cast is aligned, and I can agree to the use of a pragma instead of rewriting this. Same analysis. names is strcut dm_names*, and libdevmapper (stupidly) set up next to be a byte offset instead of a struct offset. Serves them right if they botched alignment, but I agree that we are stuck here using the pragma. pep is virNWFilterSnoopEthHdrPtr, which is ATTRIBUTE_PACKED. eh_data within that struct is therefore aligned only to uint16_t boundaries. 'struct iphdr' in <netinet/ip.h> requires uint32_t alignment. This is a real case of misalignment. I'd rather see us work around this with memcpy() than risk SIGBUS if we read a 32-bit entity that crosses a word boundary due to 16-bit alignment. pip is struct iphdr *, which I already said was 32-bit aligned; and the offset (pip->ihl << 2) is guaranteed to be a multiple of 32-bit alignment. Furthermore, struct udphdr appears to be only 16-bit aligned (the warning about alignment increase is due to the intermediate use of 8-bit char*). This one is safe for use of the pragma. dhcpopt is declared as struct dhcp_option, which is only 8-bit aligned (it has a pointless ATTRIBUTE_PACKED, given that none of its fields are larger than char). But dhcpopt is created as uint16_t away from the argument dhcp; and the only caller is at line 562, where dhcp was computed based on a udphdr. Sounds like it is an alignment problem after all, and I'd rather see memcpy() used to avoid the possibility of a SIGBUS. packet is merely a byte array with no alignment (in reality, it's probably at least uint16_t aligned, and maybe we get lucky with uint32_t alignment; but I don't know that pcap_next() guarantees that). Here,... ...and in these two as well, we are progressively looking further into that raw array of packet, and trying to parse out possibly unaligned data. I have no idea how we could possibly guarantee alignment, so the pragma may be the best we can do without writing the code to access unaligned offsets a byte at a time instead of as a struct field dereference. Let's face it - trying to parse raw packets off a NIC is painful when you can't use unaligned access. Ultimately, I think this file would be better if we used accessors from virendian.h (well, we'd have to add virReadBufInt16BE/virReadBufInt16LE) to read integers from byte[]+offset, (the virendian.h macros are safe regardless of alignment) instead of trying to cast through a packed type and risking problems. But doing a rewrite like that is a much bigger task; so what you have at least shuts the compiler up, even if it doesn't fix the issues at hand. Good. Good (see my comments above). Good. Good. Tolerable, although I'd prefer a memcpy or virendian.h solution for whatever we end up reading out of pip. Good. Tolerable. Tolerable. Tolerable. Tolerable. Tolerable. No choice in the matter; good. ACK. Although I wouldn't mind if you attempt a v2 to address some of the more questionable locations, I also won't insist on it right now. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org