Re: [ovs-dev] [PATCH v2] dpif: Return ENODEV from dpif_port_query_by_name() if there's no port.

2017-01-06 Thread Ben Pfaff 
On Fri, Jan 06, 2017 at 08:43:03PM +, Daniele Di Proietto wrote:
> 
> 
> 
> 
> 
> On 06/01/2017 11:34, "Ben Pfaff"  wrote:
> 
> >On Fri, Jan 06, 2017 at 10:59:07AM -0800, Daniele Di Proietto wrote:
> >> 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().
> >
> >Should port_query_by_name() for the userspace datapath return ENODEV,
> >instead?
> 
> Sorry to waste your time on this.  Yes, that seems the more appropriate 
> solution.
> 
> I decided to handle both ENODEV and ENOENT to be consistent with what we did 
> in
> the past, e.g bee6b8bc16b1("dpif: Don't log warning for ENOENT with
> dpif_port_exists().").
> 
> I suspected that ENOENT could only come from the userspace datapath, but I 
> wasn't
> too sure about that.
> 
> After looking at vport_cmd_get() and testing it I couldn't find any ENOENT, 
> so,
> probably, we added all those special cases just for the userspace datapath.
> 
> How about the following v3?
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327323.html

Looks good.  I'm glad we got this figured out.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpif: Return ENODEV from dpif_port_query_by_name() if there's no port.

2017-01-06 Thread Daniele Di Proietto 





On 06/01/2017 11:34, "Ben Pfaff"  wrote:

>On Fri, Jan 06, 2017 at 10:59:07AM -0800, Daniele Di Proietto wrote:
>> 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().
>
>Should port_query_by_name() for the userspace datapath return ENODEV,
>instead?

Sorry to waste your time on this.  Yes, that seems the more appropriate 
solution.

I decided to handle both ENODEV and ENOENT to be consistent with what we did in
the past, e.g bee6b8bc16b1("dpif: Don't log warning for ENOENT with
dpif_port_exists().").

I suspected that ENOENT could only come from the userspace datapath, but I 
wasn't
too sure about that.

After looking at vport_cmd_get() and testing it I couldn't find any ENOENT, so,
probably, we added all those special cases just for the userspace datapath.

How about the following v3?

https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327323.html

Thanks,

Daniele

>
>> This commit fixes the problem by translating ENOENT in ENODEV in
>> dpif_port_query_by_name().
>> 
>> 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.
>> 
>> The problem was found while developing new code.
>> 
>> Signed-off-by: Daniele Di Proietto 
>> ---
>> v2:
>> * Translate ENOENT into ENODEV in dpif_port_query_by_name(), instead of
>>   handling both in port_dump_next().
>
>Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpif: Return ENODEV from dpif_port_query_by_name() if there's no port.

2017-01-06 Thread Ben Pfaff 
On Fri, Jan 06, 2017 at 10:59:07AM -0800, Daniele Di Proietto wrote:
> 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().

Should port_query_by_name() for the userspace datapath return ENODEV,
instead?

> This commit fixes the problem by translating ENOENT in ENODEV in
> dpif_port_query_by_name().
> 
> 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.
> 
> The problem was found while developing new code.
> 
> Signed-off-by: Daniele Di Proietto 
> ---
> v2:
> * Translate ENOENT into ENODEV in dpif_port_query_by_name(), instead of
>   handling both in port_dump_next().

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2017-01-06 Thread 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().

This commit fixes the problem by translating ENOENT in ENODEV in
dpif_port_query_by_name().

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.

The problem was found while developing new code.

Signed-off-by: Daniele Di Proietto 
---
v2:
* Translate ENOENT into ENODEV in dpif_port_query_by_name(), instead of
  handling both in port_dump_next().
---
 lib/dpif.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/dpif.c b/lib/dpif.c
index 53958c559..8ef0049c8 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -653,6 +653,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 +668,18 @@ 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
+if (error == ENOENT) {
+/* Some dpif provider can return ENOENT if the port is not there,
+ * we want to translate that to ENODEV. */
+error = ENODEV;
+}
+
+/* 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(_rl,
-error == ENOENT || error == ENODEV ? VLL_DBG : VLL_WARN,
+error == ENODEV ? VLL_DBG : VLL_WARN,
 "%s: failed to query port %s: %s",
 dpif_name(dpif), devname, ovs_strerror(error));
 }
-- 
2.11.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev