| Message ID | 20190122181822.1505-3-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | slirp: fix -Waddress-of-packed-member warnings | expand |
On Tue, Jan 22, 2019 at 06:18:22PM +0000, Peter Maydell wrote: > There is no reason to mark the struct ipq and struct ipasfrag as > packed: they are naturally aligned anyway, and are not representing > any on-the-wire packet format. Indeed they vary in size depending on > the size of pointers on the host system, because the 'struct qlink' > members include 'void *' fields. > > Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member > warnings and probably lets the compiler generate better code too. > > The only thing we do care about in the layout of the struct is > that the frag_link matches up with the ipf_link of the struct > ipasfrag, as documented in the comment on that struct; assert > at build time that this is the case. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > slirp/ip.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/slirp/ip.h b/slirp/ip.h > index 243b6c8b249..20614f3b53e 100644 > --- a/slirp/ip.h > +++ b/slirp/ip.h Just here there's a misleading comment * size 28 bytes that should be purged > @@ -217,7 +217,7 @@ struct ipq { > uint8_t ipq_p; /* protocol of this fragment */ > uint16_t ipq_id; /* sequence id for reassembly */ > struct in_addr ipq_src,ipq_dst; > -} QEMU_PACKED; > +}; > > /* > * Ip header, when holding a fragment. > @@ -227,7 +227,10 @@ struct ipq { > struct ipasfrag { > struct qlink ipf_link; > struct ip ipf_ip; > -} QEMU_PACKED; > +}; > + > +QEMU_BUILD_BUG_ON(offsetof(struct ipq, frag_link) != > + offsetof(struct ipasfrag, ipf_link)); > > #define ipf_off ipf_ip.ip_off > #define ipf_tos ipf_ip.ip_tos 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 :|
On Tue, 22 Jan 2019 at 18:55, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Jan 22, 2019 at 06:18:22PM +0000, Peter Maydell wrote: > > There is no reason to mark the struct ipq and struct ipasfrag as > > packed: they are naturally aligned anyway, and are not representing > > any on-the-wire packet format. Indeed they vary in size depending on > > the size of pointers on the host system, because the 'struct qlink' > > members include 'void *' fields. > > > > Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member > > warnings and probably lets the compiler generate better code too. > > > > The only thing we do care about in the layout of the struct is > > that the frag_link matches up with the ipf_link of the struct > > ipasfrag, as documented in the comment on that struct; assert > > at build time that this is the case. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > slirp/ip.h | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/slirp/ip.h b/slirp/ip.h > > index 243b6c8b249..20614f3b53e 100644 > > --- a/slirp/ip.h > > +++ b/slirp/ip.h > > Just here there's a misleading comment > > * size 28 bytes > > that should be purged I noticed that one but left it alone on the basis that I wasn't sure what to replace it with, and as it is it provides the useful information of "somebody at some point thought the size of this struct mattered but they were badly confused, so tread with caution" :-) thanks -- PMM
On 1/22/19 12:18 PM, Peter Maydell wrote: > There is no reason to mark the struct ipq and struct ipasfrag as > packed: they are naturally aligned anyway, and are not representing > any on-the-wire packet format. Indeed they vary in size depending on > the size of pointers on the host system, because the 'struct qlink' > members include 'void *' fields. > > Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member > warnings and probably lets the compiler generate better code too. > > The only thing we do care about in the layout of the struct is > that the frag_link matches up with the ipf_link of the struct > ipasfrag, as documented in the comment on that struct; assert > at build time that this is the case. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > slirp/ip.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
diff --git a/slirp/ip.h b/slirp/ip.h index 243b6c8b249..20614f3b53e 100644 --- a/slirp/ip.h +++ b/slirp/ip.h @@ -217,7 +217,7 @@ struct ipq { uint8_t ipq_p; /* protocol of this fragment */ uint16_t ipq_id; /* sequence id for reassembly */ struct in_addr ipq_src,ipq_dst; -} QEMU_PACKED; +}; /* * Ip header, when holding a fragment. @@ -227,7 +227,10 @@ struct ipq { struct ipasfrag { struct qlink ipf_link; struct ip ipf_ip; -} QEMU_PACKED; +}; + +QEMU_BUILD_BUG_ON(offsetof(struct ipq, frag_link) != + offsetof(struct ipasfrag, ipf_link)); #define ipf_off ipf_ip.ip_off #define ipf_tos ipf_ip.ip_tos
There is no reason to mark the struct ipq and struct ipasfrag as packed: they are naturally aligned anyway, and are not representing any on-the-wire packet format. Indeed they vary in size depending on the size of pointers on the host system, because the 'struct qlink' members include 'void *' fields. Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member warnings and probably lets the compiler generate better code too. The only thing we do care about in the layout of the struct is that the frag_link matches up with the ipf_link of the struct ipasfrag, as documented in the comment on that struct; assert at build time that this is the case. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- slirp/ip.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) -- 2.20.1