[ovs-dev] [RFC 4/5] dpctl: uses open_type when calling netdev_open
Thadeu Lima de Souza Cascardo
cascardo at redhat.com
Mon Jul 25 16:57:32 UTC 2016
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, 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 at 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 at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list