[ovs-dev] [PATCH] packets: Change IP_ARGS interface to take an ovs_be32 instead of a pointer.
Ethan Jackson
ethan at nicira.com
Wed Dec 12 23:13:36 UTC 2012
Acked-by: Ethan Jackson <ethan at nicira.com>
On Thu, Nov 8, 2012 at 5:12 PM, Ben Pfaff <blp at nicira.com> wrote:
> An ovs_be32 is a more obvious way to represent an IP address than a
> pointer to one. It is also more type-safe, especially since "sparse" is
> able to check that the argument is in network byte order.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> lib/netdev-linux.c | 2 +-
> lib/netdev-vport.c | 4 ++--
> lib/odp-util.c | 10 +++++-----
> lib/ofp-actions.c | 4 ++--
> lib/ofp-print.c | 2 +-
> lib/packets.c | 4 ++--
> lib/packets.h | 14 +++++---------
> lib/socket-util.c | 2 +-
> lib/stream-ssl.c | 4 ++--
> lib/stream-tcp.c | 4 ++--
> ofproto/in-band.c | 6 +++---
> tests/test-netflow.c | 4 ++--
> vswitchd/bridge.c | 4 ++--
> 13 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 0460c06..35b4562 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2327,7 +2327,7 @@ netdev_linux_arp_lookup(const struct netdev *netdev,
> memcpy(mac, r.arp_ha.sa_data, ETH_ADDR_LEN);
> } else if (retval != ENXIO) {
> VLOG_WARN_RL(&rl, "%s: could not look up ARP entry for "IP_FMT":
> %s",
> - netdev_get_name(netdev), IP_ARGS(&ip),
> strerror(retval));
> + netdev_get_name(netdev), IP_ARGS(ip),
> strerror(retval));
> }
> return retval;
> }
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 5171171..da3f733 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -790,11 +790,11 @@ unparse_tunnel_config(const char *name OVS_UNUSED,
> const char *type OVS_UNUSED,
>
>
> daddr = nl_attr_get_be32(a[OVS_TUNNEL_ATTR_DST_IPV4]);
> - smap_add_format(args, "remote_ip", IP_FMT, IP_ARGS(&daddr));
> + smap_add_format(args, "remote_ip", IP_FMT, IP_ARGS(daddr));
>
> if (a[OVS_TUNNEL_ATTR_SRC_IPV4]) {
> ovs_be32 saddr = nl_attr_get_be32(a[OVS_TUNNEL_ATTR_SRC_IPV4]);
> - smap_add_format(args, "local_ip", IP_FMT, IP_ARGS(&saddr));
> + smap_add_format(args, "local_ip", IP_FMT, IP_ARGS(saddr));
> }
>
> if (!a[OVS_TUNNEL_ATTR_IN_KEY] && !a[OVS_TUNNEL_ATTR_OUT_KEY]) {
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 08823e2..978fd9a 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -706,8 +706,8 @@ format_odp_key_attr(const struct nlattr *a, struct ds
> *ds)
> ds_put_format(ds, "(tun_id=0x%"PRIx64",flags=0x%"PRIx32
>
> ",src="IP_FMT",dst="IP_FMT",tos=0x%"PRIx8",ttl=%"PRIu8")",
> ntohll(ipv4_tun_key->tun_id),
> ipv4_tun_key->tun_flags,
> - IP_ARGS(&ipv4_tun_key->ipv4_src),
> - IP_ARGS(&ipv4_tun_key->ipv4_dst),
> + IP_ARGS(ipv4_tun_key->ipv4_src),
> + IP_ARGS(ipv4_tun_key->ipv4_dst),
> ipv4_tun_key->ipv4_tos, ipv4_tun_key->ipv4_ttl);
> break;
>
> @@ -737,8 +737,8 @@ format_odp_key_attr(const struct nlattr *a, struct ds
> *ds)
> ipv4_key = nl_attr_get(a);
> ds_put_format(ds, "(src="IP_FMT",dst="IP_FMT",proto=%"PRIu8
> ",tos=%#"PRIx8",ttl=%"PRIu8",frag=%s)",
> - IP_ARGS(&ipv4_key->ipv4_src),
> - IP_ARGS(&ipv4_key->ipv4_dst),
> + IP_ARGS(ipv4_key->ipv4_src),
> + IP_ARGS(ipv4_key->ipv4_dst),
> ipv4_key->ipv4_proto, ipv4_key->ipv4_tos,
> ipv4_key->ipv4_ttl,
> ovs_frag_type_to_string(ipv4_key->ipv4_frag));
> @@ -789,7 +789,7 @@ format_odp_key_attr(const struct nlattr *a, struct ds
> *ds)
> arp_key = nl_attr_get(a);
> ds_put_format(ds, "(sip="IP_FMT",tip="IP_FMT",op=%"PRIu16","
> "sha="ETH_ADDR_FMT",tha="ETH_ADDR_FMT")",
> - IP_ARGS(&arp_key->arp_sip),
> IP_ARGS(&arp_key->arp_tip),
> + IP_ARGS(arp_key->arp_sip),
> IP_ARGS(arp_key->arp_tip),
> ntohs(arp_key->arp_op),
> ETH_ADDR_ARGS(arp_key->arp_sha),
> ETH_ADDR_ARGS(arp_key->arp_tha));
> break;
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 170e796..d192507 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1930,12 +1930,12 @@ ofpact_format(const struct ofpact *a, struct ds *s)
>
> case OFPACT_SET_IPV4_SRC:
> ds_put_format(s, "mod_nw_src:"IP_FMT,
> - IP_ARGS(&ofpact_get_SET_IPV4_SRC(a)->ipv4));
> + IP_ARGS(ofpact_get_SET_IPV4_SRC(a)->ipv4));
> break;
>
> case OFPACT_SET_IPV4_DST:
> ds_put_format(s, "mod_nw_dst:"IP_FMT,
> - IP_ARGS(&ofpact_get_SET_IPV4_DST(a)->ipv4));
> + IP_ARGS(ofpact_get_SET_IPV4_DST(a)->ipv4));
> break;
>
> case OFPACT_SET_IPV4_DSCP:
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index c1b50e2..60f0a41 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -571,7 +571,7 @@ print_ip_netmask(struct ds *string, const char
> *leader, ovs_be32 ip,
> }
> ds_put_cstr(string, leader);
> if (wild_bits < 32) {
> - ds_put_format(string, IP_FMT, IP_ARGS(&ip));
> + ds_put_format(string, IP_FMT, IP_ARGS(ip));
> if (wild_bits) {
> ds_put_format(string, "/%d", 32 - wild_bits);
> }
> diff --git a/lib/packets.c b/lib/packets.c
> index 16f4fe6..8850fa0 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -272,12 +272,12 @@ ip_count_cidr_bits(ovs_be32 netmask)
> void
> ip_format_masked(ovs_be32 ip, ovs_be32 mask, struct ds *s)
> {
> - ds_put_format(s, IP_FMT, IP_ARGS(&ip));
> + ds_put_format(s, IP_FMT, IP_ARGS(ip));
> if (mask != htonl(UINT32_MAX)) {
> if (ip_is_cidr(mask)) {
> ds_put_format(s, "/%d", ip_count_cidr_bits(mask));
> } else {
> - ds_put_format(s, "/"IP_FMT, IP_ARGS(&mask));
> + ds_put_format(s, "/"IP_FMT, IP_ARGS(mask));
> }
> }
> }
> diff --git a/lib/packets.h b/lib/packets.h
> index e550be0..72d1843 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -272,16 +272,12 @@ struct vlan_eth_header {
> } __attribute__((packed));
> BUILD_ASSERT_DECL(VLAN_ETH_HEADER_LEN == sizeof(struct vlan_eth_header));
>
> -/* The "(void) (ip)[0]" below has no effect on the value, since it's the
> first
> - * argument of a comma expression, but it makes sure that 'ip' is a
> pointer.
> - * This is useful since a common mistake is to pass an integer instead of
> a
> - * pointer to IP_ARGS. */
> -#define IP_FMT "%"PRIu8".%"PRIu8".%"PRIu8".%"PRIu8
> +#define IP_FMT "%"PRIu32".%"PRIu32".%"PRIu32".%"PRIu32
> #define IP_ARGS(ip) \
> - ((void) (ip)[0], ((uint8_t *) ip)[0]), \
> - ((uint8_t *) ip)[1], \
> - ((uint8_t *) ip)[2], \
> - ((uint8_t *) ip)[3]
> + ntohl(ip) >> 24, \
> + (ntohl(ip) >> 16) & 0xff, \
> + (ntohl(ip) >> 8) & 0xff, \
> + ntohl(ip) & 0xff
>
> /* Example:
> *
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 4edf956..583bd0a 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -945,7 +945,7 @@ describe_sockaddr(struct ds *string, int fd,
>
> memcpy(&sin, &ss, sizeof sin);
> ds_put_format(string, IP_FMT":%"PRIu16,
> - IP_ARGS(&sin.sin_addr.s_addr),
> ntohs(sin.sin_port));
> + IP_ARGS(sin.sin_addr.s_addr),
> ntohs(sin.sin_port));
> } else if (ss.ss_family == AF_UNIX) {
> struct sockaddr_un sun;
> const char *null;
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index 0ca5b18..184b3ff 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -805,7 +805,7 @@ pssl_open(const char *name OVS_UNUSED, char *suffix,
> struct pstream **pstreamp,
> return -fd;
> }
> sprintf(bound_name, "pssl:%"PRIu16":"IP_FMT,
> - ntohs(sin.sin_port), IP_ARGS(&sin.sin_addr.s_addr));
> + ntohs(sin.sin_port), IP_ARGS(sin.sin_addr.s_addr));
>
> pssl = xmalloc(sizeof *pssl);
> pstream_init(&pssl->pstream, &pssl_pstream_class, bound_name);
> @@ -847,7 +847,7 @@ pssl_accept(struct pstream *pstream, struct stream
> **new_streamp)
> return error;
> }
>
> - sprintf(name, "ssl:"IP_FMT, IP_ARGS(&sin.sin_addr));
> + sprintf(name, "ssl:"IP_FMT, IP_ARGS(sin.sin_addr.s_addr));
> if (sin.sin_port != htons(OFP_SSL_PORT)) {
> sprintf(strchr(name, '\0'), ":%"PRIu16, ntohs(sin.sin_port));
> }
> diff --git a/lib/stream-tcp.c b/lib/stream-tcp.c
> index 05601e1..0384c42 100644
> --- a/lib/stream-tcp.c
> +++ b/lib/stream-tcp.c
> @@ -116,7 +116,7 @@ ptcp_open(const char *name OVS_UNUSED, char *suffix,
> struct pstream **pstreamp,
> }
>
> sprintf(bound_name, "ptcp:%"PRIu16":"IP_FMT,
> - ntohs(sin.sin_port), IP_ARGS(&sin.sin_addr.s_addr));
> + ntohs(sin.sin_port), IP_ARGS(sin.sin_addr.s_addr));
> return new_fd_pstream(bound_name, fd, ptcp_accept, set_dscp, NULL,
> pstreamp);
> }
> @@ -129,7 +129,7 @@ ptcp_accept(int fd, const struct sockaddr *sa, size_t
> sa_len,
> char name[128];
>
> if (sa_len == sizeof(struct sockaddr_in) && sin->sin_family ==
> AF_INET) {
> - sprintf(name, "tcp:"IP_FMT, IP_ARGS(&sin->sin_addr));
> + sprintf(name, "tcp:"IP_FMT, IP_ARGS(sin->sin_addr.s_addr));
> sprintf(strchr(name, '\0'), ":%"PRIu16, ntohs(sin->sin_port));
> } else {
> strcpy(name, "tcp");
> diff --git a/ofproto/in-band.c b/ofproto/in-band.c
> index 0e4754c..81b330d 100644
> --- a/ofproto/in-band.c
> +++ b/ofproto/in-band.c
> @@ -120,7 +120,7 @@ refresh_remote(struct in_band *ib, struct
> in_band_remote *r)
> &next_hop_inaddr, &next_hop_dev);
> if (retval) {
> VLOG_WARN("cannot find route for controller ("IP_FMT"): %s",
> - IP_ARGS(&r->remote_addr.sin_addr), strerror(retval));
> + IP_ARGS(r->remote_addr.sin_addr.s_addr),
> strerror(retval));
> return 1;
> }
> if (!next_hop_inaddr.s_addr) {
> @@ -137,7 +137,7 @@ refresh_remote(struct in_band *ib, struct
> in_band_remote *r)
> if (retval) {
> VLOG_WARN_RL(&rl, "cannot open netdev %s (next hop "
> "to controller "IP_FMT"): %s",
> - next_hop_dev, IP_ARGS(&r->remote_addr.sin_addr),
> + next_hop_dev,
> IP_ARGS(r->remote_addr.sin_addr.s_addr),
> strerror(retval));
> free(next_hop_dev);
> return 1;
> @@ -150,7 +150,7 @@ refresh_remote(struct in_band *ib, struct
> in_band_remote *r)
> r->remote_mac);
> if (retval) {
> VLOG_DBG_RL(&rl, "cannot look up remote MAC address ("IP_FMT"):
> %s",
> - IP_ARGS(&next_hop_inaddr.s_addr), strerror(retval));
> + IP_ARGS(next_hop_inaddr.s_addr), strerror(retval));
> }
>
> /* If we don't have a MAC address, then refresh quickly, since we
> probably
> diff --git a/tests/test-netflow.c b/tests/test-netflow.c
> index c37eeaf..5147a20 100644
> --- a/tests/test-netflow.c
> +++ b/tests/test-netflow.c
> @@ -75,7 +75,7 @@ print_netflow(struct ofpbuf *buf)
> }
>
> printf("seq %"PRIu32": "IP_FMT" > "IP_FMT, ntohl(hdr->flow_seq),
> - IP_ARGS(&rec->src_addr), IP_ARGS(&rec->dst_addr));
> + IP_ARGS(rec->src_addr), IP_ARGS(rec->dst_addr));
>
> printf(", if %"PRIu16" > %"PRIu16,
> ntohs(rec->input), ntohs(rec->output));
> @@ -137,7 +137,7 @@ print_netflow(struct ofpbuf *buf)
> ntohl(rec->init_time), ntohl(rec->used_time));
>
> if (rec->nexthop != htonl(0)) {
> - printf(", nexthop "IP_FMT, IP_ARGS(&rec->nexthop));
> + printf(", nexthop "IP_FMT, IP_ARGS(rec->nexthop));
> }
> if (rec->src_as != htons(0) || rec->dst_as != htons(0)) {
> printf(", AS %"PRIu16" > %"PRIu16,
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 8f544a9..0dd16c1 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2691,7 +2691,7 @@ bridge_configure_local_iface_netdev(struct bridge
> *br,
> }
> if (!netdev_set_in4(netdev, ip, mask)) {
> VLOG_INFO("bridge %s: configured IP address "IP_FMT", netmask
> "IP_FMT,
> - br->name, IP_ARGS(&ip.s_addr), IP_ARGS(&mask.s_addr));
> + br->name, IP_ARGS(ip.s_addr), IP_ARGS(mask.s_addr));
> }
>
> /* Configure the default gateway. */
> @@ -2700,7 +2700,7 @@ bridge_configure_local_iface_netdev(struct bridge
> *br,
> && gateway.s_addr) {
> if (!netdev_add_router(netdev, gateway)) {
> VLOG_INFO("bridge %s: configured gateway "IP_FMT,
> - br->name, IP_ARGS(&gateway.s_addr));
> + br->name, IP_ARGS(gateway.s_addr));
> }
> }
> }
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121212/faf6a7b3/attachment-0003.html>
More information about the dev
mailing list