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

[Daniele Di Proietto]

2016-07-26 11:29 GMT-07:00 Thadeu Lima de Souza Cascardo <
casca...@redhat.com>:

> On Tue, Jul 26, 2016 at 11:20:38AM -0700, Daniele Di Proietto wrote:
> > Hi Cascardo,
> >
> > thanks for your input on this.  It's quite messy right now, but I believe
> > we have a chance
> > to fix this up.
> >
> > Replies inline
> >
> > 2016-07-26 7:33 GMT-07:00 Thadeu Lima de Souza Cascardo <
> casca...@redhat.com
> > >:
> >
> > > On Mon, Jul 25, 2016 at 11:03:29AM -0700, Daniele Di Proietto wrote:
> > > > 2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo <
> > > casca...@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:
> > >
> > > Well, we also have ofproto type.
> > >
> > >
> > If I'm not mistaken, ofproto type is always equal to b) and therefore to
> c).
> >
>
> I didn't think so, I thought that a would equal the ofproto type. But it
> seems
> you are right, and we can just have two types: database type and netdev
> type,
> and make sure dpif and ofproto types match the netdev type.
>
> >
> > > I had a different approach, in which I would use the netdev_type when
> > > doing the
> > > query. That broke tests too. The affected tests were just dpctl output
> > > shown to
> > > the user. But I would expect some breakage when ofproto_query also used
> > > the same
> > > type and vswitchd would see the database type and ofproto type as
> > > different and
> > > try to reconfigure the port.
> > >
> > >
> > I don't think so. In bridge_delete_or_reconfigure() we compare the
> ofproto
> > type with
> > iface->netdev_type:
> >
> > if (strcmp(ofproto_port.type, iface->netdev_type)
> > || netdev_set_config(iface->netdev, >cfg->options,
>
> You are right. It was just some confusion because of the related problem I
> found
> (and this code is a recent fix from myself because we were using the
> database
> type).
>
> The problem was that we were comparing the database type to "system" and
> my mind
> was thinking "system" was the database type and the ofproto_port.type was
> different. I just didn't look back at the code and made a quick assumption.
>
> > NULL)) {
> > /* The interface is the wrong type or can't be configured.
> >  * Delete it. */
> > goto delete;
> > }
> >
> >
> > > Then, I looked at your patch below and noticed that you do the
> opposite,
> > > you
> > > eliminate the open_type and only use it for the internal type. Then I
> > > thought
> > > that would break other cases. But dpif_netdev_port_add uses the
> netdev_type
> > > already. Hey...
> > >
> > > So, dpctl does see it as tap when I add an internal port. Which
> probably
> > > means
> > > ofproto_type is also tap. I guess we will have to fix that too.
> > >
> >
> > To sum up, why do we have to fix that?  The translation between the
> database
> > type and the netdev type happens in vswitchd/bridge.c.  The below layers,
> > ofproto
> > and dpif-netdev, deal with the netdev type directly.
> >
> > Is there a problem with this approach?
> >
> > The few changes I suggested fix the confusion for the "ovs-netdev" port.
> >
>
> I guess that just causes the slight difference in behavior that dpctl will
> output "tap" instead of "internal" for the local port and only for the
> local
> port. For other internal ports, "tap" was already used. As you mentioned
> dpctl
> is a debugging tool and would be OK with that change, I will use your
> patch with
> your from and my sign-off. Is that OK?
>
>
I think it's fine to change the output.

You can be the author of the change.  Quite a few other lines need to be
changed
in the testsuite as well.

Here's a signoff for my part:

Signed-off-by: Daniele Di Proietto 

Thanks for getting to the bottom of this.

Daniele


> Thanks.
> Cascardo.
>
> >
> > >
> > > I am attaching my version of the patch here as well. Which of the 3
> > > versions do
> > > you think I should send? The original one I sent 

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

[Thadeu Lima de Souza Cascardo]

On Tue, Jul 26, 2016 at 11:20:38AM -0700, Daniele Di Proietto wrote:
> Hi Cascardo,
> 
> thanks for your input on this.  It's quite messy right now, but I believe
> we have a chance
> to fix this up.
> 
> Replies inline
> 
> 2016-07-26 7:33 GMT-07:00 Thadeu Lima de Souza Cascardo  >:
> 
> > On Mon, Jul 25, 2016 at 11:03:29AM -0700, Daniele Di Proietto wrote:
> > > 2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo <
> > casca...@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:
> >
> > Well, we also have ofproto type.
> >
> >
> If I'm not mistaken, ofproto type is always equal to b) and therefore to c).
> 

I didn't think so, I thought that a would equal the ofproto type. But it seems
you are right, and we can just have two types: database type and netdev type,
and make sure dpif and ofproto types match the netdev type.

> 
> > I had a different approach, in which I would use the netdev_type when
> > doing the
> > query. That broke tests too. The affected tests were just dpctl output
> > shown to
> > the user. But I would expect some breakage when ofproto_query also used
> > the same
> > type and vswitchd would see the database type and ofproto type as
> > different and
> > try to reconfigure the port.
> >
> >
> I don't think so. In bridge_delete_or_reconfigure() we compare the ofproto
> type with
> iface->netdev_type:
> 
> if (strcmp(ofproto_port.type, iface->netdev_type)
> || netdev_set_config(iface->netdev, >cfg->options,

You are right. It was just some confusion because of the related problem I found
(and this code is a recent fix from myself because we were using the database
type).

The problem was that we were comparing the database type to "system" and my mind
was thinking "system" was the database type and the ofproto_port.type was
different. I just didn't look back at the code and made a quick assumption.

> NULL)) {
> /* The interface is the wrong type or can't be configured.
>  * Delete it. */
> goto delete;
> }
> 
> 
> > Then, I looked at your patch below and noticed that you do the opposite,
> > you
> > eliminate the open_type and only use it for the internal type. Then I
> > thought
> > that would break other cases. But dpif_netdev_port_add uses the netdev_type
> > already. Hey...
> >
> > So, dpctl does see it as tap when I add an internal port. Which probably
> > means
> > ofproto_type is also tap. I guess we will have to fix that too.
> >
> 
> To sum up, why do we have to fix that?  The translation between the database
> type and the netdev type happens in vswitchd/bridge.c.  The below layers,
> ofproto
> and dpif-netdev, deal with the netdev type directly.
> 
> Is there a problem with this approach?
> 
> The few changes I suggested fix the confusion for the "ovs-netdev" port.
> 

I guess that just causes the slight difference in behavior that dpctl will
output "tap" instead of "internal" for the local port and only for the local
port. For other internal ports, "tap" was already used. As you mentioned dpctl
is a debugging tool and would be OK with that change, I will use your patch with
your from and my sign-off. Is that OK?

Thanks.
Cascardo.

> 
> >
> > I am attaching my version of the patch here as well. Which of the 3
> > versions do
> > you think I should send? The original one I sent will not require changes
> > in the
> > tests and also doesn't change behavior for the user output, so I would
> > stick to
> > it for now.
> >
> 
> Why is the code below necessary?  ofproto already deals with the netdev
> type only, right?
> (maybe I'm missing something here, probably)
> 
> 
> >
> > Cascardo.
> >
> > ---8<---
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 37c2631..19b1f87 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1437,11 +1437,12 @@ do_del_port(struct dp_netdev *dp, struct
> > dp_netdev_port *port)
> >  }
> >
> >  static void
> > -answer_port_query(const struct dp_netdev_port *port,
> 

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

[Daniele Di Proietto]

Hi Cascardo,

thanks for your input on this.  It's quite messy right now, but I believe
we have a chance
to fix this up.

Replies inline

2016-07-26 7:33 GMT-07:00 Thadeu Lima de Souza Cascardo :

> On Mon, Jul 25, 2016 at 11:03:29AM -0700, Daniele Di Proietto wrote:
> > 2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo <
> casca...@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:
>
> Well, we also have ofproto type.
>
>
If I'm not mistaken, ofproto type is always equal to b) and therefore to c).


> I had a different approach, in which I would use the netdev_type when
> doing the
> query. That broke tests too. The affected tests were just dpctl output
> shown to
> the user. But I would expect some breakage when ofproto_query also used
> the same
> type and vswitchd would see the database type and ofproto type as
> different and
> try to reconfigure the port.
>
>
I don't think so. In bridge_delete_or_reconfigure() we compare the ofproto
type with
iface->netdev_type:

if (strcmp(ofproto_port.type, iface->netdev_type)
|| netdev_set_config(iface->netdev, >cfg->options,
NULL)) {
/* The interface is the wrong type or can't be configured.
 * Delete it. */
goto delete;
}


> Then, I looked at your patch below and noticed that you do the opposite,
> you
> eliminate the open_type and only use it for the internal type. Then I
> thought
> that would break other cases. But dpif_netdev_port_add uses the netdev_type
> already. Hey...
>
> So, dpctl does see it as tap when I add an internal port. Which probably
> means
> ofproto_type is also tap. I guess we will have to fix that too.
>

To sum up, why do we have to fix that?  The translation between the database
type and the netdev type happens in vswitchd/bridge.c.  The below layers,
ofproto
and dpif-netdev, deal with the netdev type directly.

Is there a problem with this approach?

The few changes I suggested fix the confusion for the "ovs-netdev" port.


>
> I am attaching my version of the patch here as well. Which of the 3
> versions do
> you think I should send? The original one I sent will not require changes
> in the
> tests and also doesn't change behavior for the user output, so I would
> stick to
> it for now.
>

Why is the code below necessary?  ofproto already deals with the netdev
type only, right?
(maybe I'm missing something here, probably)


>
> Cascardo.
>
> ---8<---
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 37c2631..19b1f87 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1437,11 +1437,12 @@ do_del_port(struct dp_netdev *dp, struct
> dp_netdev_port *port)
>  }
>
>  static void
> -answer_port_query(const struct dp_netdev_port *port,
> +answer_port_query(struct dp_netdev *dp OVS_UNUSED, const struct
> dp_netdev_port *port,
>struct dpif_port *dpif_port)
>  {
> +const char *type = dpif_netdev_port_open_type(dp->class, port->type);
>  dpif_port->name = xstrdup(netdev_get_name(port->netdev));
> -dpif_port->type = xstrdup(port->type);
> +dpif_port->type = xstrdup(type);
>  dpif_port->port_no = port->port_no;
>  }
>
> @@ -1456,7 +1457,7 @@ dpif_netdev_port_query_by_number(const struct dpif
> *dpif, odp_port_t port_no,
>  ovs_mutex_lock(>port_mutex);
>  error = get_port_by_number(dp, port_no, );
>  if (!error && dpif_port) {
> -answer_port_query(port, dpif_port);
> +answer_port_query(dp, port, dpif_port);
>  }
>  ovs_mutex_unlock(>port_mutex);
>
> @@ -1474,7 +1475,7 @@ dpif_netdev_port_query_by_name(const struct dpif
> *dpif, const char *devname,
>  ovs_mutex_lock(>port_mutex);
>  error = get_port_by_name(dp, devname, );
>  if (!error && dpif_port) {
> -answer_port_query(port, dpif_port);
> +answer_port_query(dp, port, dpif_port);
>  }
>  ovs_mutex_unlock(>port_mutex);
>
> ---8<---
>
> >
> > ---8<---
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 787851d..effa7e0 100644
> > --- a/lib/dpif-netdev.c
> > +++ 

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

[Thadeu Lima de Souza Cascardo]

On Mon, Jul 25, 2016 at 11:03:29AM -0700, Daniele Di Proietto wrote:
> 2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo  >:
> 
> > 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:

Well, we also have ofproto type.

I had a different approach, in which I would use the netdev_type when doing the
query. That broke tests too. The affected tests were just dpctl output shown to
the user. But I would expect some breakage when ofproto_query also used the same
type and vswitchd would see the database type and ofproto type as different and
try to reconfigure the port.

Then, I looked at your patch below and noticed that you do the opposite, you
eliminate the open_type and only use it for the internal type. Then I thought
that would break other cases. But dpif_netdev_port_add uses the netdev_type
already. Hey...

So, dpctl does see it as tap when I add an internal port. Which probably means
ofproto_type is also tap. I guess we will have to fix that too.

I am attaching my version of the patch here as well. Which of the 3 versions do
you think I should send? The original one I sent will not require changes in the
tests and also doesn't change behavior for the user output, so I would stick to
it for now.

Cascardo.

---8<---
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 37c2631..19b1f87 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1437,11 +1437,12 @@ do_del_port(struct dp_netdev *dp, struct dp_netdev_port 
*port)
 }
 
 static void
-answer_port_query(const struct dp_netdev_port *port,
+answer_port_query(struct dp_netdev *dp OVS_UNUSED, const struct dp_netdev_port 
*port,
   struct dpif_port *dpif_port)
 {
+const char *type = dpif_netdev_port_open_type(dp->class, port->type);
 dpif_port->name = xstrdup(netdev_get_name(port->netdev));
-dpif_port->type = xstrdup(port->type);
+dpif_port->type = xstrdup(type);
 dpif_port->port_no = port->port_no;
 }
 
@@ -1456,7 +1457,7 @@ dpif_netdev_port_query_by_number(const struct dpif *dpif, 
odp_port_t port_no,
 ovs_mutex_lock(>port_mutex);
 error = get_port_by_number(dp, port_no, );
 if (!error && dpif_port) {
-answer_port_query(port, dpif_port);
+answer_port_query(dp, port, dpif_port);
 }
 ovs_mutex_unlock(>port_mutex);
 
@@ -1474,7 +1475,7 @@ dpif_netdev_port_query_by_name(const struct dpif *dpif, 
const char *devname,
 ovs_mutex_lock(>port_mutex);
 error = get_port_by_name(dp, devname, );
 if (!error && dpif_port) {
-answer_port_query(port, dpif_port);
+answer_port_query(dp, port, dpif_port);
 }
 ovs_mutex_unlock(>port_mutex);
 
---8<---

> 
> ---8<---
> 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(>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(>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, );
> +error = netdev_open(devname, type, );
>  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, 

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

[Daniele Di Proietto]

2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo :

> 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<---
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(>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(>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, );
+error = netdev_open(devname, type, );
 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, );
+error = port_create(devname, type, port_no, );
 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(>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(>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, );
+error = netdev_open(devname, type, );
 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, );
+error = port_create(devname, type, port_no, );
 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 

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

[Thadeu Lima de Souza Cascardo]

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 <
> casca...@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, );
> > +error = netdev_open(name, dpif_port_open_type(dpif_type(dpif),
> > type),
> > +);
> >  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(_port);
> >
> >  /* Retrieve its existing configuration. */
> > -error = netdev_open(name, type, );
> > +error = netdev_open(name, dpif_port_open_type(dpif_type(dpif),
> > type),
> > +);
> >  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], _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, );
> > +error = netdev_open(dpif_port.name, type, );
> >  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, );
> > +error = netdev_open(dpif_port.name, type, );
> >  

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

[Daniele Di Proietto]

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

2016-07-18 10:00 GMT-07:00 Thadeu Lima de Souza Cascardo <
casca...@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, );
> +error = netdev_open(name, dpif_port_open_type(dpif_type(dpif),
> type),
> +);
>  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(_port);
>
>  /* Retrieve its existing configuration. */
> -error = netdev_open(name, type, );
> +error = netdev_open(name, dpif_port_open_type(dpif_type(dpif),
> type),
> +);
>  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], _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, );
> +error = netdev_open(dpif_port.name, type, );
>  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, );
> +error = netdev_open(dpif_port.name, type, );
>  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, _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, );
> +type = dpif_port_open_type(dpif_type(dpif), dpif_port.type);
> +netdev_open(dpif_port.name, type, );
>  dpif_port_destroy(_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

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

[Thadeu Lima de Souza Cascardo]

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, );
+error = netdev_open(name, dpif_port_open_type(dpif_type(dpif), type),
+);
 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(_port);
 
 /* Retrieve its existing configuration. */
-error = netdev_open(name, type, );
+error = netdev_open(name, dpif_port_open_type(dpif_type(dpif), type),
+);
 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], _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, );
+error = netdev_open(dpif_port.name, type, );
 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, );
+error = netdev_open(dpif_port.name, type, );
 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, _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, );
+type = dpif_port_open_type(dpif_type(dpif), dpif_port.type);
+netdev_open(dpif_port.name, type, );
 dpif_port_destroy(_port);
 }
 
-- 
2.7.4

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