Re: [ovs-dev] [PATCH] dpif-netdev: Fail port addition if reconfiguration failed.

2019-08-29 Thread Stokes, Ian 



On 8/29/2019 4:47 PM, Ilya Maximets wrote:



On 26.08.2019 19:11, Stokes, Ian wrote:


On 8/26/2019 3:12 PM, Ilya Maximets wrote:

On 23.07.2019 14:20, Ilya Maximets wrote:

If the port was destroyed during the initial reconfiguration, we should
report an error to the upper layers. Otherwise, successful addition of
the port will be logged and upper layers will continue to configure
this port. For example, the 'dpif' layer will try to initilaize flow
API for this device.

Fix that by checking for port existence after reconfiguration. We can't
get the real error code here, so let's assume EINVAL. 'ovs-vsctl' will
tell the user to check the logs for a real reason anyway.

Fixes: e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling 
code.")
Signed-off-by: Ilya Maximets 
---

Any thoughts on this?

Best regards, Ilya Maximets.



Good catch Ilya,


to my mind this seems reasonable, if reconfiguration fails then the port will 
be removed from the port list associated with the datapath in question, seems a 
fair way to assess whether re-config was successful.


Tests fine on my side.


Acked

Ian


Thanks, Ian! I applied this to master and branch-2.12.

I'm thinking about further backporting. Suggestions are welcome.


Good call, at a guess I'm thinking to 2.9 at least, but let me confirm.



I have another bug fix locally that depends on this patch, so I, probably,
will backport this patch along with that later fix.


I don't think we have dates for the next dot release of the existing 
branches but would be good to confirm, there's a few patches we're 
working on as well that can go with it.


Regards
Ian



Best regards, Ilya Maximets.








   lib/dpif-netdev.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d0a1c58ad..75d85b2fd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1862,7 +1862,8 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
     reconfigure_datapath(dp);
   -    return 0;
+    /* Check that port was successfully configured. */
+    return dp_netdev_lookup_port(dp, port_no) ? 0 : EINVAL;
   }
     static int





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

Re: [ovs-dev] [PATCH] dpif-netdev: Fail port addition if reconfiguration failed.

2019-08-29 Thread Ilya Maximets 


On 26.08.2019 19:11, Stokes, Ian wrote:
> 
> On 8/26/2019 3:12 PM, Ilya Maximets wrote:
>> On 23.07.2019 14:20, Ilya Maximets wrote:
>>> If the port was destroyed during the initial reconfiguration, we should
>>> report an error to the upper layers. Otherwise, successful addition of
>>> the port will be logged and upper layers will continue to configure
>>> this port. For example, the 'dpif' layer will try to initilaize flow
>>> API for this device.
>>>
>>> Fix that by checking for port existence after reconfiguration. We can't
>>> get the real error code here, so let's assume EINVAL. 'ovs-vsctl' will
>>> tell the user to check the logs for a real reason anyway.
>>>
>>> Fixes: e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling 
>>> code.")
>>> Signed-off-by: Ilya Maximets 
>>> ---
>> Any thoughts on this?
>>
>> Best regards, Ilya Maximets.
> 
> 
> Good catch Ilya,
> 
> 
> to my mind this seems reasonable, if reconfiguration fails then the port will 
> be removed from the port list associated with the datapath in question, seems 
> a fair way to assess whether re-config was successful.
> 
> 
> Tests fine on my side.
> 
> 
> Acked
> 
> Ian

Thanks, Ian! I applied this to master and branch-2.12.

I'm thinking about further backporting. Suggestions are welcome.

I have another bug fix locally that depends on this patch, so I, probably,
will backport this patch along with that later fix.

Best regards, Ilya Maximets.


> 
>>
>>
>>>   lib/dpif-netdev.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index d0a1c58ad..75d85b2fd 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -1862,7 +1862,8 @@ do_add_port(struct dp_netdev *dp, const char 
>>> *devname, const char *type,
>>>     reconfigure_datapath(dp);
>>>   -    return 0;
>>> +    /* Check that port was successfully configured. */
>>> +    return dp_netdev_lookup_port(dp, port_no) ? 0 : EINVAL;
>>>   }
>>>     static int
>>>
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netdev: Fail port addition if reconfiguration failed.

2019-08-26 Thread Stokes, Ian 



On 8/26/2019 3:12 PM, Ilya Maximets wrote:

On 23.07.2019 14:20, Ilya Maximets wrote:

If the port was destroyed during the initial reconfiguration, we should
report an error to the upper layers. Otherwise, successful addition of
the port will be logged and upper layers will continue to configure
this port. For example, the 'dpif' layer will try to initilaize flow
API for this device.

Fix that by checking for port existence after reconfiguration. We can't
get the real error code here, so let's assume EINVAL. 'ovs-vsctl' will
tell the user to check the logs for a real reason anyway.

Fixes: e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling 
code.")
Signed-off-by: Ilya Maximets 
---

Any thoughts on this?

Best regards, Ilya Maximets.



Good catch Ilya,


to my mind this seems reasonable, if reconfiguration fails then the port 
will be removed from the port list associated with the datapath in 
question, seems a fair way to assess whether re-config was successful.



Tests fine on my side.


Acked

Ian





  lib/dpif-netdev.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d0a1c58ad..75d85b2fd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1862,7 +1862,8 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
  
  reconfigure_datapath(dp);
  
-return 0;

+/* Check that port was successfully configured. */
+return dp_netdev_lookup_port(dp, port_no) ? 0 : EINVAL;
  }
  
  static int



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

Re: [ovs-dev] [PATCH] dpif-netdev: Fail port addition if reconfiguration failed.

2019-08-26 Thread Ilya Maximets 
On 23.07.2019 14:20, Ilya Maximets wrote:
> If the port was destroyed during the initial reconfiguration, we should
> report an error to the upper layers. Otherwise, successful addition of
> the port will be logged and upper layers will continue to configure
> this port. For example, the 'dpif' layer will try to initilaize flow
> API for this device.
> 
> Fix that by checking for port existence after reconfiguration. We can't
> get the real error code here, so let's assume EINVAL. 'ovs-vsctl' will
> tell the user to check the logs for a real reason anyway.
> 
> Fixes: e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling 
> code.")
> Signed-off-by: Ilya Maximets 
> ---

Any thoughts on this?

Best regards, Ilya Maximets.


>  lib/dpif-netdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d0a1c58ad..75d85b2fd 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1862,7 +1862,8 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
> const char *type,
>  
>  reconfigure_datapath(dp);
>  
> -return 0;
> +/* Check that port was successfully configured. */
> +return dp_netdev_lookup_port(dp, port_no) ? 0 : EINVAL;
>  }
>  
>  static int
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] dpif-netdev: Fail port addition if reconfiguration failed.

2019-07-23 Thread Ilya Maximets 
If the port was destroyed during the initial reconfiguration, we should
report an error to the upper layers. Otherwise, successful addition of
the port will be logged and upper layers will continue to configure
this port. For example, the 'dpif' layer will try to initilaize flow
API for this device.

Fix that by checking for port existence after reconfiguration. We can't
get the real error code here, so let's assume EINVAL. 'ovs-vsctl' will
tell the user to check the logs for a real reason anyway.

Fixes: e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling 
code.")
Signed-off-by: Ilya Maximets 
---
 lib/dpif-netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d0a1c58ad..75d85b2fd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1862,7 +1862,8 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
 
 reconfigure_datapath(dp);
 
-return 0;
+/* Check that port was successfully configured. */
+return dp_netdev_lookup_port(dp, port_no) ? 0 : EINVAL;
 }
 
 static int
-- 
2.17.1

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