[Qemu-devel] [PATCH 00/12] add HOST_SUPPORTS_UNALIGNED_ACCESS, reduce slirp warnings

[Qemu-devel] [PATCH 00/12] add HOST_SUPPORTS_UNALIGNED_ACCESS, reduce slirp warnings

[Philippe Mathieu-Daudé]

Hi,

A recent mail from Michael remembered me this pre-2.10 series eh :)

I tagged the last patch 'RFC' because I find it ugly and hope there is a nicer
way to write it.

Regards,

Phil.

Philippe Mathieu-Daudé (12):
  slirp: remove QEMU_PACKED from structures with don't require it
  slirp: struct icmp/ethhdr ARE packed
  slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero()
  slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()
  slirp: poison IN6_*_ADDR_*() macros to avoid them
  slirp: remove unused header
  slirp: remove unnecessary
  slirp: removed unused code
  slirp: add in6_dhcp_multicast()
  configure: disable unaligned access warning on x86 arch
  configure: add HOST_SUPPORTS_UNALIGNED_ACCESS
  slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS

 configure         | 18 ++++++++++++
 slirp/dhcpv6.h    |  3 ++
 slirp/ip.h        | 23 ++++------------
 slirp/ip6.h       | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 slirp/ip6_icmp.h  |  6 ++--
 slirp/ip_icmp.h   |  2 +-
 slirp/libslirp.h  |  1 -
 slirp/slirp.h     |  5 ++--
 slirp/ip6_icmp.c  | 16 +++++------
 slirp/ndp_table.c |  6 ++--
 slirp/udp6.c      |  2 +-
 11 files changed, 126 insertions(+), 38 deletions(-)

-- 
2.15.1

[Qemu-devel] [PATCH 01/12] slirp: remove QEMU_PACKED from structures with don't require it

[Philippe Mathieu-Daudé]

theses structures are not serialized and often store host pointers.

  slirp/ip_input.c:159:43: warning: taking address of packed member 'ip_link' of class or
        structure 'ipq' may result in an unaligned pointer value
        [-Waddress-of-packed-member]
                  for (l = slirp->ipq.ip_link.next; l != &slirp->ipq.ip_link;
                                                          ^~~~~~~~~~~~~~~~~~

Reported-by: John Arbuckle <programmingkidx@gmail.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip.h    | 10 +++++-----
 slirp/slirp.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/slirp/ip.h b/slirp/ip.h
index 1df6723357..e29ccd3c9f 100644
--- a/slirp/ip.h
+++ b/slirp/ip.h
@@ -179,11 +179,11 @@ struct	ip_timestamp {
 struct mbuf_ptr {
 	struct mbuf *mptr;
 	uint32_t dummy;
-} QEMU_PACKED;
+};
 #else
 struct mbuf_ptr {
 	struct mbuf *mptr;
-} QEMU_PACKED;
+};
 #endif
 struct qlink {
 	void *next, *prev;
@@ -199,7 +199,7 @@ struct ipovly {
 	uint16_t	ih_len;			/* protocol length */
 	struct	in_addr ih_src;		/* source internet address */
 	struct	in_addr ih_dst;		/* destination internet address */
-} QEMU_PACKED;
+};
 
 /*
  * Ip reassembly queue structure.  Each fragment
@@ -215,7 +215,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.
@@ -225,7 +225,7 @@ struct ipq {
 struct	ipasfrag {
 	struct qlink ipf_link;
 	struct ip ipf_ip;
-} QEMU_PACKED;
+};
 
 #define ipf_off      ipf_ip.ip_off
 #define ipf_tos      ipf_ip.ip_tos
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 898ec9516d..9f29b52610 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -136,7 +136,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
 struct ndpentry {
     unsigned char   eth_addr[ETH_ALEN];     /* sender hardware address */
     struct in6_addr ip_addr;                /* sender IP address       */
-} QEMU_PACKED;
+};
 
 #define NDP_TABLE_SIZE 16
 
-- 
2.15.1

[Qemu-devel] [PATCH 02/12] slirp: struct icmp/ethhdr ARE packed

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip6_icmp.h | 6 +++---
 slirp/ip_icmp.h  | 2 +-
 slirp/slirp.h    | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/slirp/ip6_icmp.h b/slirp/ip6_icmp.h
index b3378b17b5..a4ccc69974 100644
--- a/slirp/ip6_icmp.h
+++ b/slirp/ip6_icmp.h
@@ -17,20 +17,20 @@
 struct icmp6_echo { /* Echo Messages */
     uint16_t id;
     uint16_t seq_num;
-};
+} QEMU_PACKED;
 
 union icmp6_error_body {
     uint32_t unused;
     uint32_t pointer;
     uint32_t mtu;
-};
+} QEMU_PACKED;
 
 /*
  * NDP Messages
  */
 struct ndp_rs {     /* Router Solicitation Message */
     uint32_t reserved;
-};
+} QEMU_PACKED;
 
 struct ndp_ra {     /* Router Advertisement Message */
     uint8_t chl;    /* Cur Hop Limit */
diff --git a/slirp/ip_icmp.h b/slirp/ip_icmp.h
index d88ab34c1b..e53242bdd8 100644
--- a/slirp/ip_icmp.h
+++ b/slirp/ip_icmp.h
@@ -88,7 +88,7 @@ struct icmp {
 #define	icmp_ip		icmp_dun.id_ip.idi_ip
 #define	icmp_mask	icmp_dun.id_mask
 #define	icmp_data	icmp_dun.id_data
-};
+} QEMU_PACKED;
 
 /*
  * Lower bounds on packet lengths for various types.
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 9f29b52610..9a7287e7cc 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -103,7 +103,7 @@ struct ethhdr {
     unsigned char  h_dest[ETH_ALEN];   /* destination eth addr */
     unsigned char  h_source[ETH_ALEN]; /* source ether addr    */
     unsigned short h_proto;            /* packet type ID field */
-};
+} QEMU_PACKED;
 
 struct slirp_arphdr {
     unsigned short ar_hrd;      /* format of hardware address */
-- 
2.15.1

[Qemu-devel] [PATCH 03/12] slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero()

[Philippe Mathieu-Daudé]

Host: Mac OS 10.12.5
Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)

  slirp/ip6_icmp.c:80:38: warning: taking address of packed member 'ip_src' of class or
        structure 'ip6' may result in an unaligned pointer value
        [-Waddress-of-packed-member]
              IN6_IS_ADDR_UNSPECIFIED(&ip->ip_src)) {
                                       ^~~~~~~~~~
  /usr/include/netinet6/in6.h:238:42: note: expanded from macro 'IN6_IS_ADDR_UNSPECIFIED'
          ((*(const __uint32_t *)(const void *)(&(a)->s6_addr[0]) == 0) && \
                                                  ^

Reported-by: John Arbuckle <programmingkidx@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip6_icmp.c  | 6 +++---
 slirp/ndp_table.c | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 777eb574be..ee333d05a2 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -77,7 +77,7 @@ void icmp6_send_error(struct mbuf *m, uint8_t type, uint8_t code)
     DEBUG_ARGS((dfd, " type = %d, code = %d\n", type, code));
 
     if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
-            IN6_IS_ADDR_UNSPECIFIED(&ip->ip_src)) {
+            in6_zero(&ip->ip_src)) {
         /* TODO icmp error? */
         return;
     }
@@ -272,7 +272,7 @@ static void ndp_send_na(Slirp *slirp, struct ip6 *ip, struct icmp6 *icmp)
     struct mbuf *t = m_get(slirp);
     struct ip6 *rip = mtod(t, struct ip6 *);
     rip->ip_src = icmp->icmp6_nns.target;
-    if (IN6_IS_ADDR_UNSPECIFIED(&ip->ip_src)) {
+    if (in6_zero(&ip->ip_src)) {
         rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
     } else {
         rip->ip_dst = ip->ip_src;
@@ -350,7 +350,7 @@ static void ndp_input(struct mbuf *m, Slirp *slirp, struct ip6 *ip,
                 && icmp->icmp6_code == 0
                 && !IN6_IS_ADDR_MULTICAST(&icmp->icmp6_nns.target)
                 && ntohs(ip->ip_pl) >= ICMP6_NDP_NS_MINLEN
-                && (!IN6_IS_ADDR_UNSPECIFIED(&ip->ip_src)
+                && (!in6_zero(&ip->ip_src)
                     || in6_solicitednode_multicast(&ip->ip_dst))) {
             if (in6_equal_host(&icmp->icmp6_nns.target)) {
                 /* Gratuitous NDP */
diff --git a/slirp/ndp_table.c b/slirp/ndp_table.c
index 9d4c39b45c..e1676a0a7b 100644
--- a/slirp/ndp_table.c
+++ b/slirp/ndp_table.c
@@ -23,7 +23,7 @@ void ndp_table_add(Slirp *slirp, struct in6_addr ip_addr,
                 ethaddr[0], ethaddr[1], ethaddr[2],
                 ethaddr[3], ethaddr[4], ethaddr[5]));
 
-    if (IN6_IS_ADDR_MULTICAST(&ip_addr) || IN6_IS_ADDR_UNSPECIFIED(&ip_addr)) {
+    if (IN6_IS_ADDR_MULTICAST(&ip_addr) || in6_zero(&ip_addr)) {
         /* Do not register multicast or unspecified addresses */
         DEBUG_CALL(" abort: do not register multicast or unspecified address");
         return;
@@ -60,7 +60,7 @@ bool ndp_table_search(Slirp *slirp, struct in6_addr ip_addr,
     DEBUG_ARG("ip = %s", addrstr);
 #endif
 
-    assert(!IN6_IS_ADDR_UNSPECIFIED(&ip_addr));
+    assert(!in6_zero(&ip_addr));
 
     /* Multicast address: fec0::abcd:efgh/8 -> 33:33:ab:cd:ef:gh */
     if (IN6_IS_ADDR_MULTICAST(&ip_addr)) {
-- 
2.15.1

[Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Philippe Mathieu-Daudé]

Host: Mac OS 10.12.5
Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)

  slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
        structure 'ip6' may result in an unaligned pointer value
        [-Waddress-of-packed-member]
      if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
                                 ^~~~~~~~~~
  /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
  #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
                                            ^

Reported-by: John Arbuckle <programmingkidx@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip6.h       |  5 +++++
 slirp/ip6_icmp.c  | 10 +++++-----
 slirp/ndp_table.c |  4 ++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/slirp/ip6.h b/slirp/ip6.h
index b1bea43b3c..6c5d4eeaa3 100644
--- a/slirp/ip6.h
+++ b/slirp/ip6.h
@@ -93,6 +93,11 @@ static inline bool in6_equal_mach(const struct in6_addr *a,
 #define in6_zero(a)\
     (in6_equal(a, &(struct in6_addr)ZERO_ADDR))
 
+static inline bool in6_multicast(const struct in6_addr *a)
+{
+    return a->s6_addr[0] == 0xff;
+}
+
 /* Compute emulated host MAC address from its ipv6 address */
 static inline void in6_compute_ethaddr(struct in6_addr ip,
                                        uint8_t eth[ETH_ALEN])
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index ee333d05a2..9796624648 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -76,7 +76,7 @@ void icmp6_send_error(struct mbuf *m, uint8_t type, uint8_t code)
     DEBUG_CALL("icmp6_send_error");
     DEBUG_ARGS((dfd, " type = %d, code = %d\n", type, code));
 
-    if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
+    if (in6_multicast(&ip->ip_src) ||
             in6_zero(&ip->ip_src)) {
         /* TODO icmp error? */
         return;
@@ -291,7 +291,7 @@ static void ndp_send_na(Slirp *slirp, struct ip6 *ip, struct icmp6 *icmp)
 
     /* NDP */
     ricmp->icmp6_nna.R = NDP_IsRouter;
-    ricmp->icmp6_nna.S = !IN6_IS_ADDR_MULTICAST(&rip->ip_dst);
+    ricmp->icmp6_nna.S = !in6_multicast(&rip->ip_dst);
     ricmp->icmp6_nna.O = 1;
     ricmp->icmp6_nna.reserved_hi = 0;
     ricmp->icmp6_nna.reserved_lo = 0;
@@ -348,7 +348,7 @@ static void ndp_input(struct mbuf *m, Slirp *slirp, struct ip6 *ip,
         DEBUG_CALL(" type = Neighbor Solicitation");
         if (ip->ip_hl == 255
                 && icmp->icmp6_code == 0
-                && !IN6_IS_ADDR_MULTICAST(&icmp->icmp6_nns.target)
+                && !in6_multicast(&icmp->icmp6_nns.target)
                 && ntohs(ip->ip_pl) >= ICMP6_NDP_NS_MINLEN
                 && (!in6_zero(&ip->ip_src)
                     || in6_solicitednode_multicast(&ip->ip_dst))) {
@@ -365,8 +365,8 @@ static void ndp_input(struct mbuf *m, Slirp *slirp, struct ip6 *ip,
         if (ip->ip_hl == 255
                 && icmp->icmp6_code == 0
                 && ntohs(ip->ip_pl) >= ICMP6_NDP_NA_MINLEN
-                && !IN6_IS_ADDR_MULTICAST(&icmp->icmp6_nna.target)
-                && (!IN6_IS_ADDR_MULTICAST(&ip->ip_dst)
+                && !in6_multicast(&icmp->icmp6_nna.target)
+                && (!in6_multicast(&ip->ip_dst)
                     || icmp->icmp6_nna.S == 0)) {
             ndp_table_add(slirp, ip->ip_src, eth->h_source);
         }
diff --git a/slirp/ndp_table.c b/slirp/ndp_table.c
index e1676a0a7b..5ff4bf2f3d 100644
--- a/slirp/ndp_table.c
+++ b/slirp/ndp_table.c
@@ -23,7 +23,7 @@ void ndp_table_add(Slirp *slirp, struct in6_addr ip_addr,
                 ethaddr[0], ethaddr[1], ethaddr[2],
                 ethaddr[3], ethaddr[4], ethaddr[5]));
 
-    if (IN6_IS_ADDR_MULTICAST(&ip_addr) || in6_zero(&ip_addr)) {
+    if (in6_multicast(&ip_addr) || in6_zero(&ip_addr)) {
         /* Do not register multicast or unspecified addresses */
         DEBUG_CALL(" abort: do not register multicast or unspecified address");
         return;
@@ -63,7 +63,7 @@ bool ndp_table_search(Slirp *slirp, struct in6_addr ip_addr,
     assert(!in6_zero(&ip_addr));
 
     /* Multicast address: fec0::abcd:efgh/8 -> 33:33:ab:cd:ef:gh */
-    if (IN6_IS_ADDR_MULTICAST(&ip_addr)) {
+    if (in6_multicast(&ip_addr)) {
         out_ethaddr[0] = 0x33; out_ethaddr[1] = 0x33;
         out_ethaddr[2] = ip_addr.s6_addr[12];
         out_ethaddr[3] = ip_addr.s6_addr[13];
-- 
2.15.1

[Qemu-devel] [PATCH 05/12] slirp: poison IN6_*_ADDR_*() macros to avoid them

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip6.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/slirp/ip6.h b/slirp/ip6.h
index 6c5d4eeaa3..c6493a0856 100644
--- a/slirp/ip6.h
+++ b/slirp/ip6.h
@@ -8,6 +8,25 @@
 
 #include "net/eth.h"
 
+#ifdef __GNUC__
+# undef IN6_IS_ADDR_UNSPECIFIED
+# pragma GCC poison IN6_IS_ADDR_UNSPECIFIED
+# undef IN6_IS_ADDR_LOOPBACK
+# pragma GCC poison IN6_IS_ADDR_LOOPBACK
+# undef IN6_IS_ADDR_LINKLOCAL
+# pragma GCC poison IN6_IS_ADDR_LINKLOCAL
+# undef IN6_IS_ADDR_SITELOCAL
+# pragma GCC poison IN6_IS_ADDR_SITELOCAL
+# undef IN6_IS_ADDR_V4MAPPED
+# pragma GCC poison IN6_IS_ADDR_V4MAPPED
+# undef IN6_IS_ADDR_V4COMPAT
+# pragma GCC poison IN6_IS_ADDR_V4COMPAT
+# undef IN6_ARE_ADDR_EQUAL
+# pragma GCC poison IN6_ARE_ADDR_EQUAL
+# undef IN6_IS_ADDR_MULTICAST
+# pragma GCC poison IN6_IS_ADDR_MULTICAST
+#endif
+
 #define ALLNODES_MULTICAST  { .s6_addr = \
                             { 0xff, 0x02, 0x00, 0x00,\
                             0x00, 0x00, 0x00, 0x00,\
-- 
2.15.1

[Qemu-devel] [PATCH 06/12] slirp: remove unused header

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/slirp.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/slirp/slirp.h b/slirp/slirp.h
index 9a7287e7cc..447dc045a8 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -1,7 +1,6 @@
 #ifndef SLIRP_H
 #define SLIRP_H
 
-#include "qemu/host-utils.h"
 #include "slirp_config.h"
 
 #ifdef _WIN32
-- 
2.15.1

[Qemu-devel] [PATCH 07/12] slirp: remove unnecessary

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/libslirp.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/slirp/libslirp.h b/slirp/libslirp.h
index f90f0f524c..540b3e5903 100644
--- a/slirp/libslirp.h
+++ b/slirp/libslirp.h
@@ -3,7 +3,6 @@
 
 #include "qemu-common.h"
 
-struct Slirp;
 typedef struct Slirp Slirp;
 
 int get_dns_addr(struct in_addr *pdns_addr);
-- 
2.15.1

[Qemu-devel] [PATCH 08/12] slirp: removed unused code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/ip.h | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/slirp/ip.h b/slirp/ip.h
index e29ccd3c9f..71c3642cfe 100644
--- a/slirp/ip.h
+++ b/slirp/ip.h
@@ -233,17 +233,4 @@ struct	ipasfrag {
 #define ipf_next     ipf_link.next
 #define ipf_prev     ipf_link.prev
 
-/*
- * Structure stored in mbuf in inpcb.ip_options
- * and passed to ip_output when ip options are in use.
- * The actual length of the options (including ipopt_dst)
- * is in m_len.
- */
-#define MAX_IPOPTLEN	40
-
-struct ipoption {
-	struct	in_addr ipopt_dst;	/* first-hop dst if source routed */
-	int8_t	ipopt_list[MAX_IPOPTLEN];	/* options proper */
-} QEMU_PACKED;
-
 #endif
-- 
2.15.1

[Qemu-devel] [PATCH 09/12] slirp: add in6_dhcp_multicast()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 slirp/dhcpv6.h | 3 +++
 slirp/udp6.c   | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/slirp/dhcpv6.h b/slirp/dhcpv6.h
index 9189cd3f2d..3373f6cb89 100644
--- a/slirp/dhcpv6.h
+++ b/slirp/dhcpv6.h
@@ -17,6 +17,9 @@
                             0x00, 0x00, 0x00, 0x00,\
                             0x00, 0x01, 0x00, 0x02 } }
 
+#define in6_dhcp_multicast(a)\
+    in6_equal(a, &(struct in6_addr)ALLDHCP_MULTICAST)
+
 void dhcpv6_input(struct sockaddr_in6 *srcsas, struct mbuf *m);
 
 #endif
diff --git a/slirp/udp6.c b/slirp/udp6.c
index 9fa314bc2d..7c4a6b003a 100644
--- a/slirp/udp6.c
+++ b/slirp/udp6.c
@@ -65,7 +65,7 @@ void udp6_input(struct mbuf *m)
     /* handle DHCPv6 */
     if (ntohs(uh->uh_dport) == DHCPV6_SERVER_PORT &&
         (in6_equal(&ip->ip_dst, &slirp->vhost_addr6) ||
-         in6_equal(&ip->ip_dst, &(struct in6_addr)ALLDHCP_MULTICAST))) {
+         in6_dhcp_multicast(&ip->ip_dst))) {
         m->m_data += iphlen;
         m->m_len -= iphlen;
         dhcpv6_input(&lhost, m);
-- 
2.15.1

[Qemu-devel] [PATCH 10/12] configure: disable unaligned access warning on x86 arch

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 configure | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/configure b/configure
index 56f9716609..28cd073941 100755
--- a/configure
+++ b/configure
@@ -276,6 +276,7 @@ cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
 stack_protector=""
+host_supports_unaligned_access="no"
 
 if test -e "$source_path/.git"
 then
@@ -692,6 +693,13 @@ if test -z "$ARCH"; then
   ARCH="$cpu"
 fi
 
+# ARCH specific
+case "$ARCH" in
+  i386|x86_64)
+    host_supports_unaligned_access="yes"
+  ;;
+esac
+
 # OS specific
 
 # host *BSD for user mode
@@ -4993,6 +5001,15 @@ if test "$fortify_source" != "no"; then
   fi
 fi
 
+#################################################
+# check if host supports unaligned access
+
+if test "$host_supports_unaligned_access" != "no"; then
+  if compile_prog "-Werror -Wno-address-of-packed-member" "" ; then
+    QEMU_CFLAGS="-Wno-address-of-packed-member $QEMU_CFLAGS"
+  fi
+fi
+
 ##########################################
 # check if struct fsxattr is available via linux/fs.h
 
-- 
2.15.1

[Qemu-devel] [PATCH 11/12] configure: add HOST_SUPPORTS_UNALIGNED_ACCESS

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 28cd073941..669e5a041a 100755
--- a/configure
+++ b/configure
@@ -5005,6 +5005,7 @@ fi
 # check if host supports unaligned access
 
 if test "$host_supports_unaligned_access" != "no"; then
+  QEMU_CFLAGS="-DHOST_SUPPORTS_UNALIGNED_ACCESS $QEMU_CFLAGS"
   if compile_prog "-Werror -Wno-address-of-packed-member" "" ; then
     QEMU_CFLAGS="-Wno-address-of-packed-member $QEMU_CFLAGS"
   fi
-- 
2.15.1

[Qemu-devel] [RFC PATCH 12/12] slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS

[Philippe Mathieu-Daudé]

Access struct in6_addr with 'void *', then cast to 'u8 *' to avoid alignment
issues.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
ugly...

 slirp/ip6.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/slirp/ip6.h b/slirp/ip6.h
index c6493a0856..4eaeaa9a0a 100644
--- a/slirp/ip6.h
+++ b/slirp/ip6.h
@@ -51,11 +51,19 @@
                         0x00, 0x00, 0x00, 0x00,\
                         0x00, 0x00, 0x00, 0x00 } }
 
+#ifdef HOST_SUPPORTS_UNALIGNED_ACCESS
 static inline bool in6_equal(const struct in6_addr *a, const struct in6_addr *b)
 {
     return memcmp(a, b, sizeof(*a)) == 0;
 }
+#else
+static inline bool in6_equal(const void *a, const void *b)
+{
+    return memcmp(a, b, sizeof(struct in6_addr)) == 0;
+}
+#endif
 
+#ifdef HOST_SUPPORTS_UNALIGNED_ACCESS
 static inline bool in6_equal_net(const struct in6_addr *a,
                                  const struct in6_addr *b,
                                  int prefix_len)
@@ -71,7 +79,28 @@ static inline bool in6_equal_net(const struct in6_addr *a,
     return a->s6_addr[prefix_len / 8] >> (8 - (prefix_len % 8))
         == b->s6_addr[prefix_len / 8] >> (8 - (prefix_len % 8));
 }
+#else
+static inline bool in6_equal_net(const void *a,
+                                 const void *b,
+                                 int prefix_len)
+{
+    const uint8_t *aa = (uint8_t *)a;
+    const uint8_t *ab = (uint8_t *)b;
+
+    if (memcmp(a, b, prefix_len / 8) != 0) {
+        return 0;
+    }
+
+    if (prefix_len % 8 == 0) {
+        return 1;
+    }
+
+    return (aa[prefix_len / 8] >> (8 - (prefix_len % 8)))
+        == (ab[prefix_len / 8] >> (8 - (prefix_len % 8)));
+}
+#endif
 
+#ifdef HOST_SUPPORTS_UNALIGNED_ACCESS
 static inline bool in6_equal_mach(const struct in6_addr *a,
                                   const struct in6_addr *b,
                                   int prefix_len)
@@ -89,6 +118,28 @@ static inline bool in6_equal_mach(const struct in6_addr *a,
     return (a->s6_addr[prefix_len / 8] & ((1U << (8 - (prefix_len % 8))) - 1))
         == (b->s6_addr[prefix_len / 8] & ((1U << (8 - (prefix_len % 8))) - 1));
 }
+#else
+static inline bool in6_equal_mach(const void *a,
+                                  const void *b,
+                                  int prefix_len)
+{
+    const uint8_t *aa = (uint8_t *)a;
+    const uint8_t *ab = (uint8_t *)b;
+
+    if (memcmp(&(aa[DIV_ROUND_UP(prefix_len, 8)]),
+               &(ab[DIV_ROUND_UP(prefix_len, 8)]),
+               16 - DIV_ROUND_UP(prefix_len, 8)) != 0) {
+        return 0;
+    }
+
+    if (prefix_len % 8 == 0) {
+        return 1;
+    }
+
+    return (aa[prefix_len / 8] & ((1U << (8 - (prefix_len % 8))) - 1))
+        == (ab[prefix_len / 8] & ((1U << (8 - (prefix_len % 8))) - 1));
+}
+#endif
 
 
 #define in6_equal_router(a)\
@@ -112,10 +163,17 @@ static inline bool in6_equal_mach(const struct in6_addr *a,
 #define in6_zero(a)\
     (in6_equal(a, &(struct in6_addr)ZERO_ADDR))
 
+#ifdef HOST_SUPPORTS_UNALIGNED_ACCESS
 static inline bool in6_multicast(const struct in6_addr *a)
 {
     return a->s6_addr[0] == 0xff;
 }
+#else
+static inline bool in6_multicast(const void *a)
+{
+    return ((const uint8_t *)a)[0] == 0xff;
+}
+#endif
 
 /* Compute emulated host MAC address from its ipv6 address */
 static inline void in6_compute_ethaddr(struct in6_addr ip,
-- 
2.15.1

Re: [Qemu-devel] [PATCH 10/12] configure: disable unaligned access warning on x86 arch

[Peter Maydell]

On 8 January 2018 at 17:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  configure | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

This doesn't seem like the right approach to me. We
want this sort of thing to be a warning/error on x86,
because that's the host that everybody actually uses
to develop with. If they only show up on non-x86
hosts then the result will be a lot more bounced
pull requests because of only-non-x86 warnings.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/12] slirp: struct icmp/ethhdr ARE packed

[Thomas Huth]

On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/ip6_icmp.h | 6 +++---
>  slirp/ip_icmp.h  | 2 +-
>  slirp/slirp.h    | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/slirp/ip6_icmp.h b/slirp/ip6_icmp.h
> index b3378b17b5..a4ccc69974 100644
> --- a/slirp/ip6_icmp.h
> +++ b/slirp/ip6_icmp.h
> @@ -17,20 +17,20 @@
>  struct icmp6_echo { /* Echo Messages */
>      uint16_t id;
>      uint16_t seq_num;
> -};
> +} QEMU_PACKED;
>  
>  union icmp6_error_body {
>      uint32_t unused;
>      uint32_t pointer;
>      uint32_t mtu;
> -};
> +} QEMU_PACKED;
>  
>  /*
>   * NDP Messages
>   */
>  struct ndp_rs {     /* Router Solicitation Message */
>      uint32_t reserved;
> -};
> +} QEMU_PACKED;
...
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 9f29b52610..9a7287e7cc 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -103,7 +103,7 @@ struct ethhdr {
>      unsigned char  h_dest[ETH_ALEN];   /* destination eth addr */
>      unsigned char  h_source[ETH_ALEN]; /* source ether addr    */
>      unsigned short h_proto;            /* packet type ID field */
> -};
> +} QEMU_PACKED;
>  
>  struct slirp_arphdr {
>      unsigned short ar_hrd;      /* format of hardware address */
> 

The above structs all look naturally aligned to me - so why do you need
the QEMU_PACKED here?

 Thomas

Re: [Qemu-devel] [PATCH 05/12] slirp: poison IN6_*_ADDR_*() macros to avoid them

[Thomas Huth]

Please add a proper patch description here why you are doing this. At
least it is not clear to me at a first glance.

 Thomas


On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/ip6.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/slirp/ip6.h b/slirp/ip6.h
> index 6c5d4eeaa3..c6493a0856 100644
> --- a/slirp/ip6.h
> +++ b/slirp/ip6.h
> @@ -8,6 +8,25 @@
>  
>  #include "net/eth.h"
>  
> +#ifdef __GNUC__
> +# undef IN6_IS_ADDR_UNSPECIFIED
> +# pragma GCC poison IN6_IS_ADDR_UNSPECIFIED
> +# undef IN6_IS_ADDR_LOOPBACK
> +# pragma GCC poison IN6_IS_ADDR_LOOPBACK
> +# undef IN6_IS_ADDR_LINKLOCAL
> +# pragma GCC poison IN6_IS_ADDR_LINKLOCAL
> +# undef IN6_IS_ADDR_SITELOCAL
> +# pragma GCC poison IN6_IS_ADDR_SITELOCAL
> +# undef IN6_IS_ADDR_V4MAPPED
> +# pragma GCC poison IN6_IS_ADDR_V4MAPPED
> +# undef IN6_IS_ADDR_V4COMPAT
> +# pragma GCC poison IN6_IS_ADDR_V4COMPAT
> +# undef IN6_ARE_ADDR_EQUAL
> +# pragma GCC poison IN6_ARE_ADDR_EQUAL
> +# undef IN6_IS_ADDR_MULTICAST
> +# pragma GCC poison IN6_IS_ADDR_MULTICAST
> +#endif
> +
>  #define ALLNODES_MULTICAST  { .s6_addr = \
>                              { 0xff, 0x02, 0x00, 0x00,\
>                              0x00, 0x00, 0x00, 0x00,\
> 

Re: [Qemu-devel] [PATCH 10/12] configure: disable unaligned access warning on x86 arch

[Michael S. Tsirkin]

On Mon, Jan 08, 2018 at 05:32:54PM +0000, Peter Maydell wrote:
> On 8 January 2018 at 17:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  configure | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> 
> This doesn't seem like the right approach to me. We
> want this sort of thing to be a warning/error on x86,
> because that's the host that everybody actually uses
> to develop with. If they only show up on non-x86
> hosts then the result will be a lot more bounced
> pull requests because of only-non-x86 warnings.
> 
> thanks
> -- PMM

And we can't be sure the compiler won't be doing something tricky by
assuming alignment in the future.

-- 
MST

Re: [Qemu-devel] [PATCH 10/12] configure: disable unaligned access warning on x86 arch

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On 01/08/2018 02:32 PM, Peter Maydell wrote:
> On 8 January 2018 at 17:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  configure | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
> 
> This doesn't seem like the right approach to me. We
> want this sort of thing to be a warning/error on x86,
> because that's the host that everybody actually uses
> to develop with. If they only show up on non-x86
> hosts then the result will be a lot more bounced
> pull requests because of only-non-x86 warnings.

Ok, good point.

Thanks,

Phil.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 06/12] slirp: remove unused header

[Thomas Huth]

On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/slirp.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 9a7287e7cc..447dc045a8 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -1,7 +1,6 @@
>  #ifndef SLIRP_H
>  #define SLIRP_H
>  
> -#include "qemu/host-utils.h"

This had been added by commit 87776ab72b02e3c99a042ab7a0a378bc457cc069
which stated "There are some inclusions of qemu/host-utils.h in headers,
but they are all necessary." ... I wonder why it is not necessary
anymore today...?

Anyway, seems like it compiles now fine without this:

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 07/12] slirp: remove unnecessary

[Thomas Huth]

The subject is missing a word or two.

 Thomas

Re: [Qemu-devel] [PATCH 08/12] slirp: removed unused code

[Thomas Huth]

On 08.01.2018 18:29, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/ip.h | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/slirp/ip.h b/slirp/ip.h
> index e29ccd3c9f..71c3642cfe 100644
> --- a/slirp/ip.h
> +++ b/slirp/ip.h
> @@ -233,17 +233,4 @@ struct	ipasfrag {
>  #define ipf_next     ipf_link.next
>  #define ipf_prev     ipf_link.prev
>  
> -/*
> - * Structure stored in mbuf in inpcb.ip_options
> - * and passed to ip_output when ip options are in use.
> - * The actual length of the options (including ipopt_dst)
> - * is in m_len.
> - */
> -#define MAX_IPOPTLEN	40
> -
> -struct ipoption {
> -	struct	in_addr ipopt_dst;	/* first-hop dst if source routed */
> -	int8_t	ipopt_list[MAX_IPOPTLEN];	/* options proper */
> -} QEMU_PACKED;
> -
>  #endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 00/12] add HOST_SUPPORTS_UNALIGNED_ACCESS, reduce slirp warnings

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180108172904.8772-1-f4bug@amsat.org
Subject: [Qemu-devel] [PATCH 00/12] add HOST_SUPPORTS_UNALIGNED_ACCESS, reduce slirp warnings

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5d996c6ff9 slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS
14a3d450b8 configure: add HOST_SUPPORTS_UNALIGNED_ACCESS
73c11da125 configure: disable unaligned access warning on x86 arch
4e5e318d16 slirp: add in6_dhcp_multicast()
4cd7293d19 slirp: removed unused code
2e3244129b slirp: remove unnecessary
1803914958 slirp: remove unused header
381b027933 slirp: poison IN6_*_ADDR_*() macros to avoid them
5f7bee9c84 slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()
733c0d2f92 slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero()
accd0acef0 slirp: struct icmp/ethhdr ARE packed
2360510b2b slirp: remove QEMU_PACKED from structures with don't require it

=== OUTPUT BEGIN ===
Checking PATCH 1/12: slirp: remove QEMU_PACKED from structures with don't require it...
Checking PATCH 2/12: slirp: struct icmp/ethhdr ARE packed...
Checking PATCH 3/12: slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero()...
Checking PATCH 4/12: slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()...
Checking PATCH 5/12: slirp: poison IN6_*_ADDR_*() macros to avoid them...
WARNING: architecture specific defines should be avoided
#20: FILE: slirp/ip6.h:11:
+#ifdef __GNUC__

total: 0 errors, 1 warnings, 25 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 6/12: slirp: remove unused header...
Checking PATCH 7/12: slirp: remove unnecessary...
Checking PATCH 8/12: slirp: removed unused code...
Checking PATCH 9/12: slirp: add in6_dhcp_multicast()...
Checking PATCH 10/12: configure: disable unaligned access warning on x86 arch...
Checking PATCH 11/12: configure: add HOST_SUPPORTS_UNALIGNED_ACCESS...
Checking PATCH 12/12: slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS...
ERROR: return is not a function, parentheses are not required
#59: FILE: slirp/ip6.h:98:
+    return (aa[prefix_len / 8] >> (8 - (prefix_len % 8)))

ERROR: return is not a function, parentheses are not required
#90: FILE: slirp/ip6.h:139:
+    return (aa[prefix_len / 8] & ((1U << (8 - (prefix_len % 8))) - 1))

total: 2 errors, 0 warnings, 92 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Thomas Huth]

On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> Host: Mac OS 10.12.5
> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> 
>   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
>         structure 'ip6' may result in an unaligned pointer value
>         [-Waddress-of-packed-member]
>       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
>                                  ^~~~~~~~~~
>   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
>   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
>                                             ^
> 
> Reported-by: John Arbuckle <programmingkidx@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  slirp/ip6.h       |  5 +++++
>  slirp/ip6_icmp.c  | 10 +++++-----
>  slirp/ndp_table.c |  4 ++--
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/slirp/ip6.h b/slirp/ip6.h
> index b1bea43b3c..6c5d4eeaa3 100644
> --- a/slirp/ip6.h
> +++ b/slirp/ip6.h
> @@ -93,6 +93,11 @@ static inline bool in6_equal_mach(const struct in6_addr *a,
>  #define in6_zero(a)\
>      (in6_equal(a, &(struct in6_addr)ZERO_ADDR))

I think you should put a comment here to say why we need our own
function and can not use the IN6_IS_ADDR_MULTICAST macro instead -
otherwise people might be confused when looking at this code in a year
or two. (and now I've also understood why you're poisining the macros in
the next patch ... a comment in the code there would certainly not hurt
either).

> +static inline bool in6_multicast(const struct in6_addr *a)
> +{
> +    return a->s6_addr[0] == 0xff;
> +}
> +
>  /* Compute emulated host MAC address from its ipv6 address */
>  static inline void in6_compute_ethaddr(struct in6_addr ip,
>                                         uint8_t eth[ETH_ALEN])

 Thomas

Re: [Qemu-devel] [PATCH 01/12] slirp: remove QEMU_PACKED from structures with don't require it

[Thomas Huth]

On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> theses structures are not serialized and often store host pointers.

Patch looks fine at a quick glance... did you check that migration still
works (while there is network traffic going on in the guest)?

 Thomas

Re: [Qemu-devel] [RFC PATCH 12/12] slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS

[Paolo Bonzini]

On 08/01/2018 18:29, Philippe Mathieu-Daudé wrote:
> Access struct in6_addr with 'void *', then cast to 'u8 *' to avoid alignment
> issues.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> ugly...
> 
>  slirp/ip6.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)

What is the bug exactly?  If a->s6_addr can be unaligned, this should be
in the commit message.

In any case, there is no need to keep the versions that use struct
in6_addr, you can use the void* version unconditionally.

Paolo

Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Richard Henderson]

On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
> Host: Mac OS 10.12.5
> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> 
>   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
>         structure 'ip6' may result in an unaligned pointer value
>         [-Waddress-of-packed-member]
>       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
>                                  ^~~~~~~~~~
>   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
>   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)

The fact that you replace a macro with a function of exactly the same code in
order to avoid a diagnostic sure looks like a compiler bug to me.


r~

Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Thomas Huth]

On 09.01.2018 17:33, Richard Henderson wrote:
> On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
>> Host: Mac OS 10.12.5
>> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
>>
>>   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
>>         structure 'ip6' may result in an unaligned pointer value
>>         [-Waddress-of-packed-member]
>>       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
>>                                  ^~~~~~~~~~
>>   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
>>   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
> 
> The fact that you replace a macro with a function of exactly the same code in
> order to avoid a diagnostic sure looks like a compiler bug to me.

I think this might also be a bug in the macro definitions. On Linux, it
is defined like this:

#define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)

 Thomas

Re: [Qemu-devel] [PATCH 01/12] slirp: remove QEMU_PACKED from structures with don't require it

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:28:53 -0300, wrote:
>  struct mbuf_ptr {
>  	struct mbuf *mptr;
>  	uint32_t dummy;
> -} QEMU_PACKED;
> +};
>  #else
>  struct mbuf_ptr {
>  	struct mbuf *mptr;
> -} QEMU_PACKED;
> +};

> @@ -199,7 +199,7 @@ struct ipovly {
>  	uint16_t	ih_len;			/* protocol length */
>  	struct	in_addr ih_src;		/* source internet address */
>  	struct	in_addr ih_dst;		/* destination internet address */
> -} QEMU_PACKED;
> +};

Did you actually try to change these structures?  The presence of the
"dummy" field should have hinted that it's not a trivial structure :)

mbuf_ptr is used in struct ipovly which is to have the same size as the
ipv4 header.  One would have to untangle that before removing the packed
attribute.

> @@ -215,7 +215,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;
> +};

This one seems safe to me. I'd still rather see an actual test with
added holes in the structure to be sure :)

> @@ -225,7 +225,7 @@ struct ipq {
>  struct	ipasfrag {
>  	struct qlink ipf_link;
>  	struct ip ipf_ip;
> -} QEMU_PACKED;
> +};

Please see iptofrag and fragtoip in ip_input.c, they assume that ipf_ip
immediately follows ipf_link.  I guess using offsetof there should avoid
the issue.  Note however that slirp assumes that in a 32bit-aligned
ethernet frame it has enough room before the IP header to stuff the
ipf_link things.  We could add a build-time check that offsetof(ipf_ip)
<= 2 + ETH_HLEN

> @@ -136,7 +136,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>  struct ndpentry {
>      unsigned char   eth_addr[ETH_ALEN];     /* sender hardware address */
>      struct in6_addr ip_addr;                /* sender IP address       */
> -} QEMU_PACKED;
> +};

This one should be safe.

Samuel

Re: [Qemu-devel] [PATCH 03/12] slirp: avoid IN6_IS_ADDR_UNSPECIFIED(), rather use in6_zero()

[Samuel Thibault]

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:28:55 -0300, wrote:
> Host: Mac OS 10.12.5
> Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> 
>   slirp/ip6_icmp.c:80:38: warning: taking address of packed member 'ip_src' of class or
>         structure 'ip6' may result in an unaligned pointer value
>         [-Waddress-of-packed-member]
>               IN6_IS_ADDR_UNSPECIFIED(&ip->ip_src)) {
>                                        ^~~~~~~~~~
>   /usr/include/netinet6/in6.h:238:42: note: expanded from macro 'IN6_IS_ADDR_UNSPECIFIED'
>           ((*(const __uint32_t *)(const void *)(&(a)->s6_addr[0]) == 0) && \

I have applied it to my tree.

Thanks,
Samuel

Re: [Qemu-devel] [PATCH 04/12] slirp: add in6_multicast() and use it instead of IN6_IS_ADDR_MULTICAST()

[Samuel Thibault]

Richard Henderson, on mar. 09 janv. 2018 08:33:22 -0800, wrote:
> On 01/08/2018 09:28 AM, Philippe Mathieu-Daudé wrote:
> > Host: Mac OS 10.12.5
> > Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
> > 
> >   slirp/ip6_icmp.c:79:32: warning: taking address of packed member 'ip_src' of class or
> >         structure 'ip6' may result in an unaligned pointer value
> >         [-Waddress-of-packed-member]
> >       if (IN6_IS_ADDR_MULTICAST(&ip->ip_src) ||
> >                                  ^~~~~~~~~~
> >   /usr/include/netinet6/in6.h:299:36: note: expanded from macro 'IN6_IS_ADDR_MULTICAST'
> >   #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)
> 
> The fact that you replace a macro with a function of exactly the same code in
> order to avoid a diagnostic sure looks like a compiler bug to me.

The compiler could be smarter to realize that it's actually a byte access indeed.

There are two things for me:

- The OS-provided implementation of IN6_IS_ADDR_MULTICAST could be doing
  32bit accesses and whatnot, so we are not supposed to use it on packed
  fields.

- With the proposal patch, the compiler could still emit the warning,
  since we are basically still passing the address of the packed field
  to a function which the compiler might not see either.  We are however
  sure that the function makes an aligned access.

So I'd say we should still rather do it to be on the safe side. With
Thomas Huth's comments applied.

BTW,

Thomas Huth <thuth@redhat.com> wrote:
> I think this might also be a bug in the macro definitions.

> > > #define IN6_IS_ADDR_MULTICAST(a)        ((a)->s6_addr[0] == 0xff)

> On Linux, it is defined like this:
> 
> #define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)

Well, the standard says that IN6_IS_ADDR_MULTICAST takes a struct
in6_addr*, so it can use the s6_addr field.

Samuel

Re: [Qemu-devel] [PATCH 06/12] slirp: remove unused header

[Samuel Thibault]

Thomas Huth, on lun. 08 janv. 2018 19:54:27 +0100, wrote:
> On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  slirp/slirp.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/slirp/slirp.h b/slirp/slirp.h
> > index 9a7287e7cc..447dc045a8 100644
> > --- a/slirp/slirp.h
> > +++ b/slirp/slirp.h
> > @@ -1,7 +1,6 @@
> >  #ifndef SLIRP_H
> >  #define SLIRP_H
> >  
> > -#include "qemu/host-utils.h"
> 
> This had been added by commit 87776ab72b02e3c99a042ab7a0a378bc457cc069
> which stated "There are some inclusions of qemu/host-utils.h in headers,
> but they are all necessary." ... I wonder why it is not necessary
> anymore today...?
> 
> Anyway, seems like it compiles now fine without this:
> 
> Tested-by: Thomas Huth <thuth@redhat.com>

Applied to my tree, thanks!

Samuel

Re: [Qemu-devel] [PATCH 07/12] slirp: remove unnecessary

[Samuel Thibault]

Thomas Huth, on lun. 08 janv. 2018 20:01:10 +0100, wrote:
> The subject is missing a word or two.

Applied to my tree with a more complete subject :)

Samuel

Re: [Qemu-devel] [PATCH 08/12] slirp: removed unused code

[Samuel Thibault]

Thomas Huth, on lun. 08 janv. 2018 20:02:14 +0100, wrote:
> On 08.01.2018 18:29, Philippe Mathieu-Daudé wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  slirp/ip.h | 13 -------------
> >  1 file changed, 13 deletions(-)
> > 
> > diff --git a/slirp/ip.h b/slirp/ip.h
> > index e29ccd3c9f..71c3642cfe 100644
> > --- a/slirp/ip.h
> > +++ b/slirp/ip.h
> > @@ -233,17 +233,4 @@ struct	ipasfrag {
> >  #define ipf_next     ipf_link.next
> >  #define ipf_prev     ipf_link.prev
> >  
> > -/*
> > - * Structure stored in mbuf in inpcb.ip_options
> > - * and passed to ip_output when ip options are in use.
> > - * The actual length of the options (including ipopt_dst)
> > - * is in m_len.
> > - */
> > -#define MAX_IPOPTLEN	40
> > -
> > -struct ipoption {
> > -	struct	in_addr ipopt_dst;	/* first-hop dst if source routed */
> > -	int8_t	ipopt_list[MAX_IPOPTLEN];	/* options proper */
> > -} QEMU_PACKED;
> > -
> >  #endif
> > 
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Applied to my tree.

Thanks,
Samuel

Re: [Qemu-devel] [PATCH 09/12] slirp: add in6_dhcp_multicast()

[Samuel Thibault]

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:29:01 -0300, wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Applied to my tree.

Thanks,
Samuel

> ---
>  slirp/dhcpv6.h | 3 +++
>  slirp/udp6.c   | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/dhcpv6.h b/slirp/dhcpv6.h
> index 9189cd3f2d..3373f6cb89 100644
> --- a/slirp/dhcpv6.h
> +++ b/slirp/dhcpv6.h
> @@ -17,6 +17,9 @@
>                              0x00, 0x00, 0x00, 0x00,\
>                              0x00, 0x01, 0x00, 0x02 } }
>  
> +#define in6_dhcp_multicast(a)\
> +    in6_equal(a, &(struct in6_addr)ALLDHCP_MULTICAST)
> +
>  void dhcpv6_input(struct sockaddr_in6 *srcsas, struct mbuf *m);
>  
>  #endif
> diff --git a/slirp/udp6.c b/slirp/udp6.c
> index 9fa314bc2d..7c4a6b003a 100644
> --- a/slirp/udp6.c
> +++ b/slirp/udp6.c
> @@ -65,7 +65,7 @@ void udp6_input(struct mbuf *m)
>      /* handle DHCPv6 */
>      if (ntohs(uh->uh_dport) == DHCPV6_SERVER_PORT &&
>          (in6_equal(&ip->ip_dst, &slirp->vhost_addr6) ||
> -         in6_equal(&ip->ip_dst, &(struct in6_addr)ALLDHCP_MULTICAST))) {
> +         in6_dhcp_multicast(&ip->ip_dst))) {
>          m->m_data += iphlen;
>          m->m_len -= iphlen;
>          dhcpv6_input(&lhost, m);
> -- 
> 2.15.1
> 

-- 
Samuel
 Créer une hiérarchie supplementaire pour remedier à un problème (?) de
 dispersion est d'une logique digne des Shadocks.
 * BT in: Guide du Cabaliste Usenet - La Cabale vote oui (les Shadocks aussi) *

Re: [Qemu-devel] [RFC PATCH 12/12] slirp: use HOST_SUPPORTS_UNALIGNED_ACCESS

[Samuel Thibault]

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:29:04 -0300, wrote:
> Access struct in6_addr with 'void *', then cast to 'u8 *' to avoid alignment
> issues.

Err, I don't understand the point. in6_addr's s6_addr is already a
uint8_t by the standard.  There is no non-byte access in the existing
code.

Samuel