diff mbox

[ovs-dev,RFC,4/5] dpctl: uses open_type when calling netdev_open

Message ID CAExiqTK7tmfsJZnwKf4ahByuC2YKtTThE3Xcf1DxCC2APaoOWw@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Daniele Di Proietto July 25, 2016, 6:03 p.m. UTC 
2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo <cascardo@redhat.com
>:

> On Fri, Jul 22, 2016 at 02:49:39PM -0700, Daniele Di Proietto wrote:
> > I would prefer if dpctl kept using the datapath types.  The translation
> > from database types to datapath type should happen in ofproto, dpctl is
> > supposed to be used to interact with the datapath directly.
> >
> > What do you guys think?
> >
> > The rest of the series looks good to me as well.
> >
> > Thanks,
> >
> > Daniele
> >
>
>
Hi Cascardo,

Thanks for the detailed analysis.  The problem is that there are three
types:

a) the database type
b) the port type in dpif-netdev
c) the netdev type

I was assuming that b and c are always equal, but they're not.  The only
case
when they're not equal is the "ovs-netdev" (or "ovs-dummy") port.

I think we can easily remove this case and make b and c always equal
with the following changes:

---8<---
         dp_netdev_free(dp);
@@ -1129,7 +1131,7 @@ hash_port_no(odp_port_t port_no)
 }

 static int
-port_create(const char *devname, const char *open_type, const char *type,
+port_create(const char *devname, const char *type,
             odp_port_t port_no, struct dp_netdev_port **portp)
 {
     struct netdev_saved_flags *sf;
@@ -1142,7 +1144,7 @@ port_create(const char *devname, const char
*open_type, const char *type,
     *portp = NULL;

     /* Open and validate network device. */
-    error = netdev_open(devname, open_type, &netdev);
+    error = netdev_open(devname, type, &netdev);
     if (error) {
         return error;
     }
@@ -1233,8 +1235,7 @@ do_add_port(struct dp_netdev *dp, const char
*devname, const char *type,
         return EEXIST;
     }

-    error = port_create(devname, dpif_netdev_port_open_type(dp->class,
type),
-                        type, port_no, &port);
+    error = port_create(devname, type, port_no, &port);
     if (error) {
         return error;
     }
---8<---

With this incremental case 2 is covered, dpctl/show always shows the
datapath type.
(The incremental also requires some testsuite changes)

For case 1 and 3 is just a matter of deciding if we want to support database
types (in addition to netdev) in dpctl.  I would lean towards no: I find it
confusing
that both types are acceptable.

That said, I feel like I'm nitpicking, dpctl is a tool used for debugging
and I think
we should do whatever comes easier.

Thanks,

Daniele

Hi, Daniele.
>
> Thanks for the comment.
>
> The best example that breaks currently is the internal type on the netdev
> datapath.
>
> There are three scenarios in dpctl, and two of them are changed with this
> patch.
>
> 1) The user input type, given for add-if. In this particular case, the code
> would try to use the "internal" type, but thinks would break as the Linux
> internal type would not work for the netdev datapath. The user was
> expected to
> use "tap" instead. We the patch applied, either way should work. So we are
> not
> breaking behavior.
>
> 2) When the type is output to the user. The current patch does not change
> its
> behavior, which basically prints what we get from dpif_port_query. However,
> dpif-netdev uses a cache of the database type and returns that, not the
> netdev
> type. That could be changed, but then the output to the user would be the
> real
> dpif. Would that be acceptable? It could break scripts out there which
> look for
> internal. Or would dpctl do the reverse mapping? That would require some
> new
> code on the dpif level.
>

> 3) Using the dpif_port.type in netdev_open. We could use the same
> alternative
> solution here as proposed for scenario 2. Make dpif_port_query return the
> real
> netdev type. This would require the new reverse mapping in order to be
> used also
> at the ofproto level, so ofproto_port_query uses the database type.
>
> I have a patch that changes dpif-netdev to return the netdev open type.
> However,
> as it broke dpctl output, I decided to provice this current patch instead,
> as I
> realized a lot more would be needed on other code, and we risked breaking
> a lot
> of behavior.
>
> I would keep this patch as is in the series, as it keeps behavior, and
> improves
> the case in add-if, without breaking it; and leave the dpif_port_query fix
> and
> reverse mapping to another day.
>
> What do you think?
>
> Regards.
> Cascardo.
>
>
> > 2016-07-18 10:00 GMT-07:00 Thadeu Lima de Souza Cascardo <
> > cascardo@redhat.com>:
> >
> > > dpctl uses a user or database defined type when calling netdev_open.
> > > Instead, it
> > > should use the type from dpif_port_open_type. Otherwise, when using the
> > > internal
> > > type, it could open it with that type instead of the correct one, which
> > > would be
> > > tap or dummy.
> > > ---
> > >  lib/dpctl.c | 17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > > index 003602a..f896161 100644
> > > --- a/lib/dpctl.c
> > > +++ b/lib/dpctl.c
> > > @@ -274,7 +274,8 @@ dpctl_add_if(int argc OVS_UNUSED, const char
> *argv[],
> > >              }
> > >          }
> > >
> > > -        error = netdev_open(name, type, &netdev);
> > > +        error = netdev_open(name, dpif_port_open_type(dpif_type(dpif),
> > > type),
> > > +                            &netdev);
> > >          if (error) {
> > >              dpctl_error(dpctl_p, error, "%s: failed to open network
> > > device",
> > >                          name);
> > > @@ -356,7 +357,8 @@ dpctl_set_if(int argc, const char *argv[], struct
> > > dpctl_params *dpctl_p)
> > >          dpif_port_destroy(&dpif_port);
> > >
> > >          /* Retrieve its existing configuration. */
> > > -        error = netdev_open(name, type, &netdev);
> > > +        error = netdev_open(name, dpif_port_open_type(dpif_type(dpif),
> > > type),
> > > +                            &netdev);
> > >          if (error) {
> > >              dpctl_error(dpctl_p, error, "%s: failed to open network
> > > device",
> > >                          name);
> > > @@ -558,10 +560,13 @@ show_dpif(struct dpif *dpif, struct dpctl_params
> > > *dpctl_p)
> > >      qsort(port_nos, n_port_nos, sizeof *port_nos, compare_port_nos);
> > >
> > >      for (int i = 0; i < n_port_nos; i++) {
> > > +        const char *type;
> > >          if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port))
> {
> > >              continue;
> > >          }
> > >
> > > +        type = dpif_port_open_type(dpif_type(dpif), dpif_port.type);
> > > +
> > >          dpctl_print(dpctl_p, "\tport %u: %s",
> > >                      dpif_port.port_no, dpif_port.name);
> > >
> > > @@ -570,7 +575,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params
> > > *dpctl_p)
> > >
> > >              dpctl_print(dpctl_p, " (%s", dpif_port.type);
> > >
> > > -            error = netdev_open(dpif_port.name, dpif_port.type,
> &netdev);
> > > +            error = netdev_open(dpif_port.name, type, &netdev);
> > >              if (!error) {
> > >                  struct smap config;
> > >
> > > @@ -603,7 +608,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params
> > > *dpctl_p)
> > >              struct netdev_stats s;
> > >              int error;
> > >
> > > -            error = netdev_open(dpif_port.name, dpif_port.type,
> &netdev);
> > > +            error = netdev_open(dpif_port.name, type, &netdev);
> > >              if (error) {
> > >                  dpctl_print(dpctl_p, ", open failed (%s)",
> > >                              ovs_strerror(error));
> > > @@ -891,6 +896,7 @@ get_in_port_netdev_from_key(struct dpif *dpif,
> const
> > > struct ofpbuf *key)
> > >          struct dpif_port dpif_port;
> > >          odp_port_t port_no;
> > >          int error;
> > > +        const char *type;
> > >
> > >          port_no = ODP_PORT_C(nl_attr_get_u32(in_port_nla));
> > >          error = dpif_port_query_by_number(dpif, port_no, &dpif_port);
> > > @@ -898,7 +904,8 @@ get_in_port_netdev_from_key(struct dpif *dpif,
> const
> > > struct ofpbuf *key)
> > >              goto out;
> > >          }
> > >
> > > -        netdev_open(dpif_port.name, dpif_port.type, &dev);
> > > +        type = dpif_port_open_type(dpif_type(dpif), dpif_port.type);
> > > +        netdev_open(dpif_port.name, type, &dev);
> > >          dpif_port_destroy(&dpif_port);
> > >      }
> > >
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
diff mbox

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 787851d..effa7e0 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -941,7 +941,9 @@  create_dp_netdev(const char *name, const struct
dpif_class *class,
     ovs_mutex_lock(&dp->port_mutex);
     dp_netdev_set_nonpmd(dp);

-    error = do_add_port(dp, name, "internal", ODPP_LOCAL);
+    error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class,
+                                                             "internal"),
+                        ODPP_LOCAL);
     ovs_mutex_unlock(&dp->port_mutex);
     if (error) {
         dp_netdev_free(dp);
@@ -1129,7 +1131,7 @@  hash_port_no(odp_port_t port_no)
 }

 static int
-port_create(const char *devname, const char *open_type, const char *type,
+port_create(const char *devname, const char *type,
             odp_port_t port_no, struct dp_netdev_port **portp)
 {
     struct netdev_saved_flags *sf;
@@ -1142,7 +1144,7 @@  port_create(const char *devname, const char
*open_type, const char *type,
     *portp = NULL;

     /* Open and validate network device. */
-    error = netdev_open(devname, open_type, &netdev);
+    error = netdev_open(devname, type, &netdev);
     if (error) {
         return error;
     }
@@ -1233,8 +1235,7 @@  do_add_port(struct dp_netdev *dp, const char
*devname, const char *type,
         return EEXIST;
     }

-    error = port_create(devname, dpif_netdev_port_open_type(dp->class,
type),
-                        type, port_no, &port);
+    error = port_create(devname, type, port_no, &port);
     if (error) {
         return error;
     }
root@diproiettod-dev:~/ovs# git diff
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 787851d..effa7e0 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -941,7 +941,9 @@  create_dp_netdev(const char *name, const struct
dpif_class *class,
     ovs_mutex_lock(&dp->port_mutex);
     dp_netdev_set_nonpmd(dp);

-    error = do_add_port(dp, name, "internal", ODPP_LOCAL);
+    error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class,
+                                                             "internal"),
+                        ODPP_LOCAL);
     ovs_mutex_unlock(&dp->port_mutex);
     if (error) {