On Thu, Oct 13, 2011 at 11:29 AM, Ben Pfaff <[email protected]> wrote: > Pravin's testing found a bug in my patch. v2 fixes up the memset and > memcpy calls, like so: > > - memset(&dst, 0, sizeof dst); > - memcpy(&dst, &src, 64); > + memset(dst, 0, sizeof *dst); > + memcpy(dst, src, 64); >
This change fixes the issue. --pravin. > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <[email protected]> > Date: Thu, 13 Oct 2011 11:22:44 -0700 > Subject: [PATCH] dpif-linux: Fix build with certain 64-bit kernel/userspace > combinations. > > Unix 64-bit ABIs have two 64-bit types: "long" and "long long". Either of > these is a reasonable choice for uint64_t (the userspace type) and for > __u64 (the kernel type). Unfortunately, kernel and userspace don't > necessarily agree on the choice, and in fact the choice varies across > kernel versions and architectures. > > Now that OVS is actually using kernel types in its kernel header, this > can make a difference: when __u64 and uint64_t differ, passing a pointer > to __u64 to OVS function get_unaligned_u64() yields a compiler warning > or error. > > This commit fixes up the problems of this type found in OVS, by avoiding > calls to get_unaligned_u64() on these types. > > I suspect that this problem will recur in the future. I'm open to > suggestions on how we can making "uint64_t *" and "__u64 *" always > incompatible from the viewpoint of sparse. > > Reported-by: Pravin Shelar <[email protected]> > --- > lib/dpif-linux.c | 19 +++++-------------- > lib/netdev-vport.c | 49 ++++++++++++++++++++++++++++--------------------- > lib/util.h | 4 ++++ > 3 files changed, 37 insertions(+), 35 deletions(-) > > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c > index 43c2161..9566dcb 100644 > --- a/lib/dpif-linux.c > +++ b/lib/dpif-linux.c > @@ -102,17 +102,13 @@ struct dpif_linux_flow { > > /* Attributes. > * > - * The 'stats' member points to 64-bit data that might only be aligned on > - * 32-bit boundaries, so get_unaligned_u64() should be used to access its > - * values. > - * > * If 'actions' is nonnull then OVS_FLOW_ATTR_ACTIONS will be included in > * the Netlink version of the command, even if actions_len is zero. */ > const struct nlattr *key; /* OVS_FLOW_ATTR_KEY. */ > size_t key_len; > const struct nlattr *actions; /* OVS_FLOW_ATTR_ACTIONS. */ > size_t actions_len; > - const struct ovs_flow_stats *stats; /* OVS_FLOW_ATTR_STATS. */ > + struct ovs_flow_stats stats; /* OVS_FLOW_ATTR_STATS. */ > const uint8_t *tcp_flags; /* OVS_FLOW_ATTR_TCP_FLAGS. */ > const ovs_32aligned_u64 *used; /* OVS_FLOW_ATTR_USED. */ > bool clear; /* OVS_FLOW_ATTR_CLEAR. */ > @@ -1622,7 +1618,8 @@ dpif_linux_flow_from_ofpbuf(struct dpif_linux_flow > *flow, > flow->actions_len = nl_attr_get_size(a[OVS_FLOW_ATTR_ACTIONS]); > } > if (a[OVS_FLOW_ATTR_STATS]) { > - flow->stats = nl_attr_get(a[OVS_FLOW_ATTR_STATS]); > + memcpy(&flow->stats, nl_attr_get(a[OVS_FLOW_ATTR_STATS]), > + sizeof flow->stats); > } > if (a[OVS_FLOW_ATTR_TCP_FLAGS]) { > flow->tcp_flags = nl_attr_get(a[OVS_FLOW_ATTR_TCP_FLAGS]); > @@ -1658,7 +1655,6 @@ dpif_linux_flow_to_ofpbuf(const struct dpif_linux_flow > *flow, > } > > /* We never need to send these to the kernel. */ > - assert(!flow->stats); > assert(!flow->tcp_flags); > assert(!flow->used); > > @@ -1711,13 +1707,8 @@ static void > dpif_linux_flow_get_stats(const struct dpif_linux_flow *flow, > struct dpif_flow_stats *stats) > { > - if (flow->stats) { > - stats->n_packets = get_unaligned_u64(&flow->stats->n_packets); > - stats->n_bytes = get_unaligned_u64(&flow->stats->n_bytes); > - } else { > - stats->n_packets = 0; > - stats->n_bytes = 0; > - } > + stats->n_packets = flow->stats.n_packets; > + stats->n_bytes = flow->stats.n_bytes; > stats->used = flow->used ? get_32aligned_u64(flow->used) : 0; > stats->tcp_flags = flow->tcp_flags ? *flow->tcp_flags : 0; > } > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 301bb43..868c935 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -369,27 +369,34 @@ static void > netdev_stats_from_ovs_vport_stats(struct netdev_stats *dst, > const struct ovs_vport_stats *src) > { > - dst->rx_packets = get_unaligned_u64(&src->rx_packets); > - dst->tx_packets = get_unaligned_u64(&src->tx_packets); > - dst->rx_bytes = get_unaligned_u64(&src->rx_bytes); > - dst->tx_bytes = get_unaligned_u64(&src->tx_bytes); > - dst->rx_errors = get_unaligned_u64(&src->rx_errors); > - dst->tx_errors = get_unaligned_u64(&src->tx_errors); > - dst->rx_dropped = get_unaligned_u64(&src->rx_dropped); > - dst->tx_dropped = get_unaligned_u64(&src->tx_dropped); > - dst->multicast = 0; > - dst->collisions = 0; > - dst->rx_length_errors = 0; > - dst->rx_over_errors = 0; > - dst->rx_crc_errors = 0; > - dst->rx_frame_errors = 0; > - dst->rx_fifo_errors = 0; > - dst->rx_missed_errors = 0; > - dst->tx_aborted_errors = 0; > - dst->tx_carrier_errors = 0; > - dst->tx_fifo_errors = 0; > - dst->tx_heartbeat_errors = 0; > - dst->tx_window_errors = 0; > + /* ovs_vport_stats and netdev_stats start with members that have the same > + * meaning in the same order, so we can just memcpy() them across as a > + * block. There's more at play here than an optimization: 'src' might be > + * misaligned, so we can't just do memberwise assignments. Second, we > + * can't just use get_unaligned_u64() on the members of 'src' because > they > + * are "__u64"s, which type might be different from "uint64_t" (e.g. > "long" > + * versus "long long") and thus we'd get a compiler warning or error > about > + * type mismatch. > + * > + * The following macro gunge checks that in fact all of the common > members > + * have identical sizes and offets. */ > +#define VERIFY_STAT_MEMBER(MEMBER, OFFSET) \ > + BUILD_ASSERT_DECL(offsetof(struct netdev_stats, MEMBER) == (OFFSET)); \ > + BUILD_ASSERT_DECL(offsetof(struct ovs_vport_stats, MEMBER) == (OFFSET)); > \ > + BUILD_ASSERT_DECL(MEMBER_SIZEOF(struct netdev_stats, MEMBER) == 8); \ > + BUILD_ASSERT_DECL(MEMBER_SIZEOF(struct ovs_vport_stats, MEMBER) == 8) > + > + VERIFY_STAT_MEMBER(rx_packets, 0); > + VERIFY_STAT_MEMBER(tx_packets, 8); > + VERIFY_STAT_MEMBER(rx_bytes, 16); > + VERIFY_STAT_MEMBER(tx_bytes, 24); > + VERIFY_STAT_MEMBER(rx_errors, 32); > + VERIFY_STAT_MEMBER(tx_errors, 40); > + VERIFY_STAT_MEMBER(rx_dropped, 48); > + VERIFY_STAT_MEMBER(tx_dropped, 56); > + > + memset(dst, 0, sizeof *dst); > + memcpy(dst, src, 64); > } > > /* Copies 'src' into 'dst', performing format conversion in the process. */ > diff --git a/lib/util.h b/lib/util.h > index 5ae0775..4ff2734 100644 > --- a/lib/util.h > +++ b/lib/util.h > @@ -88,6 +88,10 @@ extern const char *program_name; > #define STRINGIZE(ARG) STRINGIZE2(ARG) > #define STRINGIZE2(ARG) #ARG > > +/* Expands to the size of MEMBER within TYPE, which must be a structure type > + * such that appending "*" yields a pointer to TYPE. */ > +#define MEMBER_SIZEOF(STRUCT, MEMBER) (sizeof ((STRUCT *) 0)->MEMBER) > + > /* Given a pointer-typed lvalue OBJECT, expands to a pointer type that may be > * assigned to OBJECT. */ > #ifdef __GNUC__ > -- > 1.7.4.4 > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
