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
@@ -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
-- 
2.20.1

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