[ovs-dev] [PATCH v3] dpif: Return ENODEV from dpif_port_query_by_*() if there's no port.

[Daniele Di Proietto]

bridge_delete_or_reconfigure() deletes every interface that's not dumped
by OFPROTO_PORT_FOR_EACH().  ofproto_dpif.c:port_dump_next(), used by
OFPROTO_PORT_FOR_EACH, checks if the ofport is in the datapath by
calling port_query_by_name().  If port_query_by_name() returns an error,
the dump is interrupted.  If port_query_by_name() returns ENODEV, the
device doesn't exist and the dump can continue.

port_query_by_name() for the userspace datapath returns ENOENT instead
of ENODEV.  This is expected by dpif_port_query_by_name(), but it's not
handled correctly by port_dump_next().

dpif-netdev handles reconfiguration errors for an interface by deleting
it from the datapath, so it's possible that a device is missing. When this
happens we must make sure that port_dump_next() continues to dump other
devices, otherwise they will be deleted and the two layers will have an
inconsistent view.

This commit fixes the problem by returning ENODEV from the userspace
datapath if the port doesn't exist, and by documenting this clearly in
the dpif interfaces.

The problem was found while developing new code.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
---
v3: Return ENODEV instead of ENOENT from dpif-netdev. Document that ENODEV
    means that the port doesn't exist, other error numbers indicate problems.

---
 lib/dpif-netdev.c   |  7 +++++--
 lib/dpif-provider.h |  4 ++++
 lib/dpif.c          | 11 +++++++----
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 0b73056a8..1953bb37c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1435,7 +1435,7 @@ get_port_by_number(struct dp_netdev *dp,
         return EINVAL;
     } else {
         *portp = dp_netdev_lookup_port(dp, port_no);
-        return *portp ? 0 : ENOENT;
+        return *portp ? 0 : ENODEV;
     }
 }
 
@@ -1473,7 +1473,10 @@ get_port_by_name(struct dp_netdev *dp,
             return 0;
         }
     }
-    return ENOENT;
+
+    /* Callers of dpif_netdev_port_query_by_name() expect ENODEV for a non
+     * existing port. */
+    return ENODEV;
 }
 
 static int
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 56b88f93d..d3b2bb91d 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -176,6 +176,10 @@ struct dpif_class {
      * If 'port' is not null, stores information about the port into
      * '*port' if successful.
      *
+     * If the port doesn't exist, the provider must return ENODEV.  Other
+     * error numbers means that something wrong happened and will be
+     * treated differently by upper layers.
+     *
      * If 'port' is not null, the caller takes ownership of data in
      * 'port' and must free it with dpif_port_destroy() when it is no
      * longer needed. */
diff --git a/lib/dpif.c b/lib/dpif.c
index 53958c559..cc4936c70 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -602,7 +602,7 @@ bool
 dpif_port_exists(const struct dpif *dpif, const char *devname)
 {
     int error = dpif->dpif_class->port_query_by_name(dpif, devname, NULL);
-    if (error != 0 && error != ENOENT && error != ENODEV) {
+    if (error != 0 && error != ENODEV) {
         VLOG_WARN_RL(&error_rl, "%s: failed to query port %s: %s",
                      dpif_name(dpif), devname, ovs_strerror(error));
     }
@@ -631,6 +631,8 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no,
  * initializes '*port' appropriately; on failure, returns a positive errno
  * value.
  *
+ * Retuns ENODEV if the port doesn't exist.
+ *
  * The caller owns the data in 'port' and must free it with
  * dpif_port_destroy() when it is no longer needed. */
 int
@@ -653,6 +655,8 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no,
  * initializes '*port' appropriately; on failure, returns a positive errno
  * value.
  *
+ * Retuns ENODEV if the port doesn't exist.
+ *
  * The caller owns the data in 'port' and must free it with
  * dpif_port_destroy() when it is no longer needed. */
 int
@@ -666,12 +670,11 @@ dpif_port_query_by_name(const struct dpif *dpif, const char *devname,
     } else {
         memset(port, 0, sizeof *port);
 
-        /* For ENOENT or ENODEV we use DBG level because the caller is probably
+        /* For ENODEV we use DBG level because the caller is probably
          * interested in whether 'dpif' actually has a port 'devname', so that
          * it's not an issue worth logging if it doesn't.  Other errors are
          * uncommon and more likely to indicate a real problem. */
-        VLOG_RL(&error_rl,
-                error == ENOENT || error == ENODEV ? VLL_DBG : VLL_WARN,
+        VLOG_RL(&error_rl, error == ENODEV ? VLL_DBG : VLL_WARN,
                 "%s: failed to query port %s: %s",
                 dpif_name(dpif), devname, ovs_strerror(error));
     }
-- 
2.11.0