dpif: Don't rely on caller to keep dpif arg valid, in dpifmon_create().

dpifmon_create() stored away the dpif pointer that it was passed and
continued to use it, but ofproto_create() didn't keep that dpif in a
constant place in memory, so dpifmon_poll() would randomly fail with
errors like "Bad file descriptor".  Having the dpifmon keep its own
fd to a dpif is more reliable.

Additional possible fix to Dan's problem.

diff --git a/lib/dpif.c b/lib/dpif.c
index eba08551eab6e4a233f93fc6d609e052c75a0fef..fd7a8711b843442d392f934ee83f680369845649 100644 (file)
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -534,51 +534,58 @@ dpif_recv_wait(struct dpif *dpif)
 }
 \f
 struct dpifmon {
-    const struct dpif *dpif;
+    struct dpif dpif;
     struct nl_sock *sock;
     int local_ifindex;
 };
 
 int
-dpifmon_create(const struct dpif *dpif, struct dpifmon **monp)
+dpifmon_create(const char *datapath_name, struct dpifmon **monp)
 {
-    struct nl_sock *sock;
     struct dpifmon *mon;
     struct odp_port local;
-    unsigned int local_ifindex;
     int error;
 
-    *monp = NULL;
+    mon = *monp = xmalloc(sizeof *mon);
 
-    error = dpif_port_query_by_number(dpif, ODPP_LOCAL, &local);
+    error = dpif_open(datapath_name, &mon->dpif);
     if (error) {
-        return error;
+        goto error;
+    }
+    error = dpif_port_query_by_number(&mon->dpif, ODPP_LOCAL, &local);
+    if (error) {
+        goto error_close_dpif;
     }
 
-    local_ifindex = if_nametoindex(local.devname);
-    if (!local_ifindex) {
+    mon->local_ifindex = if_nametoindex(local.devname);
+    if (!mon->local_ifindex) {
+        error = errno;
         VLOG_WARN("could not get ifindex of %s device: %s",
                   local.devname, strerror(errno));
-        return errno;
+        goto error_close_dpif;
     }
 
-    error = nl_sock_create(NETLINK_ROUTE, RTNLGRP_LINK, 0, 0, &sock);
+    error = nl_sock_create(NETLINK_ROUTE, RTNLGRP_LINK, 0, 0, &mon->sock);
     if (error) {
         VLOG_WARN("could not create rtnetlink socket: %s", strerror(error));
-        return error;
+        goto error_close_dpif;
     }
 
-    mon = *monp = xmalloc(sizeof *mon);
-    mon->dpif = dpif;
-    mon->sock = sock;
-    mon->local_ifindex = local_ifindex;
     return 0;
+
+error_close_dpif:
+    dpif_close(&mon->dpif);
+error:
+    free(mon);
+    *monp = NULL;
+    return error;
 }
 
 void
 dpifmon_destroy(struct dpifmon *mon)
 {
     if (mon) {
+        dpif_close(&mon->dpif);
         nl_sock_destroy(mon->sock);
     }
 }
@@ -614,7 +621,7 @@ again:
                 for_us = master_ifindex == mon->local_ifindex;
             } else {
                 struct odp_port odp_port;
-                for_us = (dpif_port_query_by_name(mon->dpif, devname,
+                for_us = (dpif_port_query_by_name(&mon->dpif, devname,
                                                   &odp_port) == 0);
             }
 

diff --git a/lib/dpif.h b/lib/dpif.h
index 590de1eadb39b8977c002923e057c3c0d9909dca..c8e7a437179c5212ac19c6466e2a2d3e8ef8bad4 100644 (file)
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -102,7 +102,7 @@ void dpif_recv_wait(struct dpif *);
 \f
 struct dpifmon;
 
-int dpifmon_create(const struct dpif *, struct dpifmon **);
+int dpifmon_create(const char *datapath_name, struct dpifmon **);
 void dpifmon_destroy(struct dpifmon *);
 
 int dpifmon_poll(struct dpifmon *, char **devnamep);

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 4fbd4d5c40664e820061f4dc86d767c0bdeda83b..c466ce3defd8890e04aa68bfbb114b6bb24bcfa7 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -225,7 +225,7 @@ ofproto_create(const char *datapath, struct ofproto **ofprotop)
     dpif_flow_flush(&dpif);
 
     /* Start monitoring datapath ports for status changes. */
-    error = dpifmon_create(&dpif, &dpifmon);
+    error = dpifmon_create(datapath, &dpifmon);
     if (error) {
         VLOG_ERR("failed to starting monitoring datapath %s: %s",
                  datapath, strerror(error));