Re: [ovs-dev] [threads 01/23] dpif-netdev: Make 'max_mtu' a per-dp feature, for thread safety.

[Alex Wang]

Looks good to me, thanks,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> This ensures that an external lock around a dpif_netdev will allow
> thread-safe access to it.
>
> Signed-off-by: Ben Pfaff 
> ---
>  lib/dpif-netdev.c |   12 ++--
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index f7e3b1f..d21eb8d 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -87,6 +87,7 @@ struct dp_netdev {
>  char *name;
>  int open_cnt;
>  bool destroyed;
> +int max_mtu;/* Maximum MTU of any port added so far.
> */
>
>  struct dp_netdev_queue queues[N_QUEUES];
>  struct hmap flow_table; /* Flow table. */
> @@ -138,9 +139,6 @@ struct dpif_netdev {
>  /* All netdev-based datapaths. */
>  static struct shash dp_netdevs = SHASH_INITIALIZER(&dp_netdevs);
>
> -/* Maximum port MTU seen so far. */
> -static int max_mtu = ETH_PAYLOAD_MAX;
> -
>  static int get_port_by_number(struct dp_netdev *, odp_port_t port_no,
>struct dp_netdev_port **portp);
>  static int get_port_by_name(struct dp_netdev *, const char *devname,
> @@ -271,6 +269,7 @@ create_dp_netdev(const char *name, const struct
> dpif_class *class,
>  dp->class = class;
>  dp->name = xstrdup(name);
>  dp->open_cnt = 0;
> +dp->max_mtu = ETH_PAYLOAD_MAX;
>  for (i = 0; i < N_QUEUES; i++) {
>  dp->queues[i].head = dp->queues[i].tail = 0;
>  }
> @@ -426,8 +425,8 @@ do_add_port(struct dp_netdev *dp, const char *devname,
> const char *type,
>  port->type = xstrdup(type);
>
>  error = netdev_get_mtu(netdev, &mtu);
> -if (!error && mtu > max_mtu) {
> -max_mtu = mtu;
> +if (!error && mtu > dp->max_mtu) {
> +dp->max_mtu = mtu;
>  }
>
>  list_push_back(&dp->port_list, &port->node);
> @@ -1081,7 +1080,8 @@ dpif_netdev_run(struct dpif *dpif)
>  struct dp_netdev_port *port;
>  struct ofpbuf packet;
>
> -ofpbuf_init(&packet, DP_NETDEV_HEADROOM + VLAN_ETH_HEADER_LEN +
> max_mtu);
> +ofpbuf_init(&packet,
> +DP_NETDEV_HEADROOM + VLAN_ETH_HEADER_LEN + dp->max_mtu);
>
>  LIST_FOR_EACH (port, node, &dp->port_list) {
>  int error;
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 3/3] clang: Fix the alignment warning.

[Alex Wang]

This commit fixes the warning issued by 'clang' when pointer is casted
to one with greater alignment.

Signed-off-by: Alex Wang 
---
 lib/hash.c |6 +++---
 lib/jhash.c|   10 +-
 lib/mac-learning.c |4 ++--
 lib/netdev-linux.c |3 ++-
 lib/netlink.c  |7 +++
 lib/nx-match.c |   12 +++-
 lib/nx-match.h |2 +-
 lib/odp-util.c |3 ++-
 lib/ofp-actions.c  |   27 ++-
 lib/ofp-actions.h  |2 +-
 lib/ofp-util.c |7 ---
 lib/ofpbuf.c   |7 +++
 lib/ofpbuf.h   |1 +
 lib/ovs-atomic-gcc4+.h |4 ++--
 lib/packets.c  |8 +---
 lib/route-table.c  |2 +-
 lib/rtnetlink-link.c   |4 ++--
 lib/socket-util.c  |5 +++--
 lib/stream-tcp.c   |3 ++-
 lib/util.h |4 
 utilities/ovs-dpctl.c  |3 ++-
 utilities/ovs-ofctl.c  |4 ++--
 22 files changed, 75 insertions(+), 53 deletions(-)

diff --git a/lib/hash.c b/lib/hash.c
index e954d78..8f34493 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -29,15 +29,15 @@ hash_3words(uint32_t a, uint32_t b, uint32_t c)
 uint32_t
 hash_bytes(const void *p_, size_t n, uint32_t basis)
 {
-const uint8_t *p = p_;
+const uint32_t *p = p_;
 size_t orig_n = n;
 uint32_t hash;
 
 hash = basis;
 while (n >= 4) {
-hash = mhash_add(hash, get_unaligned_u32((const uint32_t *) p));
+hash = mhash_add(hash, get_unaligned_u32(p));
 n -= 4;
-p += 4;
+p += 1;
 }
 
 if (n) {
diff --git a/lib/jhash.c b/lib/jhash.c
index 4ec2871..c08c368 100644
--- a/lib/jhash.c
+++ b/lib/jhash.c
@@ -96,18 +96,18 @@ jhash_words(const uint32_t *p, size_t n, uint32_t basis)
 uint32_t
 jhash_bytes(const void *p_, size_t n, uint32_t basis)
 {
-const uint8_t *p = p_;
+const uint32_t *p = p_;
 uint32_t a, b, c;
 
 a = b = c = 0xdeadbeef + n + basis;
 
 while (n >= 12) {
-a += get_unaligned_u32((uint32_t *) p);
-b += get_unaligned_u32((uint32_t *) (p + 4));
-c += get_unaligned_u32((uint32_t *) (p + 8));
+a += get_unaligned_u32(p);
+b += get_unaligned_u32(p + 1);
+c += get_unaligned_u32(p + 2);
 jhash_mix(&a, &b, &c);
 n -= 12;
-p += 12;
+p += 3;
 }
 
 if (n) {
diff --git a/lib/mac-learning.c b/lib/mac-learning.c
index ca0ccb8..0fa0459 100644
--- a/lib/mac-learning.c
+++ b/lib/mac-learning.c
@@ -49,8 +49,8 @@ static uint32_t
 mac_table_hash(const struct mac_learning *ml, const uint8_t mac[ETH_ADDR_LEN],
uint16_t vlan)
 {
-unsigned int mac1 = get_unaligned_u32((uint32_t *) mac);
-unsigned int mac2 = get_unaligned_u16((uint16_t *) (mac + 4));
+unsigned int mac1 = get_unaligned_u32(ALIGNED_CAST(uint32_t *, mac));
+unsigned int mac2 = get_unaligned_u16(ALIGNED_CAST(uint16_t *, (mac + 4)));
 return hash_3words(mac1, mac2 | (vlan << 16), ml->secret);
 }
 
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 05877c1..c2f09ea 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -4546,7 +4546,8 @@ netdev_linux_get_ipv4(const struct netdev *netdev, struct 
in_addr *ip,
 ifr.ifr_addr.sa_family = AF_INET;
 error = netdev_linux_do_ioctl(netdev_get_name(netdev), &ifr, cmd, 
cmd_name);
 if (!error) {
-const struct sockaddr_in *sin = (struct sockaddr_in *) &ifr.ifr_addr;
+const struct sockaddr_in *sin = ALIGNED_CAST(struct sockaddr_in *,
+ &ifr.ifr_addr);
 *ip = sin->sin_addr;
 }
 return error;
diff --git a/lib/netlink.c b/lib/netlink.c
index a686784..1f96b18 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -712,7 +712,7 @@ nl_policy_parse(const struct ofpbuf *msg, size_t nla_offset,
 }
 
 NL_ATTR_FOR_EACH (nla, left,
-  (struct nlattr *) ((char *) msg->data + nla_offset),
+  (struct nlattr *) ofpbuf_forward_ref(msg, nla_offset),
   msg->size - nla_offset)
 {
 uint16_t type = nl_attr_type(nla);
@@ -777,9 +777,8 @@ nl_attr_find__(const struct nlattr *attrs, size_t size, 
uint16_t type)
 const struct nlattr *
 nl_attr_find(const struct ofpbuf *buf, size_t hdr_len, uint16_t type)
 {
-const uint8_t *start = (const uint8_t *) buf->data + hdr_len;
-return nl_attr_find__((const struct nlattr *) start, buf->size - hdr_len,
-  type);
+const struct nlattr *start = ofpbuf_forward_ref(buf, hdr_len);
+return nl_attr_find__(start, buf->size - hdr_len, type);
 }
 
 /* Returns the first Netlink attribute within 'nla' with the specified
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 3a6d7cc..2da06a2 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -744,7 +744,7 @@ oxm_put_match(struct ofpbuf *b, const struct match *match)
 match_len = nx_put_raw(b, true, match, cookie, cookie_mask) + siz

Re: [ovs-dev] [threads 07/23] xenserver: Make thread-safe.

[Alex Wang]

Looks good to me, thanks,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  vswitchd/xenserver.c |   23 +++
>  1 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/vswitchd/xenserver.c b/vswitchd/xenserver.c
> index 1f26e0d..bc57f0e 100644
> --- a/vswitchd/xenserver.c
> +++ b/vswitchd/xenserver.c
> @@ -17,6 +17,7 @@
>  #include "xenserver.h"
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -26,7 +27,11 @@
>
>  VLOG_DEFINE_THIS_MODULE(xenserver);
>
> -static char *
> +/* If running on a XenServer, the XenServer host UUID as a 36-character
> string,
> + * otherwise null. */
> +static char *host_uuid;
> +
> +static void
>  read_host_uuid(void)
>  {
>  static const char filename[] = "/etc/xensource-inventory";
> @@ -40,7 +45,7 @@ read_host_uuid(void)
>  } else {
>  VLOG_INFO("%s: open: %s", filename, ovs_strerror(errno));
>  }
> -return NULL;
> +return;
>  }
>
>  while (fgets(line, sizeof line, file)) {
> @@ -53,27 +58,21 @@ read_host_uuid(void)
>  if (strlen(line) == leader_len + uuid_len + trailer_len
>  && !memcmp(line, leader, leader_len)
>  && !memcmp(line + leader_len + uuid_len, trailer,
> trailer_len)) {
> -char *host_uuid = xmemdup0(line + leader_len, uuid_len);
> +host_uuid = xmemdup0(line + leader_len, uuid_len);
>  VLOG_INFO("running on XenServer, host-uuid %s", host_uuid);
>  fclose(file);
> -return host_uuid;
> +return;
>  }
>  }
>  fclose(file);
>  VLOG_ERR("%s: INSTALLATION_UUID not found", filename);
> -return NULL;
>  }
>
>  const char *
>  xenserver_get_host_uuid(void)
>  {
> -static char *host_uuid;
> -static bool inited;
> -
> -if (!inited) {
> -host_uuid = read_host_uuid();
> -inited = true;
> -}
> +static pthread_once_t once = PTHREAD_ONCE_INIT;
> +pthread_once(&once, read_host_uuid);
>  return host_uuid;
>  }
>
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 05/23] memory: Add note about threaded usage to comment.

[Alex Wang]

Looks good to me, thanks,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/memory.c |5 -
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/lib/memory.c b/lib/memory.c
> index 1137390..6c97e19 100644
> --- a/lib/memory.c
> +++ b/lib/memory.c
> @@ -51,7 +51,10 @@ static void memory_init(void);
>
>  /* Runs the memory monitor.
>   *
> - * The client should call memory_should_report() afterward. */
> + * The client should call memory_should_report() afterward.
> + *
> + * This function, and the remainder of this module's interface, should be
> + * called from only a single thread. */
>  void
>  memory_run(void)
>  {
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 04/23] lockfile: Make thread-safe.

[Alex Wang]

Looks good to me,

Also, for confirmation, we do not need to guarantee the thread safety
of coverage counters, since they are not meant to be perfectly accurate,
right?

Thanks,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/lockfile.c |   11 ++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/lib/lockfile.c b/lib/lockfile.c
> index 14e553d..50a4e0c 100644
> --- a/lib/lockfile.c
> +++ b/lib/lockfile.c
> @@ -27,6 +27,7 @@
>  #include "coverage.h"
>  #include "hash.h"
>  #include "hmap.h"
> +#include "ovs-thread.h"
>  #include "timeval.h"
>  #include "util.h"
>  #include "vlog.h"
> @@ -54,6 +55,9 @@ struct lockfile {
>   * once. */
>  static struct hmap lock_table = HMAP_INITIALIZER(&lock_table);
>
> +/* Protects 'lock_table'. */
> +static pthread_mutex_t lock_table_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
>  static void lockfile_unhash(struct lockfile *);
>  static int lockfile_try_lock(const char *name, pid_t *pidp,
>   struct lockfile **lockfilep);
> @@ -106,7 +110,9 @@ lockfile_lock(const char *file, struct lockfile
> **lockfilep)
>
>  lock_name = lockfile_name(file);
>
> +xpthread_mutex_lock(&lock_table_mutex);
>  error = lockfile_try_lock(lock_name, &pid, lockfilep);
> +xpthread_mutex_unlock(&lock_table_mutex);
>
>  if (error) {
>  COVERAGE_INC(lockfile_error);
> @@ -132,8 +138,11 @@ void
>  lockfile_unlock(struct lockfile *lockfile)
>  {
>  if (lockfile) {
> -COVERAGE_INC(lockfile_unlock);
> +xpthread_mutex_lock(&lock_table_mutex);
>  lockfile_unhash(lockfile);
> +xpthread_mutex_unlock(&lock_table_mutex);
> +
> +COVERAGE_INC(lockfile_unlock);
>  free(lockfile->name);
>  free(lockfile);
>  }
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 02/23] ofp-msgs: Make thread-safe.

[Alex Wang]

Looks good to me,

Want to ask question about "ovsthread_once_start()",
1. why do we have "ovsthread_once_start()" defined in header file?

2. if __check__ is defined, the macro function will override the
previously defined "ovsthread_once_start()" function. Right?

3. could you explain why you use "ovsthread_once_start()" rather than
"pthread_once()" like in other patches (since the init function does not
take
any input argument)?

Thanks,
Alex Wang,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/ofp-msgs.c |6 +-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/lib/ofp-msgs.c b/lib/ofp-msgs.c
> index af4178e..5e043d2 100644
> --- a/lib/ofp-msgs.c
> +++ b/lib/ofp-msgs.c
> @@ -23,6 +23,7 @@
>  #include "ofpbuf.h"
>  #include "openflow/nicira-ext.h"
>  #include "openflow/openflow.h"
> +#include "ovs-thread.h"
>  #include "vlog.h"
>
>  VLOG_DEFINE_THIS_MODULE(ofp_msgs);
> @@ -988,9 +989,10 @@ ofpraw_from_ofphdrs(enum ofpraw *raw, const struct
> ofphdrs *hdrs)
>  static void
>  ofpmsgs_init(void)
>  {
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>  const struct raw_info *info;
>
> -if (raw_instance_map.buckets) {
> +if (!ovsthread_once_start(&once)) {
>  return;
>  }
>
> @@ -1008,4 +1010,6 @@ ofpmsgs_init(void)
>  ofphdrs_hash(&inst->hdrs));
>  }
>  }
> +
> +ovsthread_once_done(&once);
>  }
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 03/23] jsonrpc: Make thread-safe.

[Alex Wang]

Looks good to me,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/jsonrpc.c |   12 ++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index b4bbc84..6c482c2 100644
> --- a/lib/jsonrpc.c
> +++ b/lib/jsonrpc.c
> @@ -26,6 +26,7 @@
>  #include "json.h"
>  #include "list.h"
>  #include "ofpbuf.h"
> +#include "ovs-thread.h"
>  #include "poll-loop.h"
>  #include "reconnect.h"
>  #include "stream.h"
> @@ -514,8 +515,15 @@ jsonrpc_create(enum jsonrpc_msg_type type, const char
> *method,
>  static struct json *
>  jsonrpc_create_id(void)
>  {
> -static unsigned int id;
> -return json_integer_create(id++);
> +static pthread_mutex_t mutex = PTHREAD_ADAPTIVE_MUTEX_INITIALIZER;
> +static unsigned int next_id;
> +unsigned int id;
> +
> +xpthread_mutex_lock(&mutex);
> +id = next_id++;
> +xpthread_mutex_unlock(&mutex);
> +
> +return json_integer_create(id);
>  }
>
>  struct jsonrpc_msg *
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 06/23] meta-flow: Make thread-safe.

[Alex Wang]

Looks good to me, thanks,



On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/meta-flow.c |   41 +++--
>  1 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 6b3b5eb..8f67b94 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -27,6 +27,7 @@
>  #include "dynamic-string.h"
>  #include "ofp-errors.h"
>  #include "ofp-util.h"
> +#include "ovs-thread.h"
>  #include "packets.h"
>  #include "random.h"
>  #include "shash.h"
> @@ -580,13 +581,17 @@ struct nxm_field {
>  };
>
>  /* Contains 'struct nxm_field's. */
> -static struct hmap all_fields = HMAP_INITIALIZER(&all_fields);
> +static struct hmap all_fields;
> +
> +/* Maps from an mf_field's 'name' or 'extra_name' to the mf_field. */
> +static struct shash mf_by_name;
>
>  /* Rate limit for parse errors.  These always indicate a bug in an
> OpenFlow
>   * controller and so there's not much point in showing a lot of them. */
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
>  const struct mf_field *mf_from_nxm_header__(uint32_t header);
> +static void nxm_init(void);
>
>  /* Returns the field with the given 'id'. */
>  const struct mf_field *
> @@ -601,19 +606,7 @@ mf_from_id(enum mf_field_id id)
>  const struct mf_field *
>  mf_from_name(const char *name)
>  {
> -static struct shash mf_by_name = SHASH_INITIALIZER(&mf_by_name);
> -
> -if (shash_is_empty(&mf_by_name)) {
> -const struct mf_field *mf;
> -
> -for (mf = mf_fields; mf < &mf_fields[MFF_N_IDS]; mf++) {
> -shash_add_once(&mf_by_name, mf->name, mf);
> -if (mf->extra_name) {
> -shash_add_once(&mf_by_name, mf->extra_name, mf);
> -}
> -}
> -}
> -
> +nxm_init();
>  return shash_find_data(&mf_by_name, name);
>  }
>
> @@ -641,24 +634,36 @@ nxm_init_add_field(const struct mf_field *mf,
> uint32_t header)
>  }
>
>  static void
> -nxm_init(void)
> +nxm_do_init(void)
>  {
>  const struct mf_field *mf;
>
> +hmap_init(&all_fields);
> +shash_init(&mf_by_name);
>  for (mf = mf_fields; mf < &mf_fields[MFF_N_IDS]; mf++) {
>  nxm_init_add_field(mf, mf->nxm_header);
>  if (mf->oxm_header != mf->nxm_header) {
>  nxm_init_add_field(mf, mf->oxm_header);
>  }
> +
> +shash_add_once(&mf_by_name, mf->name, mf);
> +if (mf->extra_name) {
> +shash_add_once(&mf_by_name, mf->extra_name, mf);
> +}
>  }
>  }
>
> +static void
> +nxm_init(void)
> +{
> +static pthread_once_t once = PTHREAD_ONCE_INIT;
> +pthread_once(&once, nxm_do_init);
> +}
> +
>  const struct mf_field *
>  mf_from_nxm_header(uint32_t header)
>  {
> -if (hmap_is_empty(&all_fields)) {
> -nxm_init();
> -}
> +nxm_init();
>  return mf_from_nxm_header__(header);
>  }
>
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Patch for MPLS

[Jesse Gross]

On Sat, Jul 20, 2013 at 2:14 AM, msj  wrote:
> Hi,
>  It is my honour to communicate with you .
>  I want to ask you one question : I notice that the patch for MPLS does not
> change too much in part of include/openflow , and it only contains some change
> in nicira-ext.h .
>  well ,does that mean you did not expand the openflow protocol ?
>  then ,how can the patch for MPLS make openvswitch communicate with POX or
> other controller ?

There patches are over a year old and were never merged. Please look
at the code in the tree.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 08/23] dirs: Make thread-safe.

[Alex Wang]

Looks good to me,

One question, why do we have the "#line directive" in "lib/dirs.c.in"?
We have already created "dirs.c" in "lib/automake.mk". What is the
use of "#line directive" here?


Thanks,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/dirs.c.in |   37 +
>  1 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/lib/dirs.c.in b/lib/dirs.c.in
> index 658a74b..85c49ee 100644
> --- a/lib/dirs.c.in
> +++ b/lib/dirs.c.in
> @@ -1,6 +1,6 @@
>  #line 2 "@srcdir@/lib/dirs.c.in"
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -18,22 +18,25 @@
>  #include 
>  #include "dirs.h"
>  #include 
> +#include "ovs-thread.h"
>  #include "util.h"
>
>  struct directory {
>  const char *value;  /* Actual value; NULL if not yet
> determined. */
>  const char *default_value;  /* Default value. */
>  const char *var_name;   /* Environment variable to override
> default. */
> +struct ovsthread_once once; /* Ensures 'value' gets initialized once.
> */
>  };
>
>  static const char *
>  get_dir(struct directory *d)
>  {
> -if (!d->value) {
> +if (ovsthread_once_start(&d->once)) {
>  d->value = getenv(d->var_name);
>  if (!d->value || !d->value[0]) {
>  d->value = d->default_value;
>  }
> +ovsthread_once_done(&d->once);
>  }
>  return d->value;
>  }
> @@ -41,36 +44,50 @@ get_dir(struct directory *d)
>  const char *
>  ovs_sysconfdir(void)
>  {
> -static struct directory d = { NULL, @sysconfdir@, "OVS_SYSCONFDIR" };
> +static struct directory d = {
> +NULL, @sysconfdir@, "OVS_SYSCONFDIR", OVSTHREAD_ONCE_INITIALIZER
> +};
> +
>  return get_dir(&d);
>  }
>
>  const char *
>  ovs_pkgdatadir(void)
>  {
> -static struct directory d = { NULL, @pkgdatadir@, "OVS_PKGDATADIR" };
> +static struct directory d = {
> +NULL, @pkgdatadir@, "OVS_PKGDATADIR", OVSTHREAD_ONCE_INITIALIZER
> +};
> +
>  return get_dir(&d);
>  }
>
>  const char *
>  ovs_rundir(void)
>  {
> -static struct directory d = { NULL, @RUNDIR@, "OVS_RUNDIR" };
> +static struct directory d = {
> +NULL, @RUNDIR@, "OVS_RUNDIR", OVSTHREAD_ONCE_INITIALIZER
> +};
> +
>  return get_dir(&d);
>  }
>
>  const char *
>  ovs_logdir(void)
>  {
> -static struct directory d = { NULL, @LOGDIR@, "OVS_LOGDIR" };
> +static struct directory d = {
> +NULL, @LOGDIR@, "OVS_LOGDIR", OVSTHREAD_ONCE_INITIALIZER
> +};
> +
>  return get_dir(&d);
>  }
>
>  const char *
>  ovs_dbdir(void)
>  {
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>  static const char *dbdir;
> -if (!dbdir) {
> +
> +if (ovsthread_once_start(&once)) {
>  dbdir = getenv("OVS_DBDIR");
>  if (!dbdir || !dbdir[0]) {
>  char *sysconfdir = getenv("OVS_SYSCONFDIR");
> @@ -79,6 +96,7 @@ ovs_dbdir(void)
>   ? xasprintf("%s/openvswitch", sysconfdir)
>   : @DBDIR@);
>  }
> +ovsthread_once_done(&once);
>  }
>  return dbdir;
>  }
> @@ -86,6 +104,9 @@ ovs_dbdir(void)
>  const char *
>  ovs_bindir(void)
>  {
> -static struct directory d = { NULL, @bindir@, "OVS_BINDIR" };
> +static struct directory d = {
> +NULL, @bindir@, "OVS_BINDIR", OVSTHREAD_ONCE_INITIALIZER
> +};
> +
>  return get_dir(&d);
>  }
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 08/23] dirs: Make thread-safe.

[Alex Wang]

Seems I understand now,

Is that for directing all error messages to corresponding lines in
"lib/dirs.c.in"?


On Mon, Jul 22, 2013 at 10:20 AM, Alex Wang  wrote:

> Looks good to me,
>
> One question, why do we have the "#line directive" in "lib/dirs.c.in"?
> We have already created "dirs.c" in "lib/automake.mk". What is the
> use of "#line directive" here?
>
>
> Thanks,
>
>
> On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:
>
>> Signed-off-by: Ben Pfaff 
>> ---
>>  lib/dirs.c.in |   37 +
>>  1 files changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/dirs.c.in b/lib/dirs.c.in
>> index 658a74b..85c49ee 100644
>> --- a/lib/dirs.c.in
>> +++ b/lib/dirs.c.in
>> @@ -1,6 +1,6 @@
>>  #line 2 "@srcdir@/lib/dirs.c.in"
>>  /*
>> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
>> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>   * you may not use this file except in compliance with the License.
>> @@ -18,22 +18,25 @@
>>  #include 
>>  #include "dirs.h"
>>  #include 
>> +#include "ovs-thread.h"
>>  #include "util.h"
>>
>>  struct directory {
>>  const char *value;  /* Actual value; NULL if not yet
>> determined. */
>>  const char *default_value;  /* Default value. */
>>  const char *var_name;   /* Environment variable to override
>> default. */
>> +struct ovsthread_once once; /* Ensures 'value' gets initialized
>> once. */
>>  };
>>
>>  static const char *
>>  get_dir(struct directory *d)
>>  {
>> -if (!d->value) {
>> +if (ovsthread_once_start(&d->once)) {
>>  d->value = getenv(d->var_name);
>>  if (!d->value || !d->value[0]) {
>>  d->value = d->default_value;
>>  }
>> +ovsthread_once_done(&d->once);
>>  }
>>  return d->value;
>>  }
>> @@ -41,36 +44,50 @@ get_dir(struct directory *d)
>>  const char *
>>  ovs_sysconfdir(void)
>>  {
>> -static struct directory d = { NULL, @sysconfdir@, "OVS_SYSCONFDIR"
>> };
>> +static struct directory d = {
>> +NULL, @sysconfdir@, "OVS_SYSCONFDIR", OVSTHREAD_ONCE_INITIALIZER
>> +};
>> +
>>  return get_dir(&d);
>>  }
>>
>>  const char *
>>  ovs_pkgdatadir(void)
>>  {
>> -static struct directory d = { NULL, @pkgdatadir@, "OVS_PKGDATADIR"
>> };
>> +static struct directory d = {
>> +NULL, @pkgdatadir@, "OVS_PKGDATADIR", OVSTHREAD_ONCE_INITIALIZER
>> +};
>> +
>>  return get_dir(&d);
>>  }
>>
>>  const char *
>>  ovs_rundir(void)
>>  {
>> -static struct directory d = { NULL, @RUNDIR@, "OVS_RUNDIR" };
>> +static struct directory d = {
>> +NULL, @RUNDIR@, "OVS_RUNDIR", OVSTHREAD_ONCE_INITIALIZER
>> +};
>> +
>>  return get_dir(&d);
>>  }
>>
>>  const char *
>>  ovs_logdir(void)
>>  {
>> -static struct directory d = { NULL, @LOGDIR@, "OVS_LOGDIR" };
>> +static struct directory d = {
>> +NULL, @LOGDIR@, "OVS_LOGDIR", OVSTHREAD_ONCE_INITIALIZER
>> +};
>> +
>>  return get_dir(&d);
>>  }
>>
>>  const char *
>>  ovs_dbdir(void)
>>  {
>> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>>  static const char *dbdir;
>> -if (!dbdir) {
>> +
>> +if (ovsthread_once_start(&once)) {
>>  dbdir = getenv("OVS_DBDIR");
>>  if (!dbdir || !dbdir[0]) {
>>  char *sysconfdir = getenv("OVS_SYSCONFDIR");
>> @@ -79,6 +96,7 @@ ovs_dbdir(void)
>>   ? xasprintf("%s/openvswitch", sysconfdir)
>>   : @DBDIR@);
>>  }
>> +ovsthread_once_done(&once);
>>  }
>>  return dbdir;
>>  }
>> @@ -86,6 +104,9 @@ ovs_dbdir(void)
>>  const char *
>>  ovs_bindir(void)
>>  {
>> -static struct directory d = { NULL, @bindir@, "OVS_BINDIR" };
>> +static struct directory d = {
>> +NULL, @bindir@, "OVS_BINDIR", OVSTHREAD_ONCE_INITIALIZER
>> +};
>> +
>>  return get_dir(&d);
>>  }
>> --
>> 1.7.2.5
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 04/23] lockfile: Make thread-safe.

[Alex Wang]

Thanks, it makes sense,


On Mon, Jul 22, 2013 at 10:26 AM, Ben Pfaff  wrote:

> On Mon, Jul 22, 2013 at 09:17:37AM -0700, Alex Wang wrote:
> > Also, for confirmation, we do not need to guarantee the thread safety
> > of coverage counters, since they are not meant to be perfectly accurate,
> > right?
>
> The coverage counters should probably become per-thread.  I haven't
> done anything about them yet because they'll do minimal harm.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Conditionally define skb_unclone in datapath compat code

[Jesse Gross]

On Mon, Jul 22, 2013 at 7:38 AM, Kyle Mestery  wrote:
> Recent versions of Fedora have skb_unclone included in their
> kernels. This patch adds a conditional check into the compat directory so as
> not to error out by defining it twice. This allows the latest OVS kernel
> module to build on Fedora 19.
>
> Signed-off-by: Kyle Mestery 

Applied, thanks.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 02/23] ofp-msgs: Make thread-safe.

[Alex Wang]

Thanks Ben for the explanation,


On Mon, Jul 22, 2013 at 10:23 AM, Ben Pfaff  wrote:

> On Mon, Jul 22, 2013 at 09:10:52AM -0700, Alex Wang wrote:
> > Looks good to me,
> >
> > Want to ask question about "ovsthread_once_start()",
> > 1. why do we have "ovsthread_once_start()" defined in header file?
>
> The "fast path" for using ovsthread_once_start() is just a test and
> conditional jump.  The alternative is a function call followed by a
> test and a conditional jump.  I decided to use the former as an
> optimization; it might have been premature.


Still not clear about, why there is no function call when defining the
function in header file. This is different to having "inline" function,
right?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 08/23] dirs: Make thread-safe.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 10:20:54AM -0700, Alex Wang wrote:
> One question, why do we have the "#line directive" in "lib/dirs.c.in"?
> We have already created "dirs.c" in "lib/automake.mk". What is the
> use of "#line directive" here?

dirs.c gets generated by search-and-replace on dirs.c.in.  If there's
a compiler warning or error we want it reported against dirs.c.in so
that the developer doesn't waste time fixing the generated file, and
that's what the #line directive does.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 08/23] dirs: Make thread-safe.

[Alex Wang]

Thanks Ben for the explanation,


On Mon, Jul 22, 2013 at 10:37 AM, Ben Pfaff  wrote:

> Yes.
>
> On Mon, Jul 22, 2013 at 10:25:09AM -0700, Alex Wang wrote:
> > Seems I understand now,
> >
> > Is that for directing all error messages to corresponding lines in
> > "lib/dirs.c.in"?
> >
> >
> > On Mon, Jul 22, 2013 at 10:20 AM, Alex Wang  wrote:
> >
> > > Looks good to me,
> > >
> > > One question, why do we have the "#line directive" in "lib/dirs.c.in"?
> > > We have already created "dirs.c" in "lib/automake.mk". What is the
> > > use of "#line directive" here?
> > >
> > >
> > > Thanks,
> > >
> > >
> > > On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:
> > >
> > >> Signed-off-by: Ben Pfaff 
> > >> ---
> > >>  lib/dirs.c.in |   37 +
> > >>  1 files changed, 29 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/lib/dirs.c.in b/lib/dirs.c.in
> > >> index 658a74b..85c49ee 100644
> > >> --- a/lib/dirs.c.in
> > >> +++ b/lib/dirs.c.in
> > >> @@ -1,6 +1,6 @@
> > >>  #line 2 "@srcdir@/lib/dirs.c.in"
> > >>  /*
> > >> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> > >> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> > >>   *
> > >>   * Licensed under the Apache License, Version 2.0 (the "License");
> > >>   * you may not use this file except in compliance with the License.
> > >> @@ -18,22 +18,25 @@
> > >>  #include 
> > >>  #include "dirs.h"
> > >>  #include 
> > >> +#include "ovs-thread.h"
> > >>  #include "util.h"
> > >>
> > >>  struct directory {
> > >>  const char *value;  /* Actual value; NULL if not yet
> > >> determined. */
> > >>  const char *default_value;  /* Default value. */
> > >>  const char *var_name;   /* Environment variable to override
> > >> default. */
> > >> +struct ovsthread_once once; /* Ensures 'value' gets initialized
> > >> once. */
> > >>  };
> > >>
> > >>  static const char *
> > >>  get_dir(struct directory *d)
> > >>  {
> > >> -if (!d->value) {
> > >> +if (ovsthread_once_start(&d->once)) {
> > >>  d->value = getenv(d->var_name);
> > >>  if (!d->value || !d->value[0]) {
> > >>  d->value = d->default_value;
> > >>  }
> > >> +ovsthread_once_done(&d->once);
> > >>  }
> > >>  return d->value;
> > >>  }
> > >> @@ -41,36 +44,50 @@ get_dir(struct directory *d)
> > >>  const char *
> > >>  ovs_sysconfdir(void)
> > >>  {
> > >> -static struct directory d = { NULL, @sysconfdir@,
> "OVS_SYSCONFDIR"
> > >> };
> > >> +static struct directory d = {
> > >> +NULL, @sysconfdir@, "OVS_SYSCONFDIR",
> OVSTHREAD_ONCE_INITIALIZER
> > >> +};
> > >> +
> > >>  return get_dir(&d);
> > >>  }
> > >>
> > >>  const char *
> > >>  ovs_pkgdatadir(void)
> > >>  {
> > >> -static struct directory d = { NULL, @pkgdatadir@,
> "OVS_PKGDATADIR"
> > >> };
> > >> +static struct directory d = {
> > >> +NULL, @pkgdatadir@, "OVS_PKGDATADIR",
> OVSTHREAD_ONCE_INITIALIZER
> > >> +};
> > >> +
> > >>  return get_dir(&d);
> > >>  }
> > >>
> > >>  const char *
> > >>  ovs_rundir(void)
> > >>  {
> > >> -static struct directory d = { NULL, @RUNDIR@, "OVS_RUNDIR" };
> > >> +static struct directory d = {
> > >> +NULL, @RUNDIR@, "OVS_RUNDIR", OVSTHREAD_ONCE_INITIALIZER
> > >> +};
> > >> +
> > >>  return get_dir(&d);
> > >>  }
> > >>
> > >>  const char *
> > >>  ovs_logdir(void)
> > >>  {
> > >> -static struct directory d = { NULL, @LOGDIR@, "OVS_LOGDIR" };
> > >> +static struct directory d = {
> > >> +NULL, @LOGDIR@, "OVS_LOGDIR", OVSTHREAD_ONCE_INITIALIZER
> > >> +};
> > >> +
> > >>  return get_dir(&d);
> > >>  }
> > >>
> > >>  const char *
> > >>  ovs_dbdir(void)
> > >>  {
> > >> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> > >>  static const char *dbdir;
> > >> -if (!dbdir) {
> > >> +
> > >> +if (ovsthread_once_start(&once)) {
> > >>  dbdir = getenv("OVS_DBDIR");
> > >>  if (!dbdir || !dbdir[0]) {
> > >>  char *sysconfdir = getenv("OVS_SYSCONFDIR");
> > >> @@ -79,6 +96,7 @@ ovs_dbdir(void)
> > >>   ? xasprintf("%s/openvswitch", sysconfdir)
> > >>   : @DBDIR@);
> > >>  }
> > >> +ovsthread_once_done(&once);
> > >>  }
> > >>  return dbdir;
> > >>  }
> > >> @@ -86,6 +104,9 @@ ovs_dbdir(void)
> > >>  const char *
> > >>  ovs_bindir(void)
> > >>  {
> > >> -static struct directory d = { NULL, @bindir@, "OVS_BINDIR" };
> > >> +static struct directory d = {
> > >> +NULL, @bindir@, "OVS_BINDIR", OVSTHREAD_ONCE_INITIALIZER
> > >> +};
> > >> +
> > >>  return get_dir(&d);
> > >>  }
> > >> --
> > >> 1.7.2.5
> > >>
> > >> ___
> > >> dev mailing list
> > >> dev@openvswitch.org
> > >> http://openvswitch.org/mailman/listinfo/dev
> > >>
> > >
> > >
>
___
dev mail

Re: [ovs-dev] [threads 02/23] ofp-msgs: Make thread-safe.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 10:35:27AM -0700, Alex Wang wrote:
> On Mon, Jul 22, 2013 at 10:23 AM, Ben Pfaff  wrote:
> 
> > On Mon, Jul 22, 2013 at 09:10:52AM -0700, Alex Wang wrote:
> > > Looks good to me,
> > >
> > > Want to ask question about "ovsthread_once_start()",
> > > 1. why do we have "ovsthread_once_start()" defined in header file?
> >
> > The "fast path" for using ovsthread_once_start() is just a test and
> > conditional jump.  The alternative is a function call followed by a
> > test and a conditional jump.  I decided to use the former as an
> > optimization; it might have been premature.
> 
> Still not clear about, why there is no function call when defining the
> function in header file. This is different to having "inline" function,
> right?

ovsthread_once_start() is defined as an inline function.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 08/23] dirs: Make thread-safe.

[Ben Pfaff]

Yes.

On Mon, Jul 22, 2013 at 10:25:09AM -0700, Alex Wang wrote:
> Seems I understand now,
> 
> Is that for directing all error messages to corresponding lines in
> "lib/dirs.c.in"?
> 
> 
> On Mon, Jul 22, 2013 at 10:20 AM, Alex Wang  wrote:
> 
> > Looks good to me,
> >
> > One question, why do we have the "#line directive" in "lib/dirs.c.in"?
> > We have already created "dirs.c" in "lib/automake.mk". What is the
> > use of "#line directive" here?
> >
> >
> > Thanks,
> >
> >
> > On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:
> >
> >> Signed-off-by: Ben Pfaff 
> >> ---
> >>  lib/dirs.c.in |   37 +
> >>  1 files changed, 29 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/dirs.c.in b/lib/dirs.c.in
> >> index 658a74b..85c49ee 100644
> >> --- a/lib/dirs.c.in
> >> +++ b/lib/dirs.c.in
> >> @@ -1,6 +1,6 @@
> >>  #line 2 "@srcdir@/lib/dirs.c.in"
> >>  /*
> >> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> >> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -18,22 +18,25 @@
> >>  #include 
> >>  #include "dirs.h"
> >>  #include 
> >> +#include "ovs-thread.h"
> >>  #include "util.h"
> >>
> >>  struct directory {
> >>  const char *value;  /* Actual value; NULL if not yet
> >> determined. */
> >>  const char *default_value;  /* Default value. */
> >>  const char *var_name;   /* Environment variable to override
> >> default. */
> >> +struct ovsthread_once once; /* Ensures 'value' gets initialized
> >> once. */
> >>  };
> >>
> >>  static const char *
> >>  get_dir(struct directory *d)
> >>  {
> >> -if (!d->value) {
> >> +if (ovsthread_once_start(&d->once)) {
> >>  d->value = getenv(d->var_name);
> >>  if (!d->value || !d->value[0]) {
> >>  d->value = d->default_value;
> >>  }
> >> +ovsthread_once_done(&d->once);
> >>  }
> >>  return d->value;
> >>  }
> >> @@ -41,36 +44,50 @@ get_dir(struct directory *d)
> >>  const char *
> >>  ovs_sysconfdir(void)
> >>  {
> >> -static struct directory d = { NULL, @sysconfdir@, "OVS_SYSCONFDIR"
> >> };
> >> +static struct directory d = {
> >> +NULL, @sysconfdir@, "OVS_SYSCONFDIR", OVSTHREAD_ONCE_INITIALIZER
> >> +};
> >> +
> >>  return get_dir(&d);
> >>  }
> >>
> >>  const char *
> >>  ovs_pkgdatadir(void)
> >>  {
> >> -static struct directory d = { NULL, @pkgdatadir@, "OVS_PKGDATADIR"
> >> };
> >> +static struct directory d = {
> >> +NULL, @pkgdatadir@, "OVS_PKGDATADIR", OVSTHREAD_ONCE_INITIALIZER
> >> +};
> >> +
> >>  return get_dir(&d);
> >>  }
> >>
> >>  const char *
> >>  ovs_rundir(void)
> >>  {
> >> -static struct directory d = { NULL, @RUNDIR@, "OVS_RUNDIR" };
> >> +static struct directory d = {
> >> +NULL, @RUNDIR@, "OVS_RUNDIR", OVSTHREAD_ONCE_INITIALIZER
> >> +};
> >> +
> >>  return get_dir(&d);
> >>  }
> >>
> >>  const char *
> >>  ovs_logdir(void)
> >>  {
> >> -static struct directory d = { NULL, @LOGDIR@, "OVS_LOGDIR" };
> >> +static struct directory d = {
> >> +NULL, @LOGDIR@, "OVS_LOGDIR", OVSTHREAD_ONCE_INITIALIZER
> >> +};
> >> +
> >>  return get_dir(&d);
> >>  }
> >>
> >>  const char *
> >>  ovs_dbdir(void)
> >>  {
> >> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> >>  static const char *dbdir;
> >> -if (!dbdir) {
> >> +
> >> +if (ovsthread_once_start(&once)) {
> >>  dbdir = getenv("OVS_DBDIR");
> >>  if (!dbdir || !dbdir[0]) {
> >>  char *sysconfdir = getenv("OVS_SYSCONFDIR");
> >> @@ -79,6 +96,7 @@ ovs_dbdir(void)
> >>   ? xasprintf("%s/openvswitch", sysconfdir)
> >>   : @DBDIR@);
> >>  }
> >> +ovsthread_once_done(&once);
> >>  }
> >>  return dbdir;
> >>  }
> >> @@ -86,6 +104,9 @@ ovs_dbdir(void)
> >>  const char *
> >>  ovs_bindir(void)
> >>  {
> >> -static struct directory d = { NULL, @bindir@, "OVS_BINDIR" };
> >> +static struct directory d = {
> >> +NULL, @bindir@, "OVS_BINDIR", OVSTHREAD_ONCE_INITIALIZER
> >> +};
> >> +
> >>  return get_dir(&d);
> >>  }
> >> --
> >> 1.7.2.5
> >>
> >> ___
> >> dev mailing list
> >> dev@openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
> >>
> >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 04/23] lockfile: Make thread-safe.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 09:17:37AM -0700, Alex Wang wrote:
> Also, for confirmation, we do not need to guarantee the thread safety
> of coverage counters, since they are not meant to be perfectly accurate,
> right?

The coverage counters should probably become per-thread.  I haven't
done anything about them yet because they'll do minimal harm.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 02/23] ofp-msgs: Make thread-safe.

[Alex Wang]

Oh! How could I not see the "inline". Sorry for the nonsense,


On Mon, Jul 22, 2013 at 10:39 AM, Ben Pfaff  wrote:

> On Mon, Jul 22, 2013 at 10:35:27AM -0700, Alex Wang wrote:
> > On Mon, Jul 22, 2013 at 10:23 AM, Ben Pfaff  wrote:
> >
> > > On Mon, Jul 22, 2013 at 09:10:52AM -0700, Alex Wang wrote:
> > > > Looks good to me,
> > > >
> > > > Want to ask question about "ovsthread_once_start()",
> > > > 1. why do we have "ovsthread_once_start()" defined in header file?
> > >
> > > The "fast path" for using ovsthread_once_start() is just a test and
> > > conditional jump.  The alternative is a function call followed by a
> > > test and a conditional jump.  I decided to use the former as an
> > > optimization; it might have been premature.
> >
> > Still not clear about, why there is no function call when defining the
> > function in header file. This is different to having "inline" function,
> > right?
>
> ovsthread_once_start() is defined as an inline function.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 09/23] dpif: Serialize initialization.

[Alex Wang]

Looks good to me, thanks,


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/dpif-linux.c |7 +--
>  lib/dpif.c   |6 +++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index 958873c..831da3b 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -1440,9 +1440,10 @@ const struct dpif_class dpif_linux_class = {
>  static int
>  dpif_linux_init(void)
>  {
> -static int error = -1;
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +static int error;
>
> -if (error < 0) {
> +if (ovsthread_once_start(&once)) {
>  unsigned int ovs_vport_mcgroup;
>
>  error = nl_lookup_genl_family(OVS_DATAPATH_FAMILY,
> @@ -1472,6 +1473,8 @@ dpif_linux_init(void)
>  nln = nln_create(NETLINK_GENERIC, ovs_vport_mcgroup,
>   dpif_linux_nln_parse, &vport);
>  }
> +
> +ovsthread_once_done(&once);
>  }
>
>  return error;
> diff --git a/lib/dpif.c b/lib/dpif.c
> index e793262..4878aac 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -97,15 +97,15 @@ static void log_execute_message(struct dpif *, const
> struct dpif_execute *,
>  static void
>  dp_initialize(void)
>  {
> -static int status = -1;
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>
> -if (status < 0) {
> +if (ovsthread_once_start(&once)) {
>  int i;
>
> -status = 0;
>  for (i = 0; i < ARRAY_SIZE(base_dpif_classes); i++) {
>  dp_register_provider(base_dpif_classes[i]);
>  }
> +ovsthread_once_done(&once);
>  }
>  }
>
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 10/23] ovsdb-data: Make ovsdb_atom_default() thread-safe.

[Alex Wang]

Looks good to me, thanks,

Review for patch 1/23~10/23 is complete.


On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  lib/ovsdb-data.c |7 ---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> index 7ec7694..ade1971 100644
> --- a/lib/ovsdb-data.c
> +++ b/lib/ovsdb-data.c
> @@ -24,6 +24,7 @@
>
>  #include "dynamic-string.h"
>  #include "hash.h"
> +#include "ovs-thread.h"
>  #include "ovsdb-error.h"
>  #include "ovsdb-parser.h"
>  #include "json.h"
> @@ -94,9 +95,9 @@ const union ovsdb_atom *
>  ovsdb_atom_default(enum ovsdb_atomic_type type)
>  {
>  static union ovsdb_atom default_atoms[OVSDB_N_TYPES];
> -static bool inited;
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>
> -if (!inited) {
> +if (ovsthread_once_start(&once)) {
>  int i;
>
>  for (i = 0; i < OVSDB_N_TYPES; i++) {
> @@ -104,7 +105,7 @@ ovsdb_atom_default(enum ovsdb_atomic_type type)
>  ovsdb_atom_init_default(&default_atoms[i], i);
>  }
>  }
> -inited = true;
> +ovsthread_once_done(&once);
>  }
>
>  ovs_assert(ovsdb_atomic_type_is_valid(type));
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 10/23] ovsdb-data: Make ovsdb_atom_default() thread-safe.

[Ben Pfaff]

Thanks for the reviews.  I applied these to master.

On Mon, Jul 22, 2013 at 10:51:43AM -0700, Alex Wang wrote:
> Looks good to me, thanks,
> 
> Review for patch 1/23~10/23 is complete.
> 
> 
> On Thu, Jul 18, 2013 at 4:15 PM, Ben Pfaff  wrote:
> 
> > Signed-off-by: Ben Pfaff 
> > ---
> >  lib/ovsdb-data.c |7 ---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> > index 7ec7694..ade1971 100644
> > --- a/lib/ovsdb-data.c
> > +++ b/lib/ovsdb-data.c
> > @@ -24,6 +24,7 @@
> >
> >  #include "dynamic-string.h"
> >  #include "hash.h"
> > +#include "ovs-thread.h"
> >  #include "ovsdb-error.h"
> >  #include "ovsdb-parser.h"
> >  #include "json.h"
> > @@ -94,9 +95,9 @@ const union ovsdb_atom *
> >  ovsdb_atom_default(enum ovsdb_atomic_type type)
> >  {
> >  static union ovsdb_atom default_atoms[OVSDB_N_TYPES];
> > -static bool inited;
> > +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> >
> > -if (!inited) {
> > +if (ovsthread_once_start(&once)) {
> >  int i;
> >
> >  for (i = 0; i < OVSDB_N_TYPES; i++) {
> > @@ -104,7 +105,7 @@ ovsdb_atom_default(enum ovsdb_atomic_type type)
> >  ovsdb_atom_init_default(&default_atoms[i], i);
> >  }
> >  }
> > -inited = true;
> > +ovsthread_once_done(&once);
> >  }
> >
> >  ovs_assert(ovsdb_atomic_type_is_valid(type));
> > --
> > 1.7.2.5
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] sparse warning

[Ben Pfaff]

I'm getting the following warning on current master (commit
dc0d542d5254c1e6 "datapath: Conditionally define skb_unclone in
datapath compat code"):

datapath/flow.c:1051:32: error: incompatible types in comparison
expression (different address spaces)

That's in ovs_masked_flow_lookup():

hlist_for_each_entry_rcu(flow, head, hash_node[table->node_ver]) {
1051 -> if (flow->mask == mask &&
__flow_cmp_key(flow, &masked_key, key_start, key_len))
return flow;
}

Last change to this line was:

commit a99c219c5d08a93f1158fcd1df0f4a429bc9062f
Author: Andy Zhou   Fri Jul 19 11:11:24 2013
Committer:  Pravin B Shelar   Fri Jul 19 07:33:42 2013

datapath: Add mask check during flow lookup

A mega flow matches when the masked key matches and the mask applied
is the same as the mask used to create the mega flow.

This patch adds the implementation of the second match condition
mentioned above. Without this fix, mega flow lookup may result false
match.

Bug #18584

Signed-off-by: Andy Zhou 
Signed-off-by: Pravin B Shelar 

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2.35 0/6] MPLS actions and matches

[Ben Pfaff]

Jesse, I'm leaving this series alone until you give some feedback on
the kernel side of things.  Then I'm happy to review (and commit when
appropriate) user space patches.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] datapath: Conditionally define skb_unclone in datapath compat code

[Kyle Mestery]

Recent versions of Fedora have skb_unclone included in their
kernels. This patch adds a conditional check into the compat directory so as
not to error out by defining it twice. This allows the latest OVS kernel
module to build on Fedora 19.

Signed-off-by: Kyle Mestery 
---
 acinclude.m4 | 1 +
 datapath/linux/compat/include/linux/skbuff.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/acinclude.m4 b/acinclude.m4
index 717c681..30a4dc6 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -256,6 +256,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [consume_skb])
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_frag_page])
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_reset_mac_len])
+  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_unclone])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/string.h], [kmemdup], [],
   [OVS_GREP_IFELSE([$KSRC/include/linux/slab.h], [kmemdup])])
diff --git a/datapath/linux/compat/include/linux/skbuff.h 
b/datapath/linux/compat/include/linux/skbuff.h
index c9c103d..461e07c 100644
--- a/datapath/linux/compat/include/linux/skbuff.h
+++ b/datapath/linux/compat/include/linux/skbuff.h
@@ -252,6 +252,7 @@ static inline void skb_reset_mac_len(struct sk_buff *skb)
 }
 #endif
 
+#ifndef HAVE_SKB_UNCLONE
 static inline int skb_unclone(struct sk_buff *skb, gfp_t pri)
 {
might_sleep_if(pri & __GFP_WAIT);
@@ -261,6 +262,7 @@ static inline int skb_unclone(struct sk_buff *skb, gfp_t 
pri)
 
return 0;
 }
+#endif
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37)
 extern u32 __skb_get_rxhash(struct sk_buff *skb);
-- 
1.8.3.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 09:19:57AM -0700, Alex Wang wrote:
> This commit makes macro function "ASSIGN_CONTAINER()" evaluates
> to "(void)0". This is to avoid the 'clang' warning: "expression
> result unused", since most of time, the final evaluated value
> is not used.
> 
> Signed-off-by: Alex Wang 

I think you missed a case in heap.h.  I had to apply the following to
avoid a pile of GCC warnings because in "a ? b : c" the b expression
had type void and the c expression had type int (value 1).  Can you
fold that in and verify that clang accepts it too?

diff --git a/lib/heap.h b/lib/heap.h
index 9326d79..5c07e04 100644
--- a/lib/heap.h
+++ b/lib/heap.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012 Nicira, Inc.
+ * Copyright (c) 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -69,13 +69,13 @@ void heap_rebuild(struct heap *);
 #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)   \
 for (((HEAP)->n > 0 \
   ? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)\
-  : ((NODE) = NULL, 1));\
+  : ((NODE) = NULL, (void) 0));   \
  (NODE) != NULL;\
  ((NODE)->MEMBER.idx < (HEAP)->n\
   ? ASSIGN_CONTAINER(NODE,  \
  (HEAP)->array[(NODE)->MEMBER.idx + 1], \
  MEMBER)\
-  : ((NODE) = NULL, 1)))
+  : ((NODE) = NULL, (void) 0)))
 
 /* Returns the index of the node that is the parent of the node with the given
  * 'idx' within a heap. */

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/3] clang: Fix segfault in unit tests.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 09:19:56AM -0700, Alex Wang wrote:
> It seems that 'clang' compiler applies strict protection on pointer
> dereference. And it causes unexpected execution in macro functions
> like "HMAP_FOR_EACH()" and unit test failures. This commit fixes
> this issue and pass all unit tests.
> 
> Co-authored-by: Ethan Jackson 
> Signed-off-by: Alex Wang 

Applied to master, thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2.35 0/6] MPLS actions and matches

[Jesse Gross]

On Mon, Jul 22, 2013 at 11:13 AM, Ben Pfaff  wrote:
> Jesse, I'm leaving this series alone until you give some feedback on
> the kernel side of things.  Then I'm happy to review (and commit when
> appropriate) user space patches.

OK, I'll look at the last patch first then.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Alex Wang]

Thanks Ben,

I'll recheck that.


On Mon, Jul 22, 2013 at 11:30 AM, Ben Pfaff  wrote:

> On Mon, Jul 22, 2013 at 09:19:57AM -0700, Alex Wang wrote:
> > This commit makes macro function "ASSIGN_CONTAINER()" evaluates
> > to "(void)0". This is to avoid the 'clang' warning: "expression
> > result unused", since most of time, the final evaluated value
> > is not used.
> >
> > Signed-off-by: Alex Wang 
>
> I think you missed a case in heap.h.  I had to apply the following to
> avoid a pile of GCC warnings because in "a ? b : c" the b expression
> had type void and the c expression had type int (value 1).  Can you
> fold that in and verify that clang accepts it too?
>
> diff --git a/lib/heap.h b/lib/heap.h
> index 9326d79..5c07e04 100644
> --- a/lib/heap.h
> +++ b/lib/heap.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2012 Nicira, Inc.
> + * Copyright (c) 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -69,13 +69,13 @@ void heap_rebuild(struct heap *);
>  #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)   \
>  for (((HEAP)->n > 0 \
>? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)\
> -  : ((NODE) = NULL, 1));\
> +  : ((NODE) = NULL, (void) 0));   \
>   (NODE) != NULL;\
>   ((NODE)->MEMBER.idx < (HEAP)->n\
>? ASSIGN_CONTAINER(NODE,  \
>   (HEAP)->array[(NODE)->MEMBER.idx + 1], \
>   MEMBER)\
> -  : ((NODE) = NULL, 1)))
> +  : ((NODE) = NULL, (void) 0)))
>
>  /* Returns the index of the node that is the parent of the node with the
> given
>   * 'idx' within a heap. */
>
> Thanks,
>
> Ben.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Alex Wang]

Hey Ben,

Also want to ask how do you get the gcc warning? I compiled with '-O3' and
didn't see it.

Thanks,

On Mon, Jul 22, 2013 at 11:35 AM, Alex Wang  wrote:

> Thanks Ben,
>
> I'll recheck that.
>
>
> On Mon, Jul 22, 2013 at 11:30 AM, Ben Pfaff  wrote:
>
>> On Mon, Jul 22, 2013 at 09:19:57AM -0700, Alex Wang wrote:
>> > This commit makes macro function "ASSIGN_CONTAINER()" evaluates
>> > to "(void)0". This is to avoid the 'clang' warning: "expression
>> > result unused", since most of time, the final evaluated value
>> > is not used.
>> >
>> > Signed-off-by: Alex Wang 
>>
>> I think you missed a case in heap.h.  I had to apply the following to
>> avoid a pile of GCC warnings because in "a ? b : c" the b expression
>> had type void and the c expression had type int (value 1).  Can you
>> fold that in and verify that clang accepts it too?
>>
>> diff --git a/lib/heap.h b/lib/heap.h
>> index 9326d79..5c07e04 100644
>> --- a/lib/heap.h
>> +++ b/lib/heap.h
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2012 Nicira, Inc.
>> + * Copyright (c) 2012, 2013 Nicira, Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>   * you may not use this file except in compliance with the License.
>> @@ -69,13 +69,13 @@ void heap_rebuild(struct heap *);
>>  #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)   \
>>  for (((HEAP)->n > 0 \
>>? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)\
>> -  : ((NODE) = NULL, 1));\
>> +  : ((NODE) = NULL, (void) 0));   \
>>   (NODE) != NULL;\
>>   ((NODE)->MEMBER.idx < (HEAP)->n\
>>? ASSIGN_CONTAINER(NODE,  \
>>   (HEAP)->array[(NODE)->MEMBER.idx + 1], \
>>   MEMBER)\
>> -  : ((NODE) = NULL, 1)))
>> +  : ((NODE) = NULL, (void) 0)))
>>
>>  /* Returns the index of the node that is the parent of the node with the
>> given
>>   * 'idx' within a heap. */
>>
>> Thanks,
>>
>> Ben.
>>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Alex Wang]

This commit makes macro function "ASSIGN_CONTAINER()" evaluates
to "(void)0". This is to avoid the 'clang' warning: "expression
result unused", since most of time, the final evaluated value
is not used.

Signed-off-by: Alex Wang 
---
 lib/classifier.h |2 +-
 lib/hindex.h |2 +-
 lib/hmap.h   |2 +-
 lib/list.h   |   10 +-
 lib/util.h   |4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/classifier.h b/lib/classifier.h
index 09b3a13..a14a24d 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -132,7 +132,7 @@ struct cls_rule *cls_cursor_next(struct cls_cursor *, 
struct cls_rule *);
 for (ASSIGN_CONTAINER(RULE, cls_cursor_first(CURSOR), MEMBER);  \
  (RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER) \
   ? ASSIGN_CONTAINER(NEXT, cls_cursor_next(CURSOR, &(RULE)->MEMBER), \
- MEMBER)\
+ MEMBER), 1 \
   : 0); \
  (RULE) = (NEXT))
 
diff --git a/lib/hindex.h b/lib/hindex.h
index ce46596..fb6b6d2 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -147,7 +147,7 @@ struct hindex_node *hindex_node_with_hash(const struct 
hindex *, size_t hash);
 #define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)\
 for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);  \
  (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER) \
-  ? ASSIGN_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), 
MEMBER) \
+  ? ASSIGN_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), 
MEMBER), 1 \
   : 0); \
  (NODE) = (NEXT))
 
diff --git a/lib/hmap.h b/lib/hmap.h
index c7df62a..ab7d3ae 100644
--- a/lib/hmap.h
+++ b/lib/hmap.h
@@ -146,7 +146,7 @@ bool hmap_contains(const struct hmap *, const struct 
hmap_node *);
 #define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP)\
 for (ASSIGN_CONTAINER(NODE, hmap_first(HMAP), MEMBER);  \
  (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)  \
-  ? ASSIGN_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER) \
+  ? ASSIGN_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 
1 \
   : 0); \
  (NODE) = (NEXT))
 
diff --git a/lib/list.h b/lib/list.h
index 55e0d0a..786b176 100644
--- a/lib/list.h
+++ b/lib/list.h
@@ -72,11 +72,11 @@ bool list_is_short(const struct list *);
 for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER);   \
  &(ITER)->MEMBER != (LIST); \
  ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)\
-for (ASSIGN_CONTAINER(ITER, (LIST)->next, MEMBER);  \
- (&(ITER)->MEMBER != (LIST) \
-  ? ASSIGN_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER) \
-  : 0); \
+#define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)   \
+for (ASSIGN_CONTAINER(ITER, (LIST)->next, MEMBER); \
+ (&(ITER)->MEMBER != (LIST)\
+  ? ASSIGN_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1 \
+  : 0);\
  (ITER) = (NEXT))
 
 #endif /* list.h */
diff --git a/lib/util.h b/lib/util.h
index 2159594..911ad32 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -189,9 +189,9 @@ is_pow2(uintmax_t x)
  * that that OBJECT points to, assigns the address of the outer object to
  * OBJECT, which must be an lvalue.
  *
- * Evaluates to 1. */
+ * Evaluates to (void) 0 as the result is not to be used. */
 #define ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER) \
-((OBJECT) = OBJECT_CONTAINING(POINTER, OBJECT, MEMBER), 1)
+((OBJECT) = OBJECT_CONTAINING(POINTER, OBJECT, MEMBER), (void) 0)
 
 #ifdef  __cplusplus
 extern "C" {
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/3] clang: Fix segfault in unit tests.

[Alex Wang]

It seems that 'clang' compiler applies strict protection on pointer
dereference. And it causes unexpected execution in macro functions
like "HMAP_FOR_EACH()" and unit test failures. This commit fixes
this issue and pass all unit tests.

Co-authored-by: Ethan Jackson 
Signed-off-by: Alex Wang 
---
 lib/classifier.h |4 ++--
 lib/hindex.h |6 +++---
 lib/hmap.h   |   10 +-
 lib/sset.h   |8 +---
 4 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/lib/classifier.h b/lib/classifier.h
index fdc3af7..09b3a13 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -124,13 +124,13 @@ struct cls_rule *cls_cursor_next(struct cls_cursor *, 
struct cls_rule *);
 
 #define CLS_CURSOR_FOR_EACH(RULE, MEMBER, CURSOR)   \
 for (ASSIGN_CONTAINER(RULE, cls_cursor_first(CURSOR), MEMBER);  \
- &(RULE)->MEMBER != NULL;   \
+ RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER); \
  ASSIGN_CONTAINER(RULE, cls_cursor_next(CURSOR, &(RULE)->MEMBER), \
   MEMBER))
 
 #define CLS_CURSOR_FOR_EACH_SAFE(RULE, NEXT, MEMBER, CURSOR)\
 for (ASSIGN_CONTAINER(RULE, cls_cursor_first(CURSOR), MEMBER);  \
- (&(RULE)->MEMBER != NULL   \
+ (RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER) \
   ? ASSIGN_CONTAINER(NEXT, cls_cursor_next(CURSOR, &(RULE)->MEMBER), \
  MEMBER)\
   : 0); \
diff --git a/lib/hindex.h b/lib/hindex.h
index 10bf024..ce46596 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -129,7 +129,7 @@ void hindex_remove(struct hindex *, struct hindex_node *);
  */
 #define HINDEX_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HINDEX)   \
 for (ASSIGN_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), MEMBER); \
- &(NODE)->MEMBER != NULL;   \
+ NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); \
  ASSIGN_CONTAINER(NODE, (NODE)->MEMBER.s, MEMBER))
 
 struct hindex_node *hindex_node_with_hash(const struct hindex *, size_t hash);
@@ -139,14 +139,14 @@ struct hindex_node *hindex_node_with_hash(const struct 
hindex *, size_t hash);
 /* Iterates through every node in HINDEX. */
 #define HINDEX_FOR_EACH(NODE, MEMBER, HINDEX)   \
 for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);  \
- &(NODE)->MEMBER != NULL;   \
+ NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); \
  ASSIGN_CONTAINER(NODE, hindex_next(HINDEX, &(NODE)->MEMBER), MEMBER))
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash index but its members remain accessible and intact). */
 #define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)\
 for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);  \
- (&(NODE)->MEMBER != NULL   \
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER) \
   ? ASSIGN_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), 
MEMBER) \
   : 0); \
  (NODE) = (NEXT))
diff --git a/lib/hmap.h b/lib/hmap.h
index 9b6d8c7..c7df62a 100644
--- a/lib/hmap.h
+++ b/lib/hmap.h
@@ -116,12 +116,12 @@ struct hmap_node *hmap_random_node(const struct hmap *);
  */
 #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)   \
 for (ASSIGN_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
- &(NODE)->MEMBER != NULL;   \
+ NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);  \
  ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),   \
   MEMBER))
 #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)   \
 for (ASSIGN_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
- &(NODE)->MEMBER != NULL;   \
+ NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);  \
  ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))
 
 static inline struct hmap_node *hmap_first_with_hash(const struct hmap *,
@@ -138,14 +138,14 @@ bool hmap_contains(const struct hmap *, const struct 
hmap_node *);
 /* Iterates through every node in HMAP. */
 #define HMAP_FOR_EACH(NODE, MEMBER, HMAP)   \
 for (ASSIGN_CONTAINER(NODE, hmap_first(HMAP), MEMBER);  \
- &(NODE)->MEMBER != NULL;   \
+ NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);  \
  ASSIGN_CONTAINER(NODE, hmap_next

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Ben Pfaff]

I didn't do anything special to get the warning.  With GCC 4.4.5 and
this patch applied, I see:

../ofproto/ofproto.c:5037:5: error: incompatible types in conditional 
expression (different base types)
../ofproto/ofproto.c:5040:9: error: incompatible types in conditional 
expression (different base types)
../ofproto/ofproto.c:5040:9: error: incompatible types in conditional 
expression (different base types)
../ofproto/ofproto.c:5037:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:97:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:97:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:107:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:107:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:139:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:139:5: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:219:9: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:219:9: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:258:9: error: incompatible types in conditional 
expression (different base types)
../tests/test-heap.c:258:9: error: incompatible types in conditional 
expression (different base types)

On Mon, Jul 22, 2013 at 11:39:41AM -0700, Alex Wang wrote:
> Hey Ben,
> 
> Also want to ask how do you get the gcc warning? I compiled with '-O3' and
> didn't see it.
> 
> Thanks,
> 
> On Mon, Jul 22, 2013 at 11:35 AM, Alex Wang  wrote:
> 
> > Thanks Ben,
> >
> > I'll recheck that.
> >
> >
> > On Mon, Jul 22, 2013 at 11:30 AM, Ben Pfaff  wrote:
> >
> >> On Mon, Jul 22, 2013 at 09:19:57AM -0700, Alex Wang wrote:
> >> > This commit makes macro function "ASSIGN_CONTAINER()" evaluates
> >> > to "(void)0". This is to avoid the 'clang' warning: "expression
> >> > result unused", since most of time, the final evaluated value
> >> > is not used.
> >> >
> >> > Signed-off-by: Alex Wang 
> >>
> >> I think you missed a case in heap.h.  I had to apply the following to
> >> avoid a pile of GCC warnings because in "a ? b : c" the b expression
> >> had type void and the c expression had type int (value 1).  Can you
> >> fold that in and verify that clang accepts it too?
> >>
> >> diff --git a/lib/heap.h b/lib/heap.h
> >> index 9326d79..5c07e04 100644
> >> --- a/lib/heap.h
> >> +++ b/lib/heap.h
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2012 Nicira, Inc.
> >> + * Copyright (c) 2012, 2013 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -69,13 +69,13 @@ void heap_rebuild(struct heap *);
> >>  #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)   \
> >>  for (((HEAP)->n > 0 \
> >>? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)\
> >> -  : ((NODE) = NULL, 1));\
> >> +  : ((NODE) = NULL, (void) 0));   \
> >>   (NODE) != NULL;\
> >>   ((NODE)->MEMBER.idx < (HEAP)->n\
> >>? ASSIGN_CONTAINER(NODE,  \
> >>   (HEAP)->array[(NODE)->MEMBER.idx + 1], \
> >>   MEMBER)\
> >> -  : ((NODE) = NULL, 1)))
> >> +  : ((NODE) = NULL, (void) 0)))
> >>
> >>  /* Returns the index of the node that is the parent of the node with the
> >> given
> >>   * 'idx' within a heap. */
> >>
> >> Thanks,
> >>
> >> Ben.
> >>
> >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Add mask check during flow lookup

[Jesse Gross]

On Fri, Jul 19, 2013 at 11:11 AM, Andy Zhou  wrote:
> A mega flow matches when the masked key matches and the mask applied
> is the same as the mask used to create the mega flow.
>
> This patch adds the implementation of the second match condition
> mentioned above. Without this fix, mega flow lookup may result false
> match.
>
> Bug #18584
>
> Signed-off-by: Andy Zhou 

Can you explain the conditions under which this can occur? It seems to
imply the overlapping flows are being installed, which is not supposed
to happen.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Alex Wang]

I see, I didn't compile use sparse.

My gcc is: gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
And it didn't issue any error, when sparse is not enabled.

I should always do that,.


On Mon, Jul 22, 2013 at 12:22 PM, Ben Pfaff  wrote:

> I didn't do anything special to get the warning.  With GCC 4.4.5 and
> this patch applied, I see:
>
> ../ofproto/ofproto.c:5037:5: error: incompatible types in conditional
> expression (different base types)
> ../ofproto/ofproto.c:5040:9: error: incompatible types in conditional
> expression (different base types)
> ../ofproto/ofproto.c:5040:9: error: incompatible types in conditional
> expression (different base types)
> ../ofproto/ofproto.c:5037:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:97:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:97:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:107:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:107:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:139:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:139:5: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:219:9: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:219:9: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:258:9: error: incompatible types in conditional
> expression (different base types)
> ../tests/test-heap.c:258:9: error: incompatible types in conditional
> expression (different base types)
>
> On Mon, Jul 22, 2013 at 11:39:41AM -0700, Alex Wang wrote:
> > Hey Ben,
> >
> > Also want to ask how do you get the gcc warning? I compiled with '-O3'
> and
> > didn't see it.
> >
> > Thanks,
> >
> > On Mon, Jul 22, 2013 at 11:35 AM, Alex Wang  wrote:
> >
> > > Thanks Ben,
> > >
> > > I'll recheck that.
> > >
> > >
> > > On Mon, Jul 22, 2013 at 11:30 AM, Ben Pfaff  wrote:
> > >
> > >> On Mon, Jul 22, 2013 at 09:19:57AM -0700, Alex Wang wrote:
> > >> > This commit makes macro function "ASSIGN_CONTAINER()" evaluates
> > >> > to "(void)0". This is to avoid the 'clang' warning: "expression
> > >> > result unused", since most of time, the final evaluated value
> > >> > is not used.
> > >> >
> > >> > Signed-off-by: Alex Wang 
> > >>
> > >> I think you missed a case in heap.h.  I had to apply the following to
> > >> avoid a pile of GCC warnings because in "a ? b : c" the b expression
> > >> had type void and the c expression had type int (value 1).  Can you
> > >> fold that in and verify that clang accepts it too?
> > >>
> > >> diff --git a/lib/heap.h b/lib/heap.h
> > >> index 9326d79..5c07e04 100644
> > >> --- a/lib/heap.h
> > >> +++ b/lib/heap.h
> > >> @@ -1,5 +1,5 @@
> > >>  /*
> > >> - * Copyright (c) 2012 Nicira, Inc.
> > >> + * Copyright (c) 2012, 2013 Nicira, Inc.
> > >>   *
> > >>   * Licensed under the Apache License, Version 2.0 (the "License");
> > >>   * you may not use this file except in compliance with the License.
> > >> @@ -69,13 +69,13 @@ void heap_rebuild(struct heap *);
> > >>  #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)   \
> > >>  for (((HEAP)->n > 0 \
> > >>? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)\
> > >> -  : ((NODE) = NULL, 1));\
> > >> +  : ((NODE) = NULL, (void) 0));
>   \
> > >>   (NODE) != NULL;\
> > >>   ((NODE)->MEMBER.idx < (HEAP)->n\
> > >>? ASSIGN_CONTAINER(NODE,  \
> > >>   (HEAP)->array[(NODE)->MEMBER.idx + 1], \
> > >>   MEMBER)\
> > >> -  : ((NODE) = NULL, 1)))
> > >> +  : ((NODE) = NULL, (void) 0)))
> > >>
> > >>  /* Returns the index of the node that is the parent of the node with
> the
> > >> given
> > >>   * 'idx' within a heap. */
> > >>
> > >> Thanks,
> > >>
> > >> Ben.
> > >>
> > >
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 02/23] ofp-msgs: Make thread-safe.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 09:10:52AM -0700, Alex Wang wrote:
> Looks good to me,
> 
> Want to ask question about "ovsthread_once_start()",
> 1. why do we have "ovsthread_once_start()" defined in header file?

The "fast path" for using ovsthread_once_start() is just a test and
conditional jump.  The alternative is a function call followed by a
test and a conditional jump.  I decided to use the former as an
optimization; it might have been premature.

> 2. if __check__ is defined, the macro function will override the
> previously defined "ovsthread_once_start()" function. Right?

Yes.

> 3. could you explain why you use "ovsthread_once_start()" rather than
> "pthread_once()" like in other patches (since the init function does not
> take
> any input argument)?

I thought it made the patch easier to read since the change was smaller.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/5] ovs-bugtool: Separate capability for general network info.

[Gurucharan Shetty]

Current situation is that CAP_NETWORK_STATUS has a max size of 50 MB.
When we have around 100,000 openflow flows, we over-run that size
by just running the "ovs-ofctl dump-flows" command. All the openvswitch
commands run through the plugin scripts in this repo won't have its
data stored in the debug bundle in this case as they are part of
CAP_NETWORK_STATUS too. One option to correct this is to increase
the CAP_NETWORK_STATUS max size to a higher number. But CAP_NETWORK_STATUS
also includes a bunch of general network related information collected
by running commands like ethtool, tc etc. and we probably want to limit
the data collected through those commands.

With this commit, we create a new capability called CAP_NETWORK_INFO
and collect general network related information through them. For OVS
related information, we continue to use CAP_NETWORK_STATUS, but remove
the maximum size restriction. One rationale to keep OVS related
information in CAP_NETWORK_STATUS is because xen-bugtool probably expects
OVS information in that capability.

Signed-off-by: Gurucharan Shetty 
---
 utilities/bugtool/ovs-bugtool.in |   37 -
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index aa5b960..854dfa1 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -204,6 +204,7 @@ CAP_KERNEL_INFO  = 'kernel-info'
 CAP_LOSETUP_A= 'loopback-devices'
 CAP_MULTIPATH= 'multipath'
 CAP_NETWORK_CONFIG   = 'network-config'
+CAP_NETWORK_INFO = 'network-info'
 CAP_NETWORK_STATUS   = 'network-status'
 CAP_OPENVSWITCH_LOGS= 'ovs-system-logs'
 CAP_PROCESS_LIST = 'process-list'
@@ -241,7 +242,9 @@ cap(CAP_MULTIPATH,   PII_MAYBE, 
max_size=20*KB,
 max_time=10)
 cap(CAP_NETWORK_CONFIG,  PII_IF_CUSTOMIZED,
 min_size=0, max_size=40*KB)
-cap(CAP_NETWORK_STATUS,  PII_YES,   max_size=50*MB,
+cap(CAP_NETWORK_INFO,PII_YES,   max_size=50*MB,
+max_time=30)
+cap(CAP_NETWORK_STATUS,  PII_YES,   max_size=-1,
 max_time=30)
 cap(CAP_OPENVSWITCH_LOGS,PII_MAYBE, max_size=-1,
 max_time=5)
@@ -542,14 +545,14 @@ exclude those logs from the archive.
 file_output(CAP_NETWORK_CONFIG, [NTP_CONF, IPTABLES_CONFIG, HOSTS_ALLOW, 
HOSTS_DENY])
 file_output(CAP_NETWORK_CONFIG, [OPENVSWITCH_CONF_DB])
 
-cmd_output(CAP_NETWORK_STATUS, [IFCONFIG, '-a'])
-cmd_output(CAP_NETWORK_STATUS, [ROUTE, '-n'])
-cmd_output(CAP_NETWORK_STATUS, [ARP, '-n'])
-cmd_output(CAP_NETWORK_STATUS, [NETSTAT, '-an'])
+cmd_output(CAP_NETWORK_INFO, [IFCONFIG, '-a'])
+cmd_output(CAP_NETWORK_INFO, [ROUTE, '-n'])
+cmd_output(CAP_NETWORK_INFO, [ARP, '-n'])
+cmd_output(CAP_NETWORK_INFO, [NETSTAT, '-an'])
 for dir in DHCP_LEASE_DIR:
-tree_output(CAP_NETWORK_STATUS, dir)
+tree_output(CAP_NETWORK_INFO, dir)
 for table in ['filter', 'nat', 'mangle', 'raw', 'security']:
-cmd_output(CAP_NETWORK_STATUS, [IPTABLES, '-t', table, '-nL'])
+cmd_output(CAP_NETWORK_INFO, [IPTABLES, '-t', table, '-nL'])
 for p in os.listdir('/sys/class/net/'):
 try:
 f = open('/sys/class/net/%s/type' % p, 'r')
@@ -557,21 +560,21 @@ exclude those logs from the archive.
 f.close()
 if os.path.islink('/sys/class/net/%s/device' % p) and int(t) == 1:
 # ARPHRD_ETHER
-cmd_output(CAP_NETWORK_STATUS, [ETHTOOL, '-S', p])
+cmd_output(CAP_NETWORK_INFO, [ETHTOOL, '-S', p])
 if not p.startswith('vif') and not p.startswith('tap'):
-cmd_output(CAP_NETWORK_STATUS, [ETHTOOL, p])
-cmd_output(CAP_NETWORK_STATUS, [ETHTOOL, '-k', p])
-cmd_output(CAP_NETWORK_STATUS, [ETHTOOL, '-i', p])
-cmd_output(CAP_NETWORK_STATUS, [ETHTOOL, '-c', p])
+cmd_output(CAP_NETWORK_INFO, [ETHTOOL, p])
+cmd_output(CAP_NETWORK_INFO, [ETHTOOL, '-k', p])
+cmd_output(CAP_NETWORK_INFO, [ETHTOOL, '-i', p])
+cmd_output(CAP_NETWORK_INFO, [ETHTOOL, '-c', p])
 if int(t) == 1:
-cmd_output(CAP_NETWORK_STATUS,
+cmd_output(CAP_NETWORK_INFO,
[TC, '-s', '-d', 'class', 'show', 'dev', p])
 except:
 pass
-tree_output(CAP_NETWORK_STATUS, PROC_NET_BONDING_DIR)
-tree_output(CAP_NETWORK_STATUS, PROC_NET_VLAN_DIR)
-cmd_output(CAP_NETWORK_STATUS, [TC, '-s', 'qdisc'])
-file_output(CAP_NETWORK_STATUS, [PROC_NET_SOFTNET_STAT])
+tree_output(CAP_NETWORK_INFO, PROC_NET_BONDING_DIR)
+tree_output(CAP_NETWORK_INFO, PROC_NET_VLAN_DIR)
+cmd_output(CAP_NETWORK_INFO, [TC, '-s', '

[ovs-dev] [PATCH 4/5] ovs-bugtool: Add config files to the debug bundle.

[Gurucharan Shetty]

The previously defined config files were never included in
the debug bundle. This will include them.

Also increase the max size for CAP_NETWORK_CONFIG to 5 MB.
A pre-compressed size of 5 MB does not amount to much after
compression for config files.

Signed-off-by: Gurucharan Shetty 
---
 utilities/bugtool/ovs-bugtool.in |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index e2a8c37..e012362 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -241,7 +241,7 @@ cap(CAP_LOSETUP_A,   PII_MAYBE, 
max_size=KB, max_time=5)
 cap(CAP_MULTIPATH,   PII_MAYBE, max_size=20*KB,
 max_time=10)
 cap(CAP_NETWORK_CONFIG,  PII_IF_CUSTOMIZED,
-min_size=0, max_size=40*KB)
+min_size=0, max_size=5*MB)
 cap(CAP_NETWORK_INFO,PII_YES,   max_size=50*MB,
 max_time=30)
 cap(CAP_NETWORK_STATUS,  PII_YES,   max_size=-1,
@@ -543,6 +543,8 @@ exclude those logs from the archive.
 tree_output(CAP_NETWORK_CONFIG, SYSCONFIG_NETWORK_SCRIPTS, ROUTE_RE)
 file_output(CAP_NETWORK_CONFIG, [SYSCONFIG_NETWORK, RESOLV_CONF, 
NSSWITCH_CONF, HOSTS])
 file_output(CAP_NETWORK_CONFIG, [NTP_CONF, IPTABLES_CONFIG, HOSTS_ALLOW, 
HOSTS_DENY])
+file_output(CAP_NETWORK_CONFIG, [OPENVSWITCH_DEFAULT_SWITCH,
+OPENVSWITCH_SYSCONFIG_SWITCH, OPENVSWITCH_DEFAULT_CONTROLLER])
 
 cmd_output(CAP_NETWORK_INFO, [IFCONFIG, '-a'])
 cmd_output(CAP_NETWORK_INFO, [ROUTE, '-n'])
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Alex Wang]

I folded your changes in, and there is no warning/error,
when compiling with 'clang' and 'gcc'.

Unit tests all passed,

Thanks for your review, do I still need to post a v2 patch?


On Mon, Jul 22, 2013 at 12:36 PM, Alex Wang  wrote:

> I see, I didn't compile use sparse.
>
> My gcc is: gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
> And it didn't issue any error, when sparse is not enabled.
>
> I should always do that,.
>
>
> On Mon, Jul 22, 2013 at 12:22 PM, Ben Pfaff  wrote:
>
>> I didn't do anything special to get the warning.  With GCC 4.4.5 and
>> this patch applied, I see:
>>
>> ../ofproto/ofproto.c:5037:5: error: incompatible types in conditional
>> expression (different base types)
>> ../ofproto/ofproto.c:5040:9: error: incompatible types in conditional
>> expression (different base types)
>> ../ofproto/ofproto.c:5040:9: error: incompatible types in conditional
>> expression (different base types)
>> ../ofproto/ofproto.c:5037:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:97:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:97:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:107:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:107:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:139:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:139:5: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:219:9: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:219:9: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:258:9: error: incompatible types in conditional
>> expression (different base types)
>> ../tests/test-heap.c:258:9: error: incompatible types in conditional
>> expression (different base types)
>>
>> On Mon, Jul 22, 2013 at 11:39:41AM -0700, Alex Wang wrote:
>> > Hey Ben,
>> >
>> > Also want to ask how do you get the gcc warning? I compiled with '-O3'
>> and
>> > didn't see it.
>> >
>> > Thanks,
>> >
>> > On Mon, Jul 22, 2013 at 11:35 AM, Alex Wang  wrote:
>> >
>> > > Thanks Ben,
>> > >
>> > > I'll recheck that.
>> > >
>> > >
>> > > On Mon, Jul 22, 2013 at 11:30 AM, Ben Pfaff  wrote:
>> > >
>> > >> On Mon, Jul 22, 2013 at 09:19:57AM -0700, Alex Wang wrote:
>> > >> > This commit makes macro function "ASSIGN_CONTAINER()" evaluates
>> > >> > to "(void)0". This is to avoid the 'clang' warning: "expression
>> > >> > result unused", since most of time, the final evaluated value
>> > >> > is not used.
>> > >> >
>> > >> > Signed-off-by: Alex Wang 
>> > >>
>> > >> I think you missed a case in heap.h.  I had to apply the following to
>> > >> avoid a pile of GCC warnings because in "a ? b : c" the b expression
>> > >> had type void and the c expression had type int (value 1).  Can you
>> > >> fold that in and verify that clang accepts it too?
>> > >>
>> > >> diff --git a/lib/heap.h b/lib/heap.h
>> > >> index 9326d79..5c07e04 100644
>> > >> --- a/lib/heap.h
>> > >> +++ b/lib/heap.h
>> > >> @@ -1,5 +1,5 @@
>> > >>  /*
>> > >> - * Copyright (c) 2012 Nicira, Inc.
>> > >> + * Copyright (c) 2012, 2013 Nicira, Inc.
>> > >>   *
>> > >>   * Licensed under the Apache License, Version 2.0 (the "License");
>> > >>   * you may not use this file except in compliance with the License.
>> > >> @@ -69,13 +69,13 @@ void heap_rebuild(struct heap *);
>> > >>  #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)
>> \
>> > >>  for (((HEAP)->n > 0
>> \
>> > >>? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)
>>  \
>> > >> -  : ((NODE) = NULL, 1));
>>  \
>> > >> +  : ((NODE) = NULL, (void) 0));
>>   \
>> > >>   (NODE) != NULL;
>>  \
>> > >>   ((NODE)->MEMBER.idx < (HEAP)->n
>>  \
>> > >>? ASSIGN_CONTAINER(NODE,
>>  \
>> > >>   (HEAP)->array[(NODE)->MEMBER.idx + 1],
>> \
>> > >>   MEMBER)
>>  \
>> > >> -  : ((NODE) = NULL, 1)))
>> > >> +  : ((NODE) = NULL, (void) 0)))
>> > >>
>> > >>  /* Returns the index of the node that is the parent of the node
>> with the
>> > >> given
>> > >>   * 'idx' within a heap. */
>> > >>
>> > >> Thanks,
>> > >>
>> > >> Ben.
>> > >>
>> > >
>> > >
>>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] sparse warning

[Jesse Gross]

On Mon, Jul 22, 2013 at 10:58 AM, Ben Pfaff  wrote:
> I'm getting the following warning on current master (commit
> dc0d542d5254c1e6 "datapath: Conditionally define skb_unclone in
> datapath compat code"):
>
> datapath/flow.c:1051:32: error: incompatible types in comparison
> expression (different address spaces)
>
> That's in ovs_masked_flow_lookup():
>
> hlist_for_each_entry_rcu(flow, head, hash_node[table->node_ver]) {
> 1051 -> if (flow->mask == mask &&
> __flow_cmp_key(flow, &masked_key, key_start, key_len))
> return flow;
> }

This is due to a missing RCU annotation when dereferencing flow->mask.
It's not a real problem because the mask never changes once assigned
to a flow so the use of RCU is really somewhat limited here.

My inclination is to just remove the RCU annotation from the mask in
the flow declaration (and corresponding assignments/dereferences)
since they are somewhat misleading when it comes to flows.

Andy, can you take a look?
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] sparse: Avoid sparse warnings for additional pthread initializers.

[Ben Pfaff]

Reported-by: Andy Zhou 
Signed-off-by: Ben Pfaff 
---
 include/sparse/pthread.h |8 +++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index 723c351..1a025bf 100644
--- a/include/sparse/pthread.h
+++ b/include/sparse/pthread.h
@@ -33,12 +33,18 @@ int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) 
OVS_RELEASES(rwlock);
 int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
 OVS_MUST_HOLD(mutex);
 
-/* Sparse complains about the proper PTHREAD_MUTEX_INITIALIZER definition.
+/* Sparse complains about the proper PTHREAD_*_INITIALIZER definitions.
  * Luckily, it's not a real compiler so we can overwrite it with something
  * simple. */
 #undef PTHREAD_MUTEX_INITIALIZER
 #define PTHREAD_MUTEX_INITIALIZER {}
 
+#undef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER
+#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER {}
+
+#undef PTHREAD_RWLOCK_INITIALIZER
+#define PTHREAD_RWLOCK_INITIALIZER {}
+
 #define pthread_mutex_trylock(MUTEX)\
 ({  \
 int retval = pthread_mutex_trylock(mutex);  \
-- 
1.7.2.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 5/5] ovs-bugtool: Increase max size of CAP_HARDWARE_INFO.

[Gurucharan Shetty]

Current size feels very low when we are collecting o/p of
'dmidecode' and 'lspci -vv'

Signed-off-by: Gurucharan Shetty 
---
 utilities/bugtool/ovs-bugtool.in |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index e012362..3703f36 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -233,7 +233,7 @@ cap(CAP_BOOT_LOADER, PII_NO,
max_size=3*KB,
 max_time=5)
 cap(CAP_DISK_INFO,   PII_MAYBE, max_size=50*KB,
 max_time=20)
-cap(CAP_HARDWARE_INFO,   PII_MAYBE, max_size=30*KB,
+cap(CAP_HARDWARE_INFO,   PII_MAYBE, max_size=2*MB,
 max_time=20)
 cap(CAP_KERNEL_INFO, PII_MAYBE, max_size=120*KB,
 max_time=5)
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 3/5] ovs-bugtool: Compact the database before collecting.

[Gurucharan Shetty]

Currently the openvswitch database is being collected with
CAP_NETWORK_CONFIG which has a max size of 50 KB. This is
quite low as the database can easily be larger than 50 KB.

Move database collection to CAP_NETWORK_STATUS which does
not have a max size. Also, compact the database before picking
it up.

Signed-off-by: Gurucharan Shetty 
---
 utilities/bugtool/ovs-bugtool.in |   11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index 854dfa1..e2a8c37 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -543,7 +543,6 @@ exclude those logs from the archive.
 tree_output(CAP_NETWORK_CONFIG, SYSCONFIG_NETWORK_SCRIPTS, ROUTE_RE)
 file_output(CAP_NETWORK_CONFIG, [SYSCONFIG_NETWORK, RESOLV_CONF, 
NSSWITCH_CONF, HOSTS])
 file_output(CAP_NETWORK_CONFIG, [NTP_CONF, IPTABLES_CONFIG, HOSTS_ALLOW, 
HOSTS_DENY])
-file_output(CAP_NETWORK_CONFIG, [OPENVSWITCH_CONF_DB])
 
 cmd_output(CAP_NETWORK_INFO, [IFCONFIG, '-a'])
 cmd_output(CAP_NETWORK_INFO, [ROUTE, '-n'])
@@ -575,6 +574,9 @@ exclude those logs from the archive.
 tree_output(CAP_NETWORK_INFO, PROC_NET_VLAN_DIR)
 cmd_output(CAP_NETWORK_INFO, [TC, '-s', 'qdisc'])
 file_output(CAP_NETWORK_INFO, [PROC_NET_SOFTNET_STAT])
+
+compact_db()
+file_output(CAP_NETWORK_STATUS, [OPENVSWITCH_CONF_DB])
 if os.path.exists(OPENVSWITCH_VSWITCHD_PID):
 cmd_output(CAP_NETWORK_STATUS, [OVS_DPCTL, 'show', '-s'])
 for d in dp_list():
@@ -739,6 +741,13 @@ def dp_list():
 return output.getvalue().splitlines()
 return []
 
+def compact_db():
+output = StringIO.StringIO()
+max_time = 5
+procs = [ProcOutput(['ovs-appctl', '-t', 'ovsdb-server',
+ 'ovsdb-server/compact', 'Open_vSwitch'], max_time, output)]
+run_procs([procs])
+
 def fd_usage(cap):
 output = ''
 fd_dict = {}
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/5] ovs-bugtool: Remove duplicate bond/show command.

[Gurucharan Shetty]

ovs-appctl bond/show is being run through the plugin by
calling the script ovs-bugtool-bond-show.

So remove the redundant code.

Signed-off-by: Gurucharan Shetty 
---
 utilities/bugtool/ovs-bugtool.in |   22 --
 1 file changed, 22 deletions(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index ffaf753..aa5b960 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -149,7 +149,6 @@ NETSTAT = 'netstat'
 OVS_DPCTL = 'ovs-dpctl'
 OVS_OFCTL = 'ovs-ofctl'
 OVS_VSCTL = 'ovs-vsctl'
-OVS_APPCTL = 'ovs-appctl'
 PS = 'ps'
 ROUTE = 'route'
 RPM = 'rpm'
@@ -577,16 +576,6 @@ exclude those logs from the archive.
 cmd_output(CAP_NETWORK_STATUS, [OVS_DPCTL, 'show', '-s'])
 for d in dp_list():
 cmd_output(CAP_NETWORK_STATUS, [OVS_DPCTL, 'dump-flows', d])
-try:
-vspidfile = open(OPENVSWITCH_VSWITCHD_PID)
-vspid = int(vspidfile.readline().strip())
-vspidfile.close()
-for b in bond_list(vspid):
-cmd_output(CAP_NETWORK_STATUS,
-   [OVS_APPCTL, '-t', '@RUNDIR@/ovs-vswitchd.%s.ctl' % 
vspid, '-e' 'bond/show %s' % b],
-   'ovs-appctl-bond-show-%s.out' % b)
-except e:
-pass
 
 cmd_output(CAP_PROCESS_LIST, [PS, 'wwwaxf', '-eo', 
'pid,tty,stat,time,nice,psr,pcpu,pmem,nwchan,wchan:25,args'], 
label='process-tree')
 func_output(CAP_PROCESS_LIST, 'fd_usage', fd_usage)
@@ -747,17 +736,6 @@ def dp_list():
 return output.getvalue().splitlines()
 return []
 
-def bond_list(pid):
-output = StringIO.StringIO()
-procs = [ProcOutput([OVS_APPCTL, '-t', '@RUNDIR@/ovs-vswitchd.%s.ctl' % 
pid, '-e' 'bond/list'], caps[CAP_NETWORK_STATUS][MAX_TIME], output)]
-
-run_procs([procs])
-
-if not procs[0].timed_out:
-bonds = output.getvalue().splitlines()[1:]
-return [x.split('\t')[1] for x in bonds]
-return []
-
 def fd_usage(cap):
 output = ''
 fd_dict = {}
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] clang: Fix the "expression result unused" warning.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 12:44:47PM -0700, Alex Wang wrote:
> I folded your changes in, and there is no warning/error,
> when compiling with 'clang' and 'gcc'.
> 
> Unit tests all passed,
> 
> Thanks for your review, do I still need to post a v2 patch?

No, that's fine, I'll fold it in myself.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Add mask check during flow lookup

[Andy Zhou]

The actual case I found this bug is more complicated to explain, but the
following made up example can illustrate the problem. Consider we have 2
kernel mega flows:

1)  in_port(1/0x),  *
2)  in_port(2/0x),  src_ip(1.1.1.1/255.255.255.255)

They are not overlapping flows, however, when the following packet arrives
(in_port(2), src_ip(2,2,2,2)). if we apply mask of 1), it could match flow
2). But it should not.

Adding the mask check would have prevented this false match.

--andy


On Mon, Jul 22, 2013 at 9:57 AM, Jesse Gross  wrote:

> On Fri, Jul 19, 2013 at 11:11 AM, Andy Zhou  wrote:
> > A mega flow matches when the masked key matches and the mask applied
> > is the same as the mask used to create the mega flow.
> >
> > This patch adds the implementation of the second match condition
> > mentioned above. Without this fix, mega flow lookup may result false
> > match.
> >
> > Bug #18584
> >
> > Signed-off-by: Andy Zhou 
>
> Can you explain the conditions under which this can occur? It seems to
> imply the overlapping flows are being installed, which is not supposed
> to happen.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix invalid memory read on port removal.

[Ethan Jackson]

Signed-off-by: Ethan Jackson 
---
 ofproto/ofproto-dpif-xlate.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index eb4ed69..e555603 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -400,7 +400,10 @@ xlate_ofport_remove(struct ofport_dpif *ofport)
 xport->peer = NULL;
 }
 
-list_remove(&xport->bundle_node);
+if (xport->xbundle) {
+list_remove(&xport->bundle_node);
+}
+
 hmap_remove(&xports, &xport->hmap_node);
 hmap_remove(&xport->xbridge->xports, &xport->ofp_node);
 
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Add mask check during flow lookup

[Jesse Gross]

Thanks, that makes sense.

On Mon, Jul 22, 2013 at 12:51 PM, Andy Zhou  wrote:
> The actual case I found this bug is more complicated to explain, but the
> following made up example can illustrate the problem. Consider we have 2
> kernel mega flows:
>
> 1)  in_port(1/0x),  *
> 2)  in_port(2/0x),  src_ip(1.1.1.1/255.255.255.255)
>
> They are not overlapping flows, however, when the following packet arrives
> (in_port(2), src_ip(2,2,2,2)). if we apply mask of 1), it could match flow
> 2). But it should not.
>
> Adding the mask check would have prevented this false match.
>
> --andy
>
>
> On Mon, Jul 22, 2013 at 9:57 AM, Jesse Gross  wrote:
>>
>> On Fri, Jul 19, 2013 at 11:11 AM, Andy Zhou  wrote:
>> > A mega flow matches when the masked key matches and the mask applied
>> > is the same as the mask used to create the mega flow.
>> >
>> > This patch adds the implementation of the second match condition
>> > mentioned above. Without this fix, mega flow lookup may result false
>> > match.
>> >
>> > Bug #18584
>> >
>> > Signed-off-by: Andy Zhou 
>>
>> Can you explain the conditions under which this can occur? It seems to
>> imply the overlapping flows are being installed, which is not supposed
>> to happen.
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] clang: Fix the alignment warning.

[Ben Pfaff]

On Mon, Jul 22, 2013 at 09:19:58AM -0700, Alex Wang wrote:
> This commit fixes the warning issued by 'clang' when pointer is casted
> to one with greater alignment.
> 
> Signed-off-by: Alex Wang 

Could you use the existing ofpbuf_at() instead of a new
ofpbuf_forward_ref()?  If there's no appropriate 'size' in a given
call then you can use 0.

It shouldn't be necessary to cast the return value of
ofpbuf_forward_ref() or ofpbuf_at(), generally, because they return
void *.

It isn't necessary to parenthesize the arguments to ALIGNED_CAST,
since the macro properly parenthesizes them internally.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2] sparse: Avoid sparse warnings for additional pthread initializers.

[Ben Pfaff]

Reported-by: Andy Zhou 
Signed-off-by: Ben Pfaff 
---
v1->v2: Avoid conflict with ovs-thread.h definitions.

 include/sparse/pthread.h |   11 ++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index 723c351..6cdf5c8 100644
--- a/include/sparse/pthread.h
+++ b/include/sparse/pthread.h
@@ -33,12 +33,21 @@ int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) 
OVS_RELEASES(rwlock);
 int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
 OVS_MUST_HOLD(mutex);
 
-/* Sparse complains about the proper PTHREAD_MUTEX_INITIALIZER definition.
+/* Sparse complains about the proper PTHREAD_*_INITIALIZER definitions.
  * Luckily, it's not a real compiler so we can overwrite it with something
  * simple. */
 #undef PTHREAD_MUTEX_INITIALIZER
 #define PTHREAD_MUTEX_INITIALIZER {}
 
+#undef PTHREAD_RWLOCK_INITIALIZER
+#define PTHREAD_RWLOCK_INITIALIZER {}
+
+#undef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
+#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP {}
+
+#undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
+#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
+
 #define pthread_mutex_trylock(MUTEX)\
 ({  \
 int retval = pthread_mutex_trylock(mutex);  \
-- 
1.7.2.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] clang: Fix the alignment warning.

[Alex Wang]

Thanks Ben, for the review,


> Signed-off-by: Alex Wang 
>
> Could you use the existing ofpbuf_at() instead of a new
> ofpbuf_forward_ref()?  If there's no appropriate 'size' in a given
> call then you can use 0.
>
> It shouldn't be necessary to cast the return value of
> ofpbuf_forward_ref() or ofpbuf_at(), generally, because they return
> void *.
>

I should have found and used ofpbuf_at(). Thanks for pointing out,


It isn't necessary to parenthesize the arguments to ALIGNED_CAST,
> since the macro properly parenthesizes them internally.
>

I'll adjust accordingly,
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] configure: Remove -Wno-override-init, add -Wextra to compile options.

[Ben Pfaff]

Applied, thank you.

On Fri, Jul 19, 2013 at 03:52:01PM -0700, Ethan Jackson wrote:
> Acked-by: Ethan Jackson 
> 
> 
> On Fri, Jul 19, 2013 at 10:57 AM, Ben Pfaff  wrote:
> > The build is clean against -Wextra at least with GCC 4.4.
> >
> > (-Wextra includes -Woverride-init.)
> >
> > Suggested-by: Alex Wang 
> > Signed-off-by: Ben Pfaff 
> > ---
> >  configure.ac |2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 2f3e474..170a852 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -89,6 +89,7 @@ OVS_CHECK_ATOMIC_ALWAYS_LOCK_FREE(8)
> >  OVS_CHECK_POSIX_AIO
> >
> >  OVS_ENABLE_OPTION([-Wall])
> > +OVS_ENABLE_OPTION([-Wextra])
> >  OVS_ENABLE_OPTION([-Wno-sign-compare])
> >  OVS_ENABLE_OPTION([-Wpointer-arith])
> >  OVS_ENABLE_OPTION([-Wdeclaration-after-statement])
> > @@ -102,7 +103,6 @@ OVS_ENABLE_OPTION([-Wstrict-prototypes])
> >  OVS_ENABLE_OPTION([-Wold-style-definition])
> >  OVS_ENABLE_OPTION([-Wmissing-prototypes])
> >  OVS_ENABLE_OPTION([-Wmissing-field-initializers])
> > -OVS_ENABLE_OPTION([-Wno-override-init])
> >  OVS_CONDITIONAL_CC_OPTION([-Wno-unused], [HAVE_WNO_UNUSED])
> >  OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], 
> > [HAVE_WNO_UNUSED_PARAMETER])
> >  OVS_ENABLE_WERROR
> > --
> > 1.7.2.5
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] sparse: Avoid sparse warnings for additional pthread initializers.

[Andy Zhou]

Looks good. It works now in my local build. Thanks.


On Mon, Jul 22, 2013 at 1:31 PM, Ben Pfaff  wrote:

> Reported-by: Andy Zhou 
> Signed-off-by: Ben Pfaff 
> ---
> v1->v2: Avoid conflict with ovs-thread.h definitions.
>
>  include/sparse/pthread.h |   11 ++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
> index 723c351..6cdf5c8 100644
> --- a/include/sparse/pthread.h
> +++ b/include/sparse/pthread.h
> @@ -33,12 +33,21 @@ int pthread_rwlock_unlock(pthread_rwlock_t *rwlock)
> OVS_RELEASES(rwlock);
>  int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
>  OVS_MUST_HOLD(mutex);
>
> -/* Sparse complains about the proper PTHREAD_MUTEX_INITIALIZER definition.
> +/* Sparse complains about the proper PTHREAD_*_INITIALIZER definitions.
>   * Luckily, it's not a real compiler so we can overwrite it with something
>   * simple. */
>  #undef PTHREAD_MUTEX_INITIALIZER
>  #define PTHREAD_MUTEX_INITIALIZER {}
>
> +#undef PTHREAD_RWLOCK_INITIALIZER
> +#define PTHREAD_RWLOCK_INITIALIZER {}
> +
> +#undef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
> +#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP {}
> +
> +#undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> +#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
> +
>  #define pthread_mutex_trylock(MUTEX)\
>  ({  \
>  int retval = pthread_mutex_trylock(mutex);  \
> --
> 1.7.2.5
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] sparse: Avoid sparse warnings for additional pthread initializers.

[Ben Pfaff]

Thanks, I applied this version.

On Mon, Jul 22, 2013 at 01:55:34PM -0700, Andy Zhou wrote:
> Looks good. It works now in my local build. Thanks.
> 
> 
> On Mon, Jul 22, 2013 at 1:31 PM, Ben Pfaff  wrote:
> 
> > Reported-by: Andy Zhou 
> > Signed-off-by: Ben Pfaff 
> > ---
> > v1->v2: Avoid conflict with ovs-thread.h definitions.
> >
> >  include/sparse/pthread.h |   11 ++-
> >  1 files changed, 10 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
> > index 723c351..6cdf5c8 100644
> > --- a/include/sparse/pthread.h
> > +++ b/include/sparse/pthread.h
> > @@ -33,12 +33,21 @@ int pthread_rwlock_unlock(pthread_rwlock_t *rwlock)
> > OVS_RELEASES(rwlock);
> >  int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
> >  OVS_MUST_HOLD(mutex);
> >
> > -/* Sparse complains about the proper PTHREAD_MUTEX_INITIALIZER definition.
> > +/* Sparse complains about the proper PTHREAD_*_INITIALIZER definitions.
> >   * Luckily, it's not a real compiler so we can overwrite it with something
> >   * simple. */
> >  #undef PTHREAD_MUTEX_INITIALIZER
> >  #define PTHREAD_MUTEX_INITIALIZER {}
> >
> > +#undef PTHREAD_RWLOCK_INITIALIZER
> > +#define PTHREAD_RWLOCK_INITIALIZER {}
> > +
> > +#undef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
> > +#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP {}
> > +
> > +#undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> > +#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
> > +
> >  #define pthread_mutex_trylock(MUTEX)\
> >  ({  \
> >  int retval = pthread_mutex_trylock(mutex);  \
> > --
> > 1.7.2.5
> >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] sparse warning

[Andy Zhou]

Sure, will send out a patch soon.


On Mon, Jul 22, 2013 at 12:44 PM, Jesse Gross  wrote:

> On Mon, Jul 22, 2013 at 10:58 AM, Ben Pfaff  wrote:
> > I'm getting the following warning on current master (commit
> > dc0d542d5254c1e6 "datapath: Conditionally define skb_unclone in
> > datapath compat code"):
> >
> > datapath/flow.c:1051:32: error: incompatible types in comparison
> > expression (different address spaces)
> >
> > That's in ovs_masked_flow_lookup():
> >
> > hlist_for_each_entry_rcu(flow, head, hash_node[table->node_ver])
> {
> > 1051 -> if (flow->mask == mask &&
> > __flow_cmp_key(flow, &masked_key, key_start,
> key_len))
> > return flow;
> > }
>
> This is due to a missing RCU annotation when dereferencing flow->mask.
> It's not a real problem because the mask never changes once assigned
> to a flow so the use of RCU is really somewhat limited here.
>
> My inclination is to just remove the RCU annotation from the mask in
> the flow declaration (and corresponding assignments/dereferences)
> since they are somewhat misleading when it comes to flows.
>
> Andy, can you take a look?
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] datapath: remove RCU annotation from flow->mask

[Andy Zhou]

After a mask is assigned to a flow, it will not change for the life of
the flow. Since flow access is protected by RCU lock, access to
flow->mask after getting a flow is always safe.

Suggested-by: Jesse Gross 
---
 datapath/datapath.c |6 ++
 datapath/flow.c |5 ++---
 datapath/flow.h |2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9a3a07b..4330ce3 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1127,7 +1127,6 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, 
struct datapath *dp,
  u32 seq, u32 flags, u8 cmd)
 {
const int skb_orig_len = skb->len;
-   struct sw_flow_mask *mask;
struct nlattr *start;
struct ovs_flow_stats stats;
struct ovs_header *ovs_header;
@@ -1157,8 +1156,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, 
struct datapath *dp,
if (!nla)
goto nla_put_failure;
 
-   mask = rcu_dereference_check(flow->mask, lockdep_ovsl_is_held());
-   err = ovs_flow_to_nlattrs(&flow->key, &mask->key, skb);
+   err = ovs_flow_to_nlattrs(&flow->key, &flow->mask->key, skb);
if (err)
goto error;
 
@@ -1344,7 +1342,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
}
 
ovs_sw_flow_mask_add_ref(mask_p);
-   rcu_assign_pointer(flow->mask, mask_p);
+   flow->mask = mask_p;
rcu_assign_pointer(flow->sf_acts, acts);
 
/* Put flow in bucket. */
diff --git a/datapath/flow.c b/datapath/flow.c
index 95fea7f..1a0786e 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1073,9 +1073,8 @@ struct sw_flow *ovs_flow_lookup(struct flow_table *tbl,
 
 void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow)
 {
-   flow->hash = ovs_flow_hash(&flow->key,
-   ovsl_dereference(flow->mask)->range.start,
-   ovsl_dereference(flow->mask)->range.end);
+   flow->hash = ovs_flow_hash(&flow->key, flow->mask->range.start,
+   flow->mask->range.end);
__tbl_insert(table, flow);
 }
 
diff --git a/datapath/flow.h b/datapath/flow.h
index 1a3764e..59c7f6e 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -120,7 +120,7 @@ struct sw_flow {
 
struct sw_flow_key key;
struct sw_flow_key unmasked_key;
-   struct sw_flow_mask __rcu *mask;
+   struct sw_flow_mask *mask;
struct sw_flow_actions __rcu *sf_acts;
 
spinlock_t lock;/* Lock for values below. */
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: remove RCU annotation from flow->mask

[Ben Pfaff]

On Mon, Jul 22, 2013 at 02:28:04PM -0700, Andy Zhou wrote:
> After a mask is assigned to a flow, it will not change for the life of
> the flow. Since flow access is protected by RCU lock, access to
> flow->mask after getting a flow is always safe.
> 
> Suggested-by: Jesse Gross 

This patch fixes the warning for me.

(This is not a review of the patch itself.)

Tested-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: remove RCU annotation from flow->mask

[Andy Zhou]

Sorry forgot to mention Ben reported this issue. Jesse, would you please
add it before committing?


On Mon, Jul 22, 2013 at 2:30 PM, Ben Pfaff  wrote:

> On Mon, Jul 22, 2013 at 02:28:04PM -0700, Andy Zhou wrote:
> > After a mask is assigned to a flow, it will not change for the life of
> > the flow. Since flow access is protected by RCU lock, access to
> > flow->mask after getting a flow is always safe.
> >
> > Suggested-by: Jesse Gross 
>
> This patch fixes the warning for me.
>
> (This is not a review of the patch itself.)
>
> Tested-by: Ben Pfaff 
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: remove RCU annotation from flow->mask

[Jesse Gross]

OK, although the patch also needs a signed-off-by from you and I think
that we can also remove an ugly cast from ovs_flow_free(). Do you just
want to send out a new version?

On Mon, Jul 22, 2013 at 2:35 PM, Andy Zhou  wrote:
> Sorry forgot to mention Ben reported this issue. Jesse, would you please add
> it before committing?
>
>
> On Mon, Jul 22, 2013 at 2:30 PM, Ben Pfaff  wrote:
>>
>> On Mon, Jul 22, 2013 at 02:28:04PM -0700, Andy Zhou wrote:
>> > After a mask is assigned to a flow, it will not change for the life of
>> > the flow. Since flow access is protected by RCU lock, access to
>> > flow->mask after getting a flow is always safe.
>> >
>> > Suggested-by: Jesse Gross 
>>
>> This patch fixes the warning for me.
>>
>> (This is not a review of the patch itself.)
>>
>> Tested-by: Ben Pfaff 
>
>
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [Remove rcu mask v2] datapath: remove RCU annotation from flow->mask

[Andy Zhou]

After a mask is assigned to a flow, it will not change for the life of
the flow. Since flow access is protected by RCU lock, access to
flow->mask after getting a flow is always safe.

Suggested-by: Jesse Gross 
Reported-by: Ben Pfaff 
Signed-off-by: Andy Zhou 


V1 - V2: remove cast from ovs_flow_free()

---
 datapath/datapath.c |6 ++
 datapath/flow.c |8 +++-
 datapath/flow.h |2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9a3a07b..4330ce3 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1127,7 +1127,6 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, 
struct datapath *dp,
  u32 seq, u32 flags, u8 cmd)
 {
const int skb_orig_len = skb->len;
-   struct sw_flow_mask *mask;
struct nlattr *start;
struct ovs_flow_stats stats;
struct ovs_header *ovs_header;
@@ -1157,8 +1156,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, 
struct datapath *dp,
if (!nla)
goto nla_put_failure;
 
-   mask = rcu_dereference_check(flow->mask, lockdep_ovsl_is_held());
-   err = ovs_flow_to_nlattrs(&flow->key, &mask->key, skb);
+   err = ovs_flow_to_nlattrs(&flow->key, &flow->mask->key, skb);
if (err)
goto error;
 
@@ -1344,7 +1342,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
}
 
ovs_sw_flow_mask_add_ref(mask_p);
-   rcu_assign_pointer(flow->mask, mask_p);
+   flow->mask = mask_p;
rcu_assign_pointer(flow->sf_acts, acts);
 
/* Put flow in bucket. */
diff --git a/datapath/flow.c b/datapath/flow.c
index 95fea7f..c6a90b9 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -638,8 +638,7 @@ void ovs_flow_free(struct sw_flow *flow, bool deferred)
if (!flow)
return;
 
-   ovs_sw_flow_mask_del_ref((struct sw_flow_mask __force *)flow->mask,
-deferred);
+   ovs_sw_flow_mask_del_ref(flow->mask, deferred);
 
if (deferred)
call_rcu(&flow->rcu, rcu_free_flow_callback);
@@ -1073,9 +1072,8 @@ struct sw_flow *ovs_flow_lookup(struct flow_table *tbl,
 
 void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow)
 {
-   flow->hash = ovs_flow_hash(&flow->key,
-   ovsl_dereference(flow->mask)->range.start,
-   ovsl_dereference(flow->mask)->range.end);
+   flow->hash = ovs_flow_hash(&flow->key, flow->mask->range.start,
+   flow->mask->range.end);
__tbl_insert(table, flow);
 }
 
diff --git a/datapath/flow.h b/datapath/flow.h
index 1a3764e..59c7f6e 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -120,7 +120,7 @@ struct sw_flow {
 
struct sw_flow_key key;
struct sw_flow_key unmasked_key;
-   struct sw_flow_mask __rcu *mask;
+   struct sw_flow_mask *mask;
struct sw_flow_actions __rcu *sf_acts;
 
spinlock_t lock;/* Lock for values below. */
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: remove RCU annotation from flow->mask

[Andy Zhou]

Done.


On Mon, Jul 22, 2013 at 2:53 PM, Jesse Gross  wrote:

> OK, although the patch also needs a signed-off-by from you and I think
> that we can also remove an ugly cast from ovs_flow_free(). Do you just
> want to send out a new version?
>
> On Mon, Jul 22, 2013 at 2:35 PM, Andy Zhou  wrote:
> > Sorry forgot to mention Ben reported this issue. Jesse, would you please
> add
> > it before committing?
> >
> >
> > On Mon, Jul 22, 2013 at 2:30 PM, Ben Pfaff  wrote:
> >>
> >> On Mon, Jul 22, 2013 at 02:28:04PM -0700, Andy Zhou wrote:
> >> > After a mask is assigned to a flow, it will not change for the life of
> >> > the flow. Since flow access is protected by RCU lock, access to
> >> > flow->mask after getting a flow is always safe.
> >> >
> >> > Suggested-by: Jesse Gross 
> >>
> >> This patch fixes the warning for me.
> >>
> >> (This is not a review of the patch itself.)
> >>
> >> Tested-by: Ben Pfaff 
> >
> >
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [Remove rcu mask v2] datapath: remove RCU annotation from flow->mask

[Jesse Gross]

On Mon, Jul 22, 2013 at 3:08 PM, Andy Zhou  wrote:
> After a mask is assigned to a flow, it will not change for the life of
> the flow. Since flow access is protected by RCU lock, access to
> flow->mask after getting a flow is always safe.
>
> Suggested-by: Jesse Gross 
> Reported-by: Ben Pfaff 
> Signed-off-by: Andy Zhou 

Applied, thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 3/3] clang: Fix the alignment warning.

[Alex Wang]

This commit fixes the warning issued by 'clang' when pointer is casted
to one with greater alignment.

Signed-off-by: Alex Wang 
---

v1 -> v2:
1. use ofpbuf_at() instead of creating ofpbuf_forward_ref()
2. remove the unnecessary parenthesis in argument to ALIGNED_CAST()

---
 lib/hash.c |6 +++---
 lib/jhash.c|   10 +-
 lib/mac-learning.c |4 ++--
 lib/netdev-linux.c |3 ++-
 lib/netlink.c  |5 ++---
 lib/nx-match.c |   11 ++-
 lib/nx-match.h |2 +-
 lib/odp-util.c |3 ++-
 lib/ofp-actions.c  |   27 ++-
 lib/ofp-actions.h  |2 +-
 lib/ofp-util.c |7 ---
 lib/ovs-atomic-gcc4+.h |4 ++--
 lib/packets.c  |8 +---
 lib/route-table.c  |2 +-
 lib/rtnetlink-link.c   |3 +--
 lib/socket-util.c  |5 +++--
 lib/stream-tcp.c   |3 ++-
 lib/util.h |4 
 utilities/ovs-dpctl.c  |3 ++-
 utilities/ovs-ofctl.c  |4 ++--
 20 files changed, 64 insertions(+), 52 deletions(-)

diff --git a/lib/hash.c b/lib/hash.c
index e954d78..8f34493 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -29,15 +29,15 @@ hash_3words(uint32_t a, uint32_t b, uint32_t c)
 uint32_t
 hash_bytes(const void *p_, size_t n, uint32_t basis)
 {
-const uint8_t *p = p_;
+const uint32_t *p = p_;
 size_t orig_n = n;
 uint32_t hash;
 
 hash = basis;
 while (n >= 4) {
-hash = mhash_add(hash, get_unaligned_u32((const uint32_t *) p));
+hash = mhash_add(hash, get_unaligned_u32(p));
 n -= 4;
-p += 4;
+p += 1;
 }
 
 if (n) {
diff --git a/lib/jhash.c b/lib/jhash.c
index 4ec2871..c08c368 100644
--- a/lib/jhash.c
+++ b/lib/jhash.c
@@ -96,18 +96,18 @@ jhash_words(const uint32_t *p, size_t n, uint32_t basis)
 uint32_t
 jhash_bytes(const void *p_, size_t n, uint32_t basis)
 {
-const uint8_t *p = p_;
+const uint32_t *p = p_;
 uint32_t a, b, c;
 
 a = b = c = 0xdeadbeef + n + basis;
 
 while (n >= 12) {
-a += get_unaligned_u32((uint32_t *) p);
-b += get_unaligned_u32((uint32_t *) (p + 4));
-c += get_unaligned_u32((uint32_t *) (p + 8));
+a += get_unaligned_u32(p);
+b += get_unaligned_u32(p + 1);
+c += get_unaligned_u32(p + 2);
 jhash_mix(&a, &b, &c);
 n -= 12;
-p += 12;
+p += 3;
 }
 
 if (n) {
diff --git a/lib/mac-learning.c b/lib/mac-learning.c
index ca0ccb8..e2ca02b 100644
--- a/lib/mac-learning.c
+++ b/lib/mac-learning.c
@@ -49,8 +49,8 @@ static uint32_t
 mac_table_hash(const struct mac_learning *ml, const uint8_t mac[ETH_ADDR_LEN],
uint16_t vlan)
 {
-unsigned int mac1 = get_unaligned_u32((uint32_t *) mac);
-unsigned int mac2 = get_unaligned_u16((uint16_t *) (mac + 4));
+unsigned int mac1 = get_unaligned_u32(ALIGNED_CAST(uint32_t *, mac));
+unsigned int mac2 = get_unaligned_u16(ALIGNED_CAST(uint16_t *, mac + 4));
 return hash_3words(mac1, mac2 | (vlan << 16), ml->secret);
 }
 
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 05877c1..c2f09ea 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -4546,7 +4546,8 @@ netdev_linux_get_ipv4(const struct netdev *netdev, struct 
in_addr *ip,
 ifr.ifr_addr.sa_family = AF_INET;
 error = netdev_linux_do_ioctl(netdev_get_name(netdev), &ifr, cmd, 
cmd_name);
 if (!error) {
-const struct sockaddr_in *sin = (struct sockaddr_in *) &ifr.ifr_addr;
+const struct sockaddr_in *sin = ALIGNED_CAST(struct sockaddr_in *,
+ &ifr.ifr_addr);
 *ip = sin->sin_addr;
 }
 return error;
diff --git a/lib/netlink.c b/lib/netlink.c
index a686784..9ed925b 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -712,7 +712,7 @@ nl_policy_parse(const struct ofpbuf *msg, size_t nla_offset,
 }
 
 NL_ATTR_FOR_EACH (nla, left,
-  (struct nlattr *) ((char *) msg->data + nla_offset),
+  (struct nlattr *) ofpbuf_at(msg, nla_offset, 0),
   msg->size - nla_offset)
 {
 uint16_t type = nl_attr_type(nla);
@@ -777,8 +777,7 @@ nl_attr_find__(const struct nlattr *attrs, size_t size, 
uint16_t type)
 const struct nlattr *
 nl_attr_find(const struct ofpbuf *buf, size_t hdr_len, uint16_t type)
 {
-const uint8_t *start = (const uint8_t *) buf->data + hdr_len;
-return nl_attr_find__((const struct nlattr *) start, buf->size - hdr_len,
+return nl_attr_find__(ofpbuf_at(buf, hdr_len, 0), buf->size - hdr_len,
   type);
 }
 
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 3a6d7cc..bdb3a2b 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -744,7 +744,7 @@ oxm_put_match(struct ofpbuf *b, const struct match *match)
 match_len = nx_put_raw(b, true, match, cookie, cookie_mask) + sizeof *omh;
 ofpbuf_put_zeros(b, ROUND_UP(match_len, 8) - match_

[ovs-dev] Oferta împrumut rapid se aplică acum

[Mr.James White]

-- 
Oferim creditele acordate persoanelor fizice, firme și să coopereze
organelor de 2%
Rata de retrage o sumă minimă puteți împrumuta este
$ 2,000.00 dolari SUA pentru a oferi un maxim de $ 10.Million.Please
în urma:

Http {MR., Pr.., MS., DR, etc.}
1) Numele tău ...
2) țara dumneavoastră 
3) ocupația dumneavoastră .
4) Starea civila .
5) Numărul de telefon 
6) venitul lunar ..
7) ADRESA .
8) Scopul creditului .
9) Cererea de împrumut 
10) Telefon ..
11) În ceea ce privește durata împrumutului  Compania noastră
cutie poștală de contact este
prin> jameswhitefinanceh...@yahoo.com
Sir.James alb
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] vlan-splinter: Fix a bug.

[Alex Wang]

When "other-config:enable-vlan-splinters=true" is set, the existing
vlans with ip address must be retained. The bug actually does the
opposite and retains the vlans without ip address. This commit fixes
it.

Reported-by: Roman Sokolkov 
Signed-off-by: Alex Wang 
---
 vswitchd/bridge.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a73379c..1460ea2 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -4090,10 +4090,10 @@ collect_splinter_vlans(const struct ovsrec_open_vswitch 
*ovs_cfg)
 if (!netdev_open(vlan_dev->name, "system", &netdev)) {
 if (!netdev_get_in4(netdev, NULL, NULL) ||
 !netdev_get_in6(netdev, NULL)) {
-vlandev_del(vlan_dev->name);
-} else {
 /* It has an IP address configured, so we don't own
  * it.  Don't delete it. */
+} else {
+vlandev_del(vlan_dev->name);
 }
 netdev_close(netdev);
 }
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [mptcp-dev] MPTCP v0.87 - Release Candidate

[Vasiliy Tolstov]

Very good. Is that possible to use mptcp with openvswitch in top of it?
For example openvswitch able to aggregate two eth links to bond
interface with lacp : active-passive, but not able to create 802.3ad
aggregation.

2013/7/12 Christoph Paasch :
> Hello,
>
> there are two weeks left until the next IETF-meeting -
> thus two weeks left until the new stable MPTCP release -
> two weeks left to test the release-candidate before the "official" release :)
>
> The release-candidate is now available in branch mptcp_v0.87 of
> https://github.com/multipath-tcp/mptcp/tree/mptcp_v0.87 .
>
> I encourage everybody to go and try it out, and report any bugs you might
> encounter.
>
>
> This MPTCP-release is based on Linux 3.10. A stable backport will be
> maintained for Linux 3.8 (to be able to use MPTCP with Ubuntu Raring). If
> somebody needs another backport of MPTCP (between 3.6 and 3.10), let us know
> and we will see what can be done.
>
> The new features are mainly on the performance-improvement side:
> - Hardware-offloading
> - Zero-copy support
> - NET_DMA support
> - Proper support for NFS
> - 'ip link set dev [itf] multipath off' now works on existing connections
>
>
> Cheers,
> Christoph
>
>
> MPTCP-developpers Mailing-List
> Visit https://scm.info.ucl.ac.be/trac/mptcp/ to access the source-code of 
> MPTCP in the Linux Kernel
>
> To Unsubscribe, visit https://listes-2.sipr.ucl.ac.be/sympa/info/mptcp-dev
>



-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev