Re: [ovs-dev] [PATCH] datapath: meter: Fix setting meter id for new entries

[Gregory Rose]

On 8/7/2018 8:31 PM, Justin Pettit wrote:

Thanks, Greg. I actually have this queued up with another patch that will 
disable meters entirely on broken kernels. I plan to send that out tomorrow.

--Justin


Oops - perhaps I was a bit hasty.  I was just trying to get caught up 
with upstream.


Apologies if I interrupted your work flow.

- Greg





On Aug 7, 2018, at 4:45 PM, Greg Rose  wrote:

From: Justin Pettit 

Upstream commit:
From: Justin Pettit 
Date: Sat, 28 Jul 2018 15:26:01 -0700
Subject: [PATCH] openvswitch: meter: Fix setting meter id for new entries

The meter code would create an entry for each new meter.  However, it
would not set the meter id in the new entry, so every meter would appear
to have a meter id of zero.  This commit properly sets the meter id when
adding the entry.

Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
Signed-off-by: Justin Pettit 
Cc: Andy Zhou 
Signed-off-by: David S. Miller 

Cc: Justin Pettit 
Signed-off-by: Greg Rose 
---
datapath/meter.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/datapath/meter.c b/datapath/meter.c
index 1c2ed46..281d869 100644
--- a/datapath/meter.c
+++ b/datapath/meter.c
@@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr **a)
if (!meter)
return ERR_PTR(-ENOMEM);

+meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
@@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
u32 meter_id;
bool failed;

+if (!a[OVS_METER_ATTR_ID]) {
+return -ENODEV;
+}
+
meter = dp_meter_create(a);
if (IS_ERR_OR_NULL(meter))
return PTR_ERR(meter);
@@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
goto exit_unlock;
}

-if (!a[OVS_METER_ATTR_ID]) {
-err = -ENODEV;
-goto exit_unlock;
-}
-
meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);

/* Cannot fail after this. */
--
1.8.3.1



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

Re: [ovs-dev] [patch v5] dpctl: Make opt_dpif_open() more general.

[Darrell Ball]

On Tue, Aug 7, 2018 at 4:20 PM, Ben Pfaff  wrote:

> On Mon, Aug 06, 2018 at 07:24:39PM -0700, Darrell Ball wrote:
> > By making opt_dpif_open() more general, it can be used effectively
> > by all potential callers and avoids trying to open potentially bogus
> > datapaths provided by the user. Also, the error handling is improved by
> > having more specific errors.
> >
> > Signed-off-by: Darrell Ball 
> > ---
> >  lib/dpctl.c | 61 ++
> +++
> >  tests/system-traffic.at |  8 +++
> >  2 files changed, 60 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index c600eeb..b8a8dbf 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -187,18 +187,69 @@ parsed_dpif_open(const char *arg_, bool create,
> struct dpif **dpifp)
> >  return result;
> >  }
> >
> > +static bool
> > +dp_exists(const char *queried_dp)
> > +{
> > +struct sset dpif_names = SSET_INITIALIZER(_names),
> > +dpif_types = SSET_INITIALIZER(_types);
> > +bool found = false;
> > +char *queried_name, *queried_type;
> > +
> > +dp_parse_name(queried_dp, _name, _type);
> > +dp_enumerate_types(_types);
> > +
> > +const char *type;
> > +SSET_FOR_EACH (type, _types) {
> > +int error = dp_enumerate_names(type, _names);
> > +if (!error) {
> > +const char *name;
> > +SSET_FOR_EACH (name, _names) {
> > +if (!strcmp(type, queried_type) &&
> > +!strcmp(name, queried_name)) {
> > +found = true;
> > +goto out;
> > +}
> > +}
> > +}
> > +}
> > +
> > +out:
> > +sset_destroy(_names);
> > +sset_destroy(_types);
> > +return found;
> > +}
>
> Thanks for working to make ovs-dpctl error messages better!
>
> I don't see why one would bother to enumerate the types here.  There is
> only one type that could be of interest.  I think you said that you
> don't want to just try to blindly open (type,name), but even if so, why
> not just try to enumerate the names within 'type'?
>


Because the type (i.e. queried_type) passed is often/usually bogus and
therefore
enumerating names with this bogus 'type' only serves to generate the
appropriate warning logs,
yielding a false positive.



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

Re: [ovs-dev] [PATCH] datapath: meter: Fix setting meter id for new entries

[Justin Pettit]

Thanks, Greg. I actually have this queued up with another patch that will 
disable meters entirely on broken kernels. I plan to send that out tomorrow. 

--Justin


> On Aug 7, 2018, at 4:45 PM, Greg Rose  wrote:
> 
> From: Justin Pettit 
> 
> Upstream commit:
>From: Justin Pettit 
>Date: Sat, 28 Jul 2018 15:26:01 -0700
>Subject: [PATCH] openvswitch: meter: Fix setting meter id for new entries
> 
>The meter code would create an entry for each new meter.  However, it
>would not set the meter id in the new entry, so every meter would appear
>to have a meter id of zero.  This commit properly sets the meter id when
>adding the entry.
> 
>Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
>Signed-off-by: Justin Pettit 
>Cc: Andy Zhou 
>Signed-off-by: David S. Miller 
> 
> Cc: Justin Pettit 
> Signed-off-by: Greg Rose 
> ---
> datapath/meter.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/datapath/meter.c b/datapath/meter.c
> index 1c2ed46..281d869 100644
> --- a/datapath/meter.c
> +++ b/datapath/meter.c
> @@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr **a)
>if (!meter)
>return ERR_PTR(-ENOMEM);
> 
> +meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
>meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
>meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
>meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
> @@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
> genl_info *info)
>u32 meter_id;
>bool failed;
> 
> +if (!a[OVS_METER_ATTR_ID]) {
> +return -ENODEV;
> +}
> +
>meter = dp_meter_create(a);
>if (IS_ERR_OR_NULL(meter))
>return PTR_ERR(meter);
> @@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
> genl_info *info)
>goto exit_unlock;
>}
> 
> -if (!a[OVS_METER_ATTR_ID]) {
> -err = -ENODEV;
> -goto exit_unlock;
> -}
> -
>meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
> 
>/* Cannot fail after this. */
> -- 
> 1.8.3.1
> 

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

[ovs-dev] [PATCH v2 2/2] table: fix html buffer output

[Aaron Conole]

Prior to this commit, html output exhibiits a doppler effect for
content by continually printing strings passed from
table_print_html_cell.

Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a table 
as a string.")
Cc: Ben Pfaff 
Cc: Jakub Sitnicki 
Signed-off-by: Aaron Conole 
---
 lib/table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/table.c b/lib/table.c
index 19bf89262..ab72668c7 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -349,7 +349,7 @@ static void
 table_escape_html_text__(const char *content, size_t n, struct ds *s)
 {
 if (!strpbrk(content, "&<>\"")) {
-ds_put_cstr(s, content);
+ds_put_buffer(s, content, n);
 } else {
 size_t i;
 
-- 
2.14.3

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

[ovs-dev] [PATCH v2 1/2] table: append newline when printing tables

[Aaron Conole]

With commit cb139fa8b3a1 ("table: New function table_format() for
formatting a table as a string.") a new mechanism for formatting
tables was introduced, and the table_print method was refactored to
use this.

During that refactor, calls to 'puts' were replaced with
'ds_put_cstr', and table print was changed to use 'fputs(...,
stdout)'.  Unfortunately, fputs() does not append a newline to the
string provided, and changes the output strings of, for example,
ovsdb-client dump to print all on one line.  This means
post-processing scripts that are chained after ovsdb-client would
either block indefinitely (if they don't detect EOF), or process the
entire bundle at once (rather than seeing each table on a separate
line).

Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a table 
as a string.")
Cc: Ben Pfaff 
Cc: Jakub Sitnicki 
Reported-by: Terry Wilson 
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1608508
Signed-off-by: Aaron Conole 
Suggested-by: Ben Pfaff 
---
 lib/table.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/table.c b/lib/table.c
index cd811caf5..19bf89262 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -547,6 +547,7 @@ table_print_json__(const struct table *table, const struct 
table_style *style,
 json_object_put(json, "data", data);
 
 json_to_ds(json, style->json_flags, s);
+ds_put_char(s, '\n');
 json_destroy(json);
 }
 
-- 
2.14.3

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

Re: [ovs-dev] [PATCH] table: append newline when printing tables

[Aaron Conole]

Ben Pfaff  writes:

> On Tue, Aug 07, 2018 at 06:23:12PM -0400, Aaron Conole wrote:
>> With commit cb139fa8b3a1 ("table: New function table_format() for
>> formatting a table as a string.") a new mechanism for formatting
>> tables was introduced, and the table_print method was refactored to
>> use this.
>> 
>> During that refactor, calls to 'puts' were replaced with
>> 'ds_put_cstr', and table print was changed to use 'fputs(...,
>> stdout)'.  Unfortunately, fputs() does not append a newline to the
>> string provided, and changes the output strings of, for example,
>> ovsdb-client dump to print all on one line.  This means
>> post-processing scripts that are chained after ovsdb-client would
>> either block indefinitely (if they don't detect EOF), or process the
>> entire bundle at once (rather than seeing each table on a separate
>> line).
>> 
>> Fixes: cb139fa8b3a1 ("table: New function table_format() for
>> formatting a table as a string.")
>> Cc: Ben Pfaff 
>> Cc: Jakub Sitnicki 
>> Reported-by: Terry Wilson 
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1608508
>> Signed-off-by: Aaron Conole 
>> ---
>> NOTE: I chose to keep the fputs and insert an additional fputs.
>>   It might alternately be appropriate to change fputs() to
>>   puts().  Dealer's choice, I guess :)
>
> It looks to me like this is a problem specifically in the formatting for
> JSON tables.  If so, then the fix is more like this:
>
> diff --git a/lib/table.c b/lib/table.c
> index cd811caf5b88..19bf89262cfc 100644
> --- a/lib/table.c
> +++ b/lib/table.c
> @@ -547,6 +547,7 @@ table_print_json__(const struct table *table, const 
> struct table_style *style,
>  json_object_put(json, "data", data);
>  
>  json_to_ds(json, style->json_flags, s);
> +ds_put_char(s, '\n');
>  json_destroy(json);
>  }
>  
>
> Can you check?

Whoops!  Looks like you're right - I rushed to the wrong routine.

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

Re: [ovs-dev] [PATCH] datapath: meter: Fix setting meter id for new entries

[Gregory Rose]

On 8/7/2018 4:55 PM, Ben Pfaff wrote:

On Tue, Aug 07, 2018 at 04:45:26PM -0700, Greg Rose wrote:

From: Justin Pettit 

Upstream commit:
 From: Justin Pettit 
 Date: Sat, 28 Jul 2018 15:26:01 -0700
 Subject: [PATCH] openvswitch: meter: Fix setting meter id for new entries

 The meter code would create an entry for each new meter.  However, it
 would not set the meter id in the new entry, so every meter would appear
 to have a meter id of zero.  This commit properly sets the meter id when
 adding the entry.

 Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
 Signed-off-by: Justin Pettit 
 Cc: Andy Zhou 
 Signed-off-by: David S. Miller 

Cc: Justin Pettit 
Signed-off-by: Greg Rose 

Thanks, applied to master and branch-2.10.  If it needs backporting
further (maybe it does?), it didn't apply without conflicts, so maybe
posting a backported version would be a good idea.

Thanks,

Ben.


Justin may correct me but as I recall metering was added after 2.9. So I 
think master and 2.10 is fine.


Thanks Ben!

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

Re: [ovs-dev] [PATCH] datapath: meter: Fix setting meter id for new entries

[Ben Pfaff]

On Tue, Aug 07, 2018 at 04:45:26PM -0700, Greg Rose wrote:
> From: Justin Pettit 
> 
> Upstream commit:
> From: Justin Pettit 
> Date: Sat, 28 Jul 2018 15:26:01 -0700
> Subject: [PATCH] openvswitch: meter: Fix setting meter id for new entries
> 
> The meter code would create an entry for each new meter.  However, it
> would not set the meter id in the new entry, so every meter would appear
> to have a meter id of zero.  This commit properly sets the meter id when
> adding the entry.
> 
> Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
> Signed-off-by: Justin Pettit 
> Cc: Andy Zhou 
> Signed-off-by: David S. Miller 
> 
> Cc: Justin Pettit 
> Signed-off-by: Greg Rose 

Thanks, applied to master and branch-2.10.  If it needs backporting
further (maybe it does?), it didn't apply without conflicts, so maybe
posting a backported version would be a good idea.

Thanks,

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

[ovs-dev] [PATCH] datapath: meter: Fix setting meter id for new entries

[Greg Rose]

From: Justin Pettit 

Upstream commit:
From: Justin Pettit 
Date: Sat, 28 Jul 2018 15:26:01 -0700
Subject: [PATCH] openvswitch: meter: Fix setting meter id for new entries

The meter code would create an entry for each new meter.  However, it
would not set the meter id in the new entry, so every meter would appear
to have a meter id of zero.  This commit properly sets the meter id when
adding the entry.

Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
Signed-off-by: Justin Pettit 
Cc: Andy Zhou 
Signed-off-by: David S. Miller 

Cc: Justin Pettit 
Signed-off-by: Greg Rose 
---
 datapath/meter.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/datapath/meter.c b/datapath/meter.c
index 1c2ed46..281d869 100644
--- a/datapath/meter.c
+++ b/datapath/meter.c
@@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr **a)
if (!meter)
return ERR_PTR(-ENOMEM);
 
+   meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
@@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
u32 meter_id;
bool failed;
 
+   if (!a[OVS_METER_ATTR_ID]) {
+   return -ENODEV;
+   }
+
meter = dp_meter_create(a);
if (IS_ERR_OR_NULL(meter))
return PTR_ERR(meter);
@@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
goto exit_unlock;
}
 
-   if (!a[OVS_METER_ATTR_ID]) {
-   err = -ENODEV;
-   goto exit_unlock;
-   }
-
meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
 
/* Cannot fail after this. */
-- 
1.8.3.1

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

Re: [ovs-dev] [patch v5] dpctl: Make opt_dpif_open() more general.

[Ben Pfaff]

On Mon, Aug 06, 2018 at 07:24:39PM -0700, Darrell Ball wrote:
> By making opt_dpif_open() more general, it can be used effectively
> by all potential callers and avoids trying to open potentially bogus
> datapaths provided by the user. Also, the error handling is improved by
> having more specific errors.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/dpctl.c | 61 
> +
>  tests/system-traffic.at |  8 +++
>  2 files changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index c600eeb..b8a8dbf 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -187,18 +187,69 @@ parsed_dpif_open(const char *arg_, bool create, struct 
> dpif **dpifp)
>  return result;
>  }
>  
> +static bool
> +dp_exists(const char *queried_dp)
> +{
> +struct sset dpif_names = SSET_INITIALIZER(_names),
> +dpif_types = SSET_INITIALIZER(_types);
> +bool found = false;
> +char *queried_name, *queried_type;
> +
> +dp_parse_name(queried_dp, _name, _type);
> +dp_enumerate_types(_types);
> +
> +const char *type;
> +SSET_FOR_EACH (type, _types) {
> +int error = dp_enumerate_names(type, _names);
> +if (!error) {
> +const char *name;
> +SSET_FOR_EACH (name, _names) {
> +if (!strcmp(type, queried_type) &&
> +!strcmp(name, queried_name)) {
> +found = true;
> +goto out;
> +}
> +}
> +}
> +}
> +
> +out:
> +sset_destroy(_names);
> +sset_destroy(_types);
> +return found;
> +}

Thanks for working to make ovs-dpctl error messages better!

I don't see why one would bother to enumerate the types here.  There is
only one type that could be of interest.  I think you said that you
don't want to just try to blindly open (type,name), but even if so, why
not just try to enumerate the names within 'type'?

Thanks,

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

Re: [ovs-dev] [PATCH v1] ovs-testcontroller: Added section for runtime managment commands

[Ben Pfaff]

On Mon, Aug 06, 2018 at 05:21:37PM -0700, Ashish Varma wrote:
> Even though there are no runtime management commands supported by
> ovs-testcontroller, the '--unixctl' section of the man page refers to
> 'RUNTIME MANAGEMENT COMMANDS'. This message which refers to the runtime
> management commands section is common for all '--unixctl' option.
> (via 'lib/unixctl.man')
> 
> Added an empty section in the man page for 'RUNTIME MANAGEMENT COMMANDS' to
> avoid confusion to the reader of the man page.
> 
> Signed-off-by: Ashish Varma 

Thanks for improving the OVS documentation!

However, unixctl is useless if there are no commands.  It's probably
better to remove the support for it.

Thanks,

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

Re: [ovs-dev] [PATCH] ovn-controller: Use ovsdb index for mac-binding update.

[Ben Pfaff]

On Wed, Jul 11, 2018 at 11:05:44AM -0700, Han Zhou wrote:
> Signed-off-by: Han Zhou 

Applied to master and branch-2.10, thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] table: append newline when printing tables

[Ben Pfaff]

On Tue, Aug 07, 2018 at 06:23:12PM -0400, Aaron Conole wrote:
> With commit cb139fa8b3a1 ("table: New function table_format() for
> formatting a table as a string.") a new mechanism for formatting
> tables was introduced, and the table_print method was refactored to
> use this.
> 
> During that refactor, calls to 'puts' were replaced with
> 'ds_put_cstr', and table print was changed to use 'fputs(...,
> stdout)'.  Unfortunately, fputs() does not append a newline to the
> string provided, and changes the output strings of, for example,
> ovsdb-client dump to print all on one line.  This means
> post-processing scripts that are chained after ovsdb-client would
> either block indefinitely (if they don't detect EOF), or process the
> entire bundle at once (rather than seeing each table on a separate
> line).
> 
> Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a 
> table as a string.")
> Cc: Ben Pfaff 
> Cc: Jakub Sitnicki 
> Reported-by: Terry Wilson 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1608508
> Signed-off-by: Aaron Conole 
> ---
> NOTE: I chose to keep the fputs and insert an additional fputs.
>   It might alternately be appropriate to change fputs() to
>   puts().  Dealer's choice, I guess :)

It looks to me like this is a problem specifically in the formatting for
JSON tables.  If so, then the fix is more like this:

diff --git a/lib/table.c b/lib/table.c
index cd811caf5b88..19bf89262cfc 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -547,6 +547,7 @@ table_print_json__(const struct table *table, const struct 
table_style *style,
 json_object_put(json, "data", data);
 
 json_to_ds(json, style->json_flags, s);
+ds_put_char(s, '\n');
 json_destroy(json);
 }
 

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

Re: [ovs-dev] [ovs-dev, 2 of 2] ip_gre: remove redundant variables t_hlen

[0-day Robot]

Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Too many signoffs; are you missing Co-authored-by lines?
Lines checked: 55, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev, 1 of 2] ip_gre: fix IFLA_MTU ignored on NEWLINK

[0-day Robot]

Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Too many signoffs; are you missing Co-authored-by lines?
Lines checked: 55, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] table: append newline when printing tables

[Aaron Conole]

With commit cb139fa8b3a1 ("table: New function table_format() for
formatting a table as a string.") a new mechanism for formatting
tables was introduced, and the table_print method was refactored to
use this.

During that refactor, calls to 'puts' were replaced with
'ds_put_cstr', and table print was changed to use 'fputs(...,
stdout)'.  Unfortunately, fputs() does not append a newline to the
string provided, and changes the output strings of, for example,
ovsdb-client dump to print all on one line.  This means
post-processing scripts that are chained after ovsdb-client would
either block indefinitely (if they don't detect EOF), or process the
entire bundle at once (rather than seeing each table on a separate
line).

Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a table 
as a string.")
Cc: Ben Pfaff 
Cc: Jakub Sitnicki 
Reported-by: Terry Wilson 
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1608508
Signed-off-by: Aaron Conole 
---
NOTE: I chose to keep the fputs and insert an additional fputs.
  It might alternately be appropriate to change fputs() to
  puts().  Dealer's choice, I guess :)

 lib/table.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/table.c b/lib/table.c
index cd811caf5..a572c3446 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -620,6 +620,7 @@ table_print(const struct table *table, const struct 
table_style *style)
 struct ds s = DS_EMPTY_INITIALIZER;
 table_format(table, style, );
 fputs(ds_cstr(), stdout);
+fputs("\n", stdout);
 ds_destroy();
 }
 
-- 
2.14.3

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

Re: [ovs-dev] [PATCH 0/3] Port group related enhancements.

[Ben Pfaff]

On Mon, Aug 06, 2018 at 07:43:59PM -0700, Han Zhou wrote:
> These patches are related to port groups, but the patch 3/3 is
> independent from the others.
> 
> Han Zhou (3):
>   ovn-northd: Simplify struct ovn_port_group.
>   ovn-northd: Improve efficiency of stateful checking for ACLs on port
> groups.
>   ovn-trace: Fix warnings when port is found but not in current
> datapath.

Thanks, Han!  I applied these to master and branch-2.10.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] infiniband (IPoIB) support

[Ben Pfaff]

I don't know how many people on this list know anything about IPoIB.  I
know that I don't.  You might not be getting an answer simply because
it's such a specialty topic.  Maybe there is a place where people talk
about IPoIB software; maybe they would know something.

On Wed, Aug 08, 2018 at 12:59:36AM +0300, Vasiliy Tolstov wrote:
> Does anybody can helps me and say, how to do connectivity from ovn
> network to physical? if ovn network and phisical have the same subnet.
> пн, 6 авг. 2018 г. в 23:35, Vasiliy Tolstov :
> >
> > пн, 6 авг. 2018 г. в 17:34, Vasiliy Tolstov :
> > >
> > > And if IPoIB device cant't be added to openvswitch bridge, how can i
> > > connect virtual network with physical in such setup:
> > >
> > > i have logical switch extnet with vm ports, each vm via ovn dhcp have
> > > ip address from external network.
> > > I want to route all traffic from this extnet via last ip address from
> > > /24 subnet (254).
> > > Does i need to create logical router for such thing?
> > > Can you explain me how can i solve this?
> > > пн, 6 авг. 2018 г. в 17:11, Vasiliy Tolstov :
> > > >
> > > > Hi. I know about dpdk, but i have mellanox connectx-2 card with IPoIB 
> > > > in linux.
> > > > And i want to add it to openvswitch bridge.
> > > > I found topics (7 years ago) that says about no plans to add support
> > > > for IPoIB devices.
> > > > How can i add to ovs bridge IPoIB device?
> > > >
> > > > --
> > > > Vasiliy Tolstov,
> > > > e-mail: v.tols...@selfip.ru
> > >
> > >
> > >
> > > --
> > > Vasiliy Tolstov,
> > > e-mail: v.tols...@selfip.ru
> >
> > I'm partially solve problem by adding to extnet gw port with type localnet.
> > so connection from vm to external world works fine, but from external
> > world to vm not:
> > ovs-vsctl show
> > f39cbc63-2dd2-486e-a2c8-1b7faee48535
> > Bridge br-ext
> > Port patch-gw-to-br-int
> > Interface patch-gw-to-br-int
> > type: patch
> > options: {peer=patch-br-int-to-gw}
> > Port br-ext
> > Interface br-ext
> > type: internal
> > Bridge br-int
> > fail_mode: secure
> > Port patch-br-int-to-gw
> > Interface patch-br-int-to-gw
> > type: patch
> > options: {peer=patch-gw-to-br-int}
> > Port "vnet0"
> > Interface "vnet0"
> > Port br-int
> > Interface br-int
> > type: internal
> > ovs_version: "2.8.1"
> >
> > port gw
> > type: localnet
> > addresses: ["unknown"]
> > port d2b171b6-c501-4e8c-b29d-adc79e573d4c
> > addresses: ["52:54:00:08:43:25 xx.xx.xx.xx"]
> > --
> > Vasiliy Tolstov,
> > e-mail: v.tols...@selfip.ru
> 
> 
> 
> -- 
> Vasiliy Tolstov,
> e-mail: v.tols...@selfip.ru
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] ip_gre: remove redundant variables t_hlen

[Gregory Rose]

On 8/7/2018 3:05 PM, Ben Pfaff wrote:

On Tue, Aug 07, 2018 at 02:48:53PM -0700, Greg Rose wrote:

From: YueHaibing 

Upstream commit:
 From: YueHaibing 
 Date: Wed, 1 Aug 2018 10:04:02 +0800
 Subject: [PATCH] ip_gre: remove redundant variables t_hlen

 After commit ffc2b6ee4174 ("ip_gre: fix IFLA_MTU ignored on NEWLINK")
 variable t_hlen is assigned values that are never read,
 hence they are redundant and can be removed.

 Signed-off-by: YueHaibing 
 Signed-off-by: David S. Miller 

Cc: YueHaibing 
Signed-off-by: Greg Rose 

These looked like straightforward backports so I applied them to master.

It's not clear to me whether they fixed any bugs so I didn't backport
them, please let me know if I should.


This is another one that touches erspan code so I don't think it will go 
much further back than 2.10.


Thanks!

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

Re: [ovs-dev] [PATCH v3] datapath: support upstream ndo_udp_tunnel_add in net_device_ops

[Gregory Rose]

On 8/7/2018 3:08 PM, Ben Pfaff wrote:

On Tue, Aug 07, 2018 at 01:53:15PM -0700, Gregory Rose wrote:

On 8/4/2018 1:31 AM, we...@ucloud.cn wrote:

From: wenxu 

It makes datapath can support both ndo_add_udp_tunnel_port and
ndo_add_vxlan/geneve_port. The newer kernels don't support vxlan/geneve
specific NDO's anymore

Signed-off-by: wenxu 

LGTM

Reviewed-by: Greg Rose 
Tested-by: Greg Rose 

Applied to master.  If it should be backported, please let me know.


It will only apply to branches with erspan so I think 2.10 is as far 
back as it can go.


Thanks,

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

Re: [ovs-dev] [PATCH v3] datapath: support upstream ndo_udp_tunnel_add in net_device_ops

[Ben Pfaff]

On Tue, Aug 07, 2018 at 01:53:15PM -0700, Gregory Rose wrote:
> On 8/4/2018 1:31 AM, we...@ucloud.cn wrote:
> >From: wenxu 
> >
> >It makes datapath can support both ndo_add_udp_tunnel_port and
> >ndo_add_vxlan/geneve_port. The newer kernels don't support vxlan/geneve
> >specific NDO's anymore
> >
> >Signed-off-by: wenxu 
> 
> LGTM
> 
> Reviewed-by: Greg Rose 
> Tested-by: Greg Rose 

Applied to master.  If it should be backported, please let me know.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] ip_gre: remove redundant variables t_hlen

[Ben Pfaff]

On Tue, Aug 07, 2018 at 02:48:53PM -0700, Greg Rose wrote:
> From: YueHaibing 
> 
> Upstream commit:
> From: YueHaibing 
> Date: Wed, 1 Aug 2018 10:04:02 +0800
> Subject: [PATCH] ip_gre: remove redundant variables t_hlen
> 
> After commit ffc2b6ee4174 ("ip_gre: fix IFLA_MTU ignored on NEWLINK")
> variable t_hlen is assigned values that are never read,
> hence they are redundant and can be removed.
> 
> Signed-off-by: YueHaibing 
> Signed-off-by: David S. Miller 
> 
> Cc: YueHaibing 
> Signed-off-by: Greg Rose 

These looked like straightforward backports so I applied them to master.

It's not clear to me whether they fixed any bugs so I didn't backport
them, please let me know if I should.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: Ignore recirc_id in "MPLS xlate action" test.

[Ben Pfaff]

On Tue, Aug 07, 2018 at 06:19:52PM -0300, Flavio Leitner wrote:
> On Thu, Jul 12, 2018 at 02:55:31PM -0700, Ben Pfaff wrote:
> > When I run this test with DPDK enabled, it fails because it ends up using
> > a different recirculation ID when DPDK is not enabled.  I guess that's a
> > little weird but the recirculation IDs are not supposed to be significant,
> > so this change makes the test ignore it.
> > 
> > Signed-off-by: Ben Pfaff 
> 
> One nit below, otherwise looks good and works for me.
> 
> Acked-by: Flavio Leitner 

Thanks, applied to master and branch-2.10.

> > ---
> >  tests/mpls-xlate.at | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
> > index ad3141c6412c..e335b7fdf0a0 100644
> > --- a/tests/mpls-xlate.at
> > +++ b/tests/mpls-xlate.at
> > @@ -129,11 +129,14 @@ AT_CHECK([tail -1 stdout], [0],
> >  
> >  dnl Double MPLS pop
> >  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
> > 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=60,tc=0,ttl=64,bos=0,label=50,tc=0,ttl=64,bos=1)'],
> >  [0], [stdout])
> > -AT_CHECK([tail -1 stdout], [0],
> > -  [Datapath actions: 
> > pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(0x7)
> > +AT_CHECK([tail -1 stdout | sed 's/recirc(0x[[0-9a-f]]*)/recirc(?)/'], [0],
> > +  [Datapath actions: 
> > pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(?)
> >  ])
> >  
> > -AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
> > 'recirc_id(7),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'],
> >  [0], [stdout])
> > +recirc_id=$(tail -1 stdout | sed 's/.*recirc(0x\([[0-9a-f]]*\)).*/\1/')
> > +echo "recirc_id $recirc_id"
> 
> Debug leftover?

Maybe, but it also might be useful if someone sends me a test failure
later.

Thanks,

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

Re: [ovs-dev] [PATCH] tests: Ignore recirc_id in "MPLS xlate action" test.

[Ben Pfaff]

On Fri, Jul 13, 2018 at 09:11:54AM -0400, Aaron Conole wrote:
> Ben Pfaff  writes:
> 
> > When I run this test with DPDK enabled, it fails because it ends up using
> > a different recirculation ID when DPDK is not enabled.  I guess that's a
> > little weird but the recirculation IDs are not supposed to be significant,
> > so this change makes the test ignore it.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> 
> Thanks for this patch!
> 
> I'm pretty sure that with this patch applied, I can re-enable the make
> distcheck for the bot.  I looked at this test a bit earlier, but didn't
> know if the recirc ID was important.  Thanks for detailing that.
> 
> After this gets applied, I'll wait a bit and see if the tests
> consistently pass.  If that's so, I'll re-enable the make distcheck in
> the bot for output.
> 
> Acked-by: Aaron Conole 

Thanks, applied to master and branch-2.10.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] infiniband (IPoIB) support

[Vasiliy Tolstov]

Does anybody can helps me and say, how to do connectivity from ovn
network to physical? if ovn network and phisical have the same subnet.
пн, 6 авг. 2018 г. в 23:35, Vasiliy Tolstov :
>
> пн, 6 авг. 2018 г. в 17:34, Vasiliy Tolstov :
> >
> > And if IPoIB device cant't be added to openvswitch bridge, how can i
> > connect virtual network with physical in such setup:
> >
> > i have logical switch extnet with vm ports, each vm via ovn dhcp have
> > ip address from external network.
> > I want to route all traffic from this extnet via last ip address from
> > /24 subnet (254).
> > Does i need to create logical router for such thing?
> > Can you explain me how can i solve this?
> > пн, 6 авг. 2018 г. в 17:11, Vasiliy Tolstov :
> > >
> > > Hi. I know about dpdk, but i have mellanox connectx-2 card with IPoIB in 
> > > linux.
> > > And i want to add it to openvswitch bridge.
> > > I found topics (7 years ago) that says about no plans to add support
> > > for IPoIB devices.
> > > How can i add to ovs bridge IPoIB device?
> > >
> > > --
> > > Vasiliy Tolstov,
> > > e-mail: v.tols...@selfip.ru
> >
> >
> >
> > --
> > Vasiliy Tolstov,
> > e-mail: v.tols...@selfip.ru
>
> I'm partially solve problem by adding to extnet gw port with type localnet.
> so connection from vm to external world works fine, but from external
> world to vm not:
> ovs-vsctl show
> f39cbc63-2dd2-486e-a2c8-1b7faee48535
> Bridge br-ext
> Port patch-gw-to-br-int
> Interface patch-gw-to-br-int
> type: patch
> options: {peer=patch-br-int-to-gw}
> Port br-ext
> Interface br-ext
> type: internal
> Bridge br-int
> fail_mode: secure
> Port patch-br-int-to-gw
> Interface patch-br-int-to-gw
> type: patch
> options: {peer=patch-gw-to-br-int}
> Port "vnet0"
> Interface "vnet0"
> Port br-int
> Interface br-int
> type: internal
> ovs_version: "2.8.1"
>
> port gw
> type: localnet
> addresses: ["unknown"]
> port d2b171b6-c501-4e8c-b29d-adc79e573d4c
> addresses: ["52:54:00:08:43:25 xx.xx.xx.xx"]
> --
> Vasiliy Tolstov,
> e-mail: v.tols...@selfip.ru



-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/2] ip_gre: remove redundant variables t_hlen

[Greg Rose]

From: YueHaibing 

Upstream commit:
From: YueHaibing 
Date: Wed, 1 Aug 2018 10:04:02 +0800
Subject: [PATCH] ip_gre: remove redundant variables t_hlen

After commit ffc2b6ee4174 ("ip_gre: fix IFLA_MTU ignored on NEWLINK")
variable t_hlen is assigned values that are never read,
hence they are redundant and can be removed.

Signed-off-by: YueHaibing 
Signed-off-by: David S. Miller 

Cc: YueHaibing 
Signed-off-by: Greg Rose 
---
 datapath/linux/compat/ip_gre.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/datapath/linux/compat/ip_gre.c b/datapath/linux/compat/ip_gre.c
index 47ee525..47ed5e0 100644
--- a/datapath/linux/compat/ip_gre.c
+++ b/datapath/linux/compat/ip_gre.c
@@ -681,7 +681,6 @@ err_free_skb:
 static void __gre_tunnel_init(struct net_device *dev)
 {
struct ip_tunnel *tunnel;
-   int t_hlen;
 
tunnel = netdev_priv(dev);
tunnel->tun_hlen = ip_gre_calc_hlen(tunnel->parms.o_flags);
@@ -689,8 +688,6 @@ static void __gre_tunnel_init(struct net_device *dev)
 
tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
 
-   t_hlen = tunnel->hlen + sizeof(struct iphdr);
-
dev->features   |= GRE_FEATURES;
dev->hw_features|= GRE_FEATURES;
 
@@ -1031,13 +1028,11 @@ EXPORT_SYMBOL_GPL(ovs_gre_fill_metadata_dst);
 static int erspan_tunnel_init(struct net_device *dev)
 {
struct ip_tunnel *tunnel = netdev_priv(dev);
-   int t_hlen;
 
tunnel->tun_hlen = 8;
tunnel->parms.iph.protocol = IPPROTO_GRE;
tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
   erspan_hdr_len(tunnel->erspan_ver);
-   t_hlen = tunnel->hlen + sizeof(struct iphdr);
 
dev->features   |= GRE_FEATURES;
dev->hw_features|= GRE_FEATURES;
-- 
1.8.3.1

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

[ovs-dev] [PATCH 1/2] ip_gre: fix IFLA_MTU ignored on NEWLINK

[Greg Rose]

From: Xin Long 

Upstream commit:
From: Xin Long 
Date: Tue, 27 Feb 2018 19:19:39 +0800
Subject: [PATCH] ip_gre: fix IFLA_MTU ignored on NEWLINK

It's safe to remove the setting of dev's needed_headroom and mtu in
__gre_tunnel_init, as discussed in [1], ip_tunnel_newlink can do it
properly.

Now Eric noticed that it could cover the mtu value set in do_setlink
when creating a ip_gre dev. It makes IFLA_MTU param not take effect.

So this patch is to remove them to make IFLA_MTU work, as in other
ipv4 tunnels.

  [1]: https://patchwork.ozlabs.org/patch/823504/

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Reported-by: Eric Garver 
Signed-off-by: Xin Long 
Signed-off-by: David S. Miller 

Part of this commit already made it into __gre_tunnel_init but
the piece for erspan_tunnel_init did not make it in so fix that
now.

Cc: Xin Long 
Signed-off-by: Greg Rose 
---
 datapath/linux/compat/ip_gre.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/datapath/linux/compat/ip_gre.c b/datapath/linux/compat/ip_gre.c
index 1ab7981..47ee525 100644
--- a/datapath/linux/compat/ip_gre.c
+++ b/datapath/linux/compat/ip_gre.c
@@ -1039,8 +1039,6 @@ static int erspan_tunnel_init(struct net_device *dev)
   erspan_hdr_len(tunnel->erspan_ver);
t_hlen = tunnel->hlen + sizeof(struct iphdr);
 
-   dev->needed_headroom = LL_MAX_HEADER + t_hlen + 4;
-   dev->mtu = ETH_DATA_LEN - t_hlen - 4;
dev->features   |= GRE_FEATURES;
dev->hw_features|= GRE_FEATURES;
dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-- 
1.8.3.1

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

Re: [ovs-dev] DNS async resolve breaks testsuite if no resolv.conf

[Ben Pfaff]

On Tue, Aug 07, 2018 at 05:39:31PM -0300, Flavio Leitner wrote:
> On Tue, Aug 07, 2018 at 12:41:03PM -0700, Ben Pfaff wrote:
> > On Tue, Aug 07, 2018 at 03:36:45PM -0300, Flavio Leitner wrote:
> > > My test system and build system doesn't have /etc/resolv.conf file
> > > and then a warning is printed which breaks a number of tests in
> > > master and branch-2.10.
> > > 
> > > Looks like unbound comes with hardcoded root servers list which could
> > > be the fallback in case of no /etc/resolv.conf, so that message would
> > > be an INFO and could be ignored by the testsuite.
> > > 
> > > What do you think?
> > 
> > It's easy enough to fix:
> > https://patchwork.ozlabs.org/patch/954665/
> 
> Thanks will have a look.
>  
> > Do we need a similar fix for reading /etc/hosts?
> 
> I think /etc/hosts is always created with at least the localhost info.

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

Re: [ovs-dev] [PATCH] dns-resolve: Only ask unbound to read /etc/resolv.conf if it exists.

[Ben Pfaff]

On Tue, Aug 07, 2018 at 05:41:24PM -0300, Flavio Leitner wrote:
> On Tue, Aug 07, 2018 at 12:40:13PM -0700, Ben Pfaff wrote:
> > The unbound library complains if we ask it to read /etc/resolv.conf but
> > that file doesn't exist.  It's better to just skip reading it in that case.
> > 
> > Reported-by: Flavio Leitner 
> > Reporetd-at: 
> > https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350751.html
> > Signed-off-by: Ben Pfaff 
> > ---
> 
> Thanks Ben, looks good and works here.
> Acked-by: Flavio Leitner 

Thanks, applied to master and branch-2.10.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: use new info-level logging helper when sending out an in_port

[Ben Pfaff]

On Tue, Aug 07, 2018 at 02:13:17PM -0700, Zak Whittington wrote:
> Added new helper function similar to xlate_report_error called
> xlate_report_info that logs info-level messages, and used that
> function to add an extra log message when attempting to send
> out an in-port.
> 
> VMware-BZ: 2158607
> Signed-off-by: Zak Whittington 

Thanks!  Applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 1/2] ovs python: ovs.stream.open_block() returns success even if the remote is unreachable

[Ben Pfaff]

On Tue, Aug 07, 2018 at 05:12:32PM -0400, Mark Michelson wrote:
> On 08/07/2018 07:37 AM, nusid...@redhat.com wrote:
> >From: Numan Siddique 
> >
> >The python function ovs.socket_util.check_connection_completion() uses 
> >select()
> >(provided by python) to monitor the socket file descriptor. The select()
> >returns 1 when the file descriptor becomes ready. For error cases like -
> >111 (Connection refused) and 113 (No route to host) (POLLERR), 
> >ovs.poller._SelectSelect.poll()
> >expects the exceptfds list to be set by select(). But that is not the case.
> >As per the select() man page, writefds list will be set for POLLERR.
> >Please see "Correspondence between select() and poll() notifications" 
> >section of select(2)
> >man page.
> >
> >Because of this behavior, ovs.socket_util.check_connection_completion() 
> >returns success
> >even if the remote is unreachable or not listening on the port.
> >
> >This patch fixes this issue by using poll() to check the connection status 
> >similar to
> >the C implementation of check_connection_completion().
> >
> >A new function 'get_system_poll() is added in ovs/poller.py which returns the
> >select.poll() object. If select.poll is monkey patched by eventlet/gevent, it
> >gets the original select.poll() and returns it.
> 
> Is this safe? My concern is that eventlet/gevent monkey patches
> select.poll() so that the green thread yields properly. Using the system
> select.poll() might lead to unexpected blocking.

gevent monkey patches Python to get rid of poll() because poll() might
block and thereby stall Python.  This use of poll() is OK because it
will never block (because it uses a timeout of 0).
 
> I read your conversation with Ben on the previous iteration of this series.
> It looks like you attempted to use the system select.poll() and ran into
> this blocking problem (the pastebin for your patch is now deleted so I can't
> see what you actually tried). Why would this approach work differently?

I don't recall what was in the pastebin, but the use of poll() here
should be sound.  We use the same approach in C and we don't want to
block in C either.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: Ignore recirc_id in "MPLS xlate action" test.

[Flavio Leitner]

On Thu, Jul 12, 2018 at 02:55:31PM -0700, Ben Pfaff wrote:
> When I run this test with DPDK enabled, it fails because it ends up using
> a different recirculation ID when DPDK is not enabled.  I guess that's a
> little weird but the recirculation IDs are not supposed to be significant,
> so this change makes the test ignore it.
> 
> Signed-off-by: Ben Pfaff 

One nit below, otherwise looks good and works for me.

Acked-by: Flavio Leitner 


> ---
>  tests/mpls-xlate.at | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
> index ad3141c6412c..e335b7fdf0a0 100644
> --- a/tests/mpls-xlate.at
> +++ b/tests/mpls-xlate.at
> @@ -129,11 +129,14 @@ AT_CHECK([tail -1 stdout], [0],
>  
>  dnl Double MPLS pop
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
> 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=60,tc=0,ttl=64,bos=0,label=50,tc=0,ttl=64,bos=1)'],
>  [0], [stdout])
> -AT_CHECK([tail -1 stdout], [0],
> -  [Datapath actions: 
> pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(0x7)
> +AT_CHECK([tail -1 stdout | sed 's/recirc(0x[[0-9a-f]]*)/recirc(?)/'], [0],
> +  [Datapath actions: 
> pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(?)
>  ])
>  
> -AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
> 'recirc_id(7),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'],
>  [0], [stdout])
> +recirc_id=$(tail -1 stdout | sed 's/.*recirc(0x\([[0-9a-f]]*\)).*/\1/')
> +echo "recirc_id $recirc_id"

Debug leftover?

> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
> "recirc_id($recirc_id),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)"],
>  [0], [stdout])
>  AT_CHECK([tail -1 stdout], [0],
>[Datapath actions: set(ipv4(ttl=10)),100
>  ])
> -- 
> 2.16.1
> 
-- 
Flavio

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

[ovs-dev] [PATCH] ofproto-dpif-xlate: use new info-level logging helper when sending out an in_port

[Zak Whittington]

Added new helper function similar to xlate_report_error called
xlate_report_info that logs info-level messages, and used that
function to add an extra log message when attempting to send
out an in-port.

VMware-BZ: 2158607
Signed-off-by: Zak Whittington 
---
 ofproto/ofproto-dpif-xlate.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 01f1faf..aa169bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -674,6 +674,34 @@ xlate_report_error(const struct xlate_ctx *ctx, const char 
*format, ...)
 ds_destroy();
 }
 
+/* This is like xlate_report() for messages that should be logged
+   at the info level (even when not tracing). */
+static void OVS_PRINTF_FORMAT(2, 3)
+xlate_report_info(const struct xlate_ctx *ctx, const char *format, ...)
+{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+if (!OVS_UNLIKELY(ctx->xin->trace)
+&& (!ctx->xin->packet || VLOG_DROP_INFO())) {
+return;
+}
+
+struct ds s = DS_EMPTY_INITIALIZER;
+va_list args;
+va_start(args, format);
+ds_put_format_valist(, format, args);
+va_end(args);
+
+if (ctx->xin->trace) {
+oftrace_report(ctx->xin->trace, OFT_WARN, ds_cstr());
+} else {
+ds_put_format(, " on bridge %s while processing ",
+  ctx->xbridge->name);
+flow_format(, >base_flow, NULL);
+VLOG_INFO("%s", ds_cstr());
+}
+ds_destroy();
+}
+
 /* This is like xlate_report() for messages that should be logged at debug
  * level (even if we are not tracing) because they can be valuable for
  * debugging. */
@@ -5007,7 +5035,7 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t 
port,
 if (port != ctx->xin->flow.in_port.ofp_port) {
 compose_output_action(ctx, port, NULL, is_last_action, truncate);
 } else {
-xlate_report(ctx, OFT_WARN, "skipping output to input port");
+xlate_report_info(ctx, "skipping output to input port");
 }
 break;
 }
@@ -5092,7 +5120,7 @@ xlate_output_trunc_action(struct xlate_ctx *ctx,
 ctx->xout->slow |= SLOW_ACTION;
 }
 } else {
-xlate_report(ctx, OFT_WARN, "skipping output to input port");
+xlate_report_info(ctx, "skipping output to input port");
 }
 break;
 }
-- 
2.7.4

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

Re: [ovs-dev] [PATCH v4 1/2] ovs python: ovs.stream.open_block() returns success even if the remote is unreachable

[Mark Michelson]

Hi Numan, See below.

On 08/07/2018 07:37 AM, nusid...@redhat.com wrote:

From: Numan Siddique 

The python function ovs.socket_util.check_connection_completion() uses select()
(provided by python) to monitor the socket file descriptor. The select()
returns 1 when the file descriptor becomes ready. For error cases like -
111 (Connection refused) and 113 (No route to host) (POLLERR), 
ovs.poller._SelectSelect.poll()
expects the exceptfds list to be set by select(). But that is not the case.
As per the select() man page, writefds list will be set for POLLERR.
Please see "Correspondence between select() and poll() notifications" section 
of select(2)
man page.

Because of this behavior, ovs.socket_util.check_connection_completion() returns 
success
even if the remote is unreachable or not listening on the port.

This patch fixes this issue by using poll() to check the connection status 
similar to
the C implementation of check_connection_completion().

A new function 'get_system_poll() is added in ovs/poller.py which returns the
select.poll() object. If select.poll is monkey patched by eventlet/gevent, it
gets the original select.poll() and returns it.


Is this safe? My concern is that eventlet/gevent monkey patches 
select.poll() so that the green thread yields properly. Using the system 
select.poll() might lead to unexpected blocking.


I read your conversation with Ben on the previous iteration of this 
series. It looks like you attempted to use the system select.poll() and 
ran into this blocking problem (the pastebin for your patch is now 
deleted so I can't see what you actually tried). Why would this approach 
work differently?




The test cases added in this patch fails without the fix.

Suggested-by: Ben Pfaff 
Signed-off-by: Numan Siddique 
---
  python/ovs/poller.py  | 34 --
  python/ovs/socket_util.py |  5 +++--
  python/ovs/stream.py  | 11 +--
  tests/automake.mk |  1 +
  tests/ovsdb-idl.at| 16 
  tests/test-stream.py  | 32 
  6 files changed, 93 insertions(+), 6 deletions(-)
  create mode 100644 tests/test-stream.py

diff --git a/python/ovs/poller.py b/python/ovs/poller.py
index 2f3fcf9b6..ef67e6763 100644
--- a/python/ovs/poller.py
+++ b/python/ovs/poller.py
@@ -31,14 +31,21 @@ except ImportError:
  SSL = None
  
  try:

-import eventlet.patcher
+from eventlet import patcher as eventlet_patcher
  
  def _using_eventlet_green_select():

-return eventlet.patcher.is_monkey_patched(select)
+return eventlet_patcher.is_monkey_patched(select)
  except:
+eventlet_patcher = None
  def _using_eventlet_green_select():
  return False
  
+try:

+from gevent import monkey as gevent_monkey
+except:
+gevent_monkey = None
+
+
  vlog = ovs.vlog.Vlog("poller")
  
  POLLIN = 0x001

@@ -257,3 +264,26 @@ class Poller(object):
  def __reset(self):
  self.poll = SelectPoll()
  self.timeout = -1
+
+
+"""
+Returns the original select.poll() object. If select.poll is monkey patched
+by eventlet or gevent library, it gets the original select.poll and returns
+an object of it using the eventlet.patcher.original/gevent.monkey.get_original
+functions.
+
+As a last resort, if there is any exception it returns the SelectPoll() object.
+"""
+def get_system_poll():
+try:
+if _using_eventlet_green_select():
+_system_poll = eventlet_patcher.original("select").poll
+elif gevent_monkey and gevent_monkey.is_object_patched(
+'select', 'poll'):
+_system_poll = gevent_monkey.get_original('select', 'poll')
+else:
+_system_poll = select.poll
+except:
+_system_poll = SelectPoll
+
+return _system_poll()
diff --git a/python/ovs/socket_util.py b/python/ovs/socket_util.py
index 403104936..8e582fe91 100644
--- a/python/ovs/socket_util.py
+++ b/python/ovs/socket_util.py
@@ -162,8 +162,8 @@ def make_unix_socket(style, nonblock, bind_path, 
connect_path, short=False):
  
  
  def check_connection_completion(sock):

-p = ovs.poller.SelectPoll()
  if sys.platform == "win32":
+p = ovs.poller.SelectPoll()
  event = winutils.get_new_event(None, False, True, None)
  # Receive notification of readiness for writing, of completed
  # connection or multipoint join operation, and of socket closure.
@@ -173,6 +173,7 @@ def check_connection_completion(sock):
   win32file.FD_CLOSE)
  p.register(event, ovs.poller.POLLOUT)
  else:
+p = ovs.poller.get_system_poll()
  p.register(sock, ovs.poller.POLLOUT)
  pfds = p.poll(0)
  if len(pfds) == 1:
@@ -180,7 +181,7 @@ def check_connection_completion(sock):
  if revents & ovs.poller.POLLERR:
  try:
  # The following should raise an exception.
-socket.send("\0", 

Re: [ovs-dev] [PATCH v3] datapath: support upstream ndo_udp_tunnel_add in net_device_ops

[Gregory Rose]

On 8/4/2018 1:31 AM, we...@ucloud.cn wrote:

From: wenxu 

It makes datapath can support both ndo_add_udp_tunnel_port and
ndo_add_vxlan/geneve_port. The newer kernels don't support vxlan/geneve
specific NDO's anymore

Signed-off-by: wenxu 


LGTM

Reviewed-by: Greg Rose 
Tested-by: Greg Rose 


---
  acinclude.m4   |  1 +
  datapath/linux/compat/geneve.c | 41 ++--
  datapath/linux/compat/include/net/udp_tunnel.h | 14 +++
  datapath/linux/compat/vxlan.c  | 52 --
  4 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index d6e0d33..7899307 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -553,6 +553,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
[OVS_DEFINE([USE_UPSTREAM_TUNNEL_GSO])])
OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [ndo_add_vxlan_port])
OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [ndo_add_geneve_port])
+  OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [ndo_udp_tunnel_add])
OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [netdev_features_t])
dnl Ubuntu kernel 3.13 has defined this struct but not used for 
netdev->tstats.
dnl So check type of tstats.
diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c
index 435a23f..5b5bbae 100644
--- a/datapath/linux/compat/geneve.c
+++ b/datapath/linux/compat/geneve.c
@@ -432,6 +432,14 @@ static void geneve_notify_add_rx_port(struct geneve_sock 
*gs)
if (dev->netdev_ops->ndo_add_geneve_port)
dev->netdev_ops->ndo_add_geneve_port(dev, sa_family,
port);
+#elif defined(HAVE_NDO_UDP_TUNNEL_ADD)
+   struct udp_tunnel_info ti;
+   ti.type = UDP_TUNNEL_TYPE_GENEVE;
+   ti.sa_family = sa_family;
+   ti.port = inet_sk(sk)->inet_sport;
+
+   if (dev->netdev_ops->ndo_udp_tunnel_add)
+   dev->netdev_ops->ndo_udp_tunnel_add(dev, );
  #endif
}
rcu_read_unlock();
@@ -452,6 +460,14 @@ static void geneve_notify_del_rx_port(struct geneve_sock 
*gs)
if (dev->netdev_ops->ndo_del_geneve_port)
dev->netdev_ops->ndo_del_geneve_port(dev, sa_family,
port);
+#elif defined(HAVE_NDO_UDP_TUNNEL_ADD)
+   struct udp_tunnel_info ti;
+   ti.type = UDP_TUNNEL_TYPE_GENEVE;
+   ti.port = inet_sk(sk)->inet_sport;
+   ti.sa_family = sa_family;
+
+   if (dev->netdev_ops->ndo_udp_tunnel_del)
+   dev->netdev_ops->ndo_udp_tunnel_del(dev, );
  #endif
}
  
@@ -1301,9 +1317,9 @@ static struct device_type geneve_type = {

.name = "geneve",
  };
  
-/* Calls the ndo_add_geneve_port of the caller in order to

- * supply the listening GENEVE udp ports. Callers are expected
- * to implement the ndo_add_geneve_port.
+/* Calls the ndo_add_geneve_port or ndo_udp_tunnel_add of the caller
+ * in order to supply the listening GENEVE udp ports. Callers are
+ * expected to implement the ndo_add_geneve_port.
   */
  static void geneve_push_rx_ports(struct net_device *dev)
  {
@@ -1326,6 +1342,25 @@ static void geneve_push_rx_ports(struct net_device *dev)
dev->netdev_ops->ndo_add_geneve_port(dev, sa_family, port);
}
rcu_read_unlock();
+#elif defined(HAVE_NDO_UDP_TUNNEL_ADD)
+   struct net *net = dev_net(dev);
+   struct geneve_net *gn = net_generic(net, geneve_net_id);
+   struct geneve_sock *gs;
+   struct sock *sk;
+
+   if (!dev->netdev_ops->ndo_udp_tunnel_add)
+   return;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(gs, >sock_list, list) {
+   struct udp_tunnel_info ti;
+   ti.type = UDP_TUNNEL_TYPE_GENEVE;
+   sk = gs->sock->sk;
+   ti.port = inet_sk(sk)->inet_sport;
+   ti.sa_family = sk->sk_family;
+   dev->netdev_ops->ndo_udp_tunnel_add(dev, );
+   }
+   rcu_read_unlock();
  #endif
  }
  
diff --git a/datapath/linux/compat/include/net/udp_tunnel.h b/datapath/linux/compat/include/net/udp_tunnel.h

index 6b5e540..6e40633 100644
--- a/datapath/linux/compat/include/net/udp_tunnel.h
+++ b/datapath/linux/compat/include/net/udp_tunnel.h
@@ -43,6 +43,20 @@ struct udp_port_cfg {
ipv6_v6only:1;
  };
  
+#ifdef HAVE_NDO_UDP_TUNNEL_ADD

+enum udp_parsable_tunnel_type {
+   UDP_TUNNEL_TYPE_VXLAN,  /* RFC 7348 */
+   UDP_TUNNEL_TYPE_GENEVE, /* draft-ietf-nvo3-geneve */
+   UDP_TUNNEL_TYPE_VXLAN_GPE,  /* draft-ietf-nvo3-vxlan-gpe */
+};
+
+struct udp_tunnel_info {
+   unsigned short type;
+   sa_family_t sa_family;
+   __be16 port;
+};
+#endif
+
  #define udp_sock_create4 rpl_udp_sock_create4
  int rpl_udp_sock_create4(struct net *net, 

Re: [ovs-dev] [PATCH 3/3] ovn-trace: Fix warnings when port is found but not in current datapath.

[Mark Michelson]

Acked-by: Mark Michelson 

With a patch like this, I like to look at it and ask myself "What will 
this break?" In this case, I believe nothing will actually be broken.


On 08/06/2018 10:44 PM, Han Zhou wrote:

When port group is used, ovn-trace may print warnings like this:

$ ovn-trace ls1 'inport == "lp111" && eth.src == f0:00:00:00:01:11 && eth.dst == f0:00:00:00:01:12  
&& ip4.src == 192.168.11.1 && ip4.dst == 192.168.11.2 && ip.ttl == 10'
2018-08-02T01:43:23Z|1|ovntrace|WARN|lp211: not in datapath ls1
2018-08-02T01:43:23Z|2|ovntrace|WARN|lp211: unknown logical port
2018-08-02T01:43:23Z|3|ovntrace|WARN|lp221: not in datapath ls1
2018-08-02T01:43:23Z|4|ovntrace|WARN|lp221: unknown logical port
2018-08-02T01:43:23Z|5|ovntrace|WARN|lp231: not in datapath ls1
2018-08-02T01:43:23Z|6|ovntrace|WARN|lp231: unknown logical port

There are 2 warnings:

For the first one, it might be reasonable
before port group is supported, but now since ports in a port group
can span across multiple datapaths, this situation is normal, and
warning should not be printed.

For the second one, it is misleading, and it should not be printed
in this situation even before port group is supported. It should be
printed only if the port is not found at all.

This patch fixes both.

Signed-off-by: Han Zhou 
---
  ovn/utilities/ovn-trace.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 1fd48f2..7ca3d97 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -1020,7 +1020,9 @@ ovntrace_lookup_port(const void *dp_, const char 
*port_name,
  *portp = port->tunnel_key;
  return true;
  }
-VLOG_WARN("%s: not in datapath %s", port_name, dp->name);
+/* The port is found but not in this datapath. It happens when port
+ * group is used in match conditions. */
+return false;
  }
  
  const struct ovntrace_mcgroup *mcgroup = ovntrace_mcgroup_find_by_name(dp, port_name);




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

Re: [ovs-dev] [PATCH 2/3] ovn-northd: Improve efficiency of stateful checking for ACLs on port groups.

[Mark Michelson]

Nice find, Han!

Acked-by: Mark Michelson 

On 08/06/2018 10:44 PM, Han Zhou wrote:

Currently in has_stateful_acl(), to check if a datapath has stateful ACLs,
it needs to iterate all port groups and check if the current datapath is
related to each port group, and then iterate the ACLs on the port group. This
is inefficient if there are a lot of port groups. A typical scenario is in
OpenStack each tenant will have a default security group which will be mapped
as a port group, and the default security group is supposed to contain ports
of the tenant only, so most likely only the logical switches belonging to the
tenant should be related to the port group, but we are checking all the port
groups belonging to all tenants for each datapath.

To improve this, a reverse direction of hmap is built from logical switch to
port group, so that the iteration is avoided. The time complexity of this
function improves from O(P * A) to O(PL * A), P = total number of port groups
in NB, PL = number of port groups related to the logical switch, A = number
of ACLs.

Signed-off-by: Han Zhou 
---
  ovn/northd/ovn-northd.c | 60 +
  1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index d2a777f..ba86bf5 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -439,6 +439,9 @@ struct ovn_datapath {
   * the "redirect-chassis". */
  struct ovn_port *l3redirect_port;
  struct ovn_port *localnet_port;
+
+/* Port groups related to the datapath, used only when nbs is NOT NULL. */
+struct hmap nb_pgs;
  };
  
  struct macam_node {

@@ -467,11 +470,14 @@ ovn_datapath_create(struct hmap *datapaths, const struct 
uuid *key,
  od->nbs = nbs;
  od->nbr = nbr;
  hmap_init(>port_tnlids);
+hmap_init(>nb_pgs);
  od->port_key_hint = 0;
  hmap_insert(datapaths, >key_node, uuid_hash(>key));
  return od;
  }
  
+static void ovn_ls_port_group_destroy(struct hmap *nb_pgs);

+
  static void
  ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
  {
@@ -483,6 +489,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct 
ovn_datapath *od)
  destroy_tnlids(>port_tnlids);
  bitmap_free(od->ipam_info.allocated_ipv4s);
  free(od->router_ports);
+ovn_ls_port_group_destroy(>nb_pgs);
  free(od);
  }
  }
@@ -3053,8 +3060,34 @@ ovn_port_group_ls_find(struct ovn_port_group *pg, const 
struct uuid *ls_uuid)
  return NULL;
  }
  
+struct ovn_ls_port_group {

+struct hmap_node key_node;  /* Index on 'key'. */
+struct uuid key;/* nb_pg->header_.uuid. */
+const struct nbrec_port_group *nb_pg;
+};
+
+static void
+ovn_ls_port_group_add(struct hmap *nb_pgs,
+  const struct nbrec_port_group *nb_pg)
+{
+struct ovn_ls_port_group *ls_pg = xzalloc(sizeof *ls_pg);
+ls_pg->key = nb_pg->header_.uuid;
+ls_pg->nb_pg = nb_pg;
+hmap_insert(nb_pgs, _pg->key_node, uuid_hash(_pg->key));
+}
+
+static void
+ovn_ls_port_group_destroy(struct hmap *nb_pgs)
+{
+struct ovn_ls_port_group *ls_pg;
+HMAP_FOR_EACH_POP (ls_pg, key_node, nb_pgs) {
+free(ls_pg);
+}
+hmap_destroy(nb_pgs);
+}
+
  static bool
-has_stateful_acl(struct ovn_datapath *od, struct hmap *port_groups)
+has_stateful_acl(struct ovn_datapath *od)
  {
  for (size_t i = 0; i < od->nbs->n_acls; i++) {
  struct nbrec_acl *acl = od->nbs->acls[i];
@@ -3063,25 +3096,23 @@ has_stateful_acl(struct ovn_datapath *od, struct hmap 
*port_groups)
  }
  }
  
-struct ovn_port_group *pg;

-HMAP_FOR_EACH (pg, key_node, port_groups) {
-if (ovn_port_group_ls_find(pg, >nbs->header_.uuid)) {
-for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
-struct nbrec_acl *acl = pg->nb_pg->acls[i];
-if (!strcmp(acl->action, "allow-related")) {
-return true;
-}
+struct ovn_ls_port_group *ls_pg;
+HMAP_FOR_EACH (ls_pg, key_node, >nb_pgs) {
+for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) {
+struct nbrec_acl *acl = ls_pg->nb_pg->acls[i];
+if (!strcmp(acl->action, "allow-related")) {
+return true;
  }
  }
  }
+
  return false;
  }
  
  static void

-build_pre_acls(struct ovn_datapath *od, struct hmap *lflows,
-   struct hmap *port_groups)
+build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
  {
-bool has_stateful = has_stateful_acl(od, port_groups);
+bool has_stateful = has_stateful_acl(od);
  
  /* Ingress and Egress Pre-ACL Table (Priority 0): Packets are

   * allowed by default. */
@@ -3606,6 +3637,7 @@ build_port_group_lswitches(struct northd_context *ctx, 
struct hmap *pgs,
  ovn_port_group_ls_find(pg, >od->nbs->header_.uuid);
  if (!pg_ls) {
  

Re: [ovs-dev] [PATCH 1/3] ovn-northd: Simplify struct ovn_port_group.

[Mark Michelson]

Acked-by: Mark Michelson 

On 08/06/2018 10:44 PM, Han Zhou wrote:

Remove the redundant members that's already in nb_pg.

Signed-off-by: Han Zhou 
---
  ovn/northd/ovn-northd.c | 12 
  1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 067d52d..d2a777f 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -3027,8 +3027,6 @@ struct ovn_port_group {
  struct uuid key;/* nb_pg->header_.uuid. */
  const struct nbrec_port_group *nb_pg;
  struct hmap nb_lswitches;   /* NB lswitches related to the port group */
-size_t n_acls;  /* Number of ACLs applied to the port group */
-struct nbrec_acl **acls;/* ACLs applied to the port group */
  };
  
  static void

@@ -3068,8 +3066,8 @@ has_stateful_acl(struct ovn_datapath *od, struct hmap 
*port_groups)
  struct ovn_port_group *pg;
  HMAP_FOR_EACH (pg, key_node, port_groups) {
  if (ovn_port_group_ls_find(pg, >nbs->header_.uuid)) {
-for (size_t i = 0; i < pg->n_acls; i++) {
-struct nbrec_acl *acl = pg->acls[i];
+for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
+struct nbrec_acl *acl = pg->nb_pg->acls[i];
  if (!strcmp(acl->action, "allow-related")) {
  return true;
  }
@@ -3558,8 +3556,6 @@ ovn_port_group_create(struct hmap *pgs,
  struct ovn_port_group *pg = xzalloc(sizeof *pg);
  pg->key = nb_pg->header_.uuid;
  pg->nb_pg = nb_pg;
-pg->n_acls = nb_pg->n_acls;
-pg->acls = nb_pg->acls;
  hmap_init(>nb_lswitches);
  hmap_insert(pgs, >key_node, uuid_hash(>key));
  return pg;
@@ -3723,8 +3719,8 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
  struct ovn_port_group *pg;
  HMAP_FOR_EACH (pg, key_node, port_groups) {
  if (ovn_port_group_ls_find(pg, >nbs->header_.uuid)) {
-for (size_t i = 0; i < pg->n_acls; i++) {
-consider_acl(lflows, od, pg->acls[i], has_stateful);
+for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
+consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful);
  }
  }
  }



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

Re: [ovs-dev] [PATCH] dns-resolve: Only ask unbound to read /etc/resolv.conf if it exists.

[Flavio Leitner]

On Tue, Aug 07, 2018 at 12:40:13PM -0700, Ben Pfaff wrote:
> The unbound library complains if we ask it to read /etc/resolv.conf but
> that file doesn't exist.  It's better to just skip reading it in that case.
> 
> Reported-by: Flavio Leitner 
> Reporetd-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350751.html
> Signed-off-by: Ben Pfaff 
> ---

Thanks Ben, looks good and works here.
Acked-by: Flavio Leitner 


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

Re: [ovs-dev] DNS async resolve breaks testsuite if no resolv.conf

[Flavio Leitner]

On Tue, Aug 07, 2018 at 12:41:03PM -0700, Ben Pfaff wrote:
> On Tue, Aug 07, 2018 at 03:36:45PM -0300, Flavio Leitner wrote:
> > My test system and build system doesn't have /etc/resolv.conf file
> > and then a warning is printed which breaks a number of tests in
> > master and branch-2.10.
> > 
> > Looks like unbound comes with hardcoded root servers list which could
> > be the fallback in case of no /etc/resolv.conf, so that message would
> > be an INFO and could be ignored by the testsuite.
> > 
> > What do you think?
> 
> It's easy enough to fix:
> https://patchwork.ozlabs.org/patch/954665/

Thanks will have a look.
 
> Do we need a similar fix for reading /etc/hosts?

I think /etc/hosts is always created with at least the localhost info.

-- 
Flavio

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

Re: [ovs-dev] DNS async resolve breaks testsuite if no resolv.conf

[Ben Pfaff]

On Tue, Aug 07, 2018 at 03:36:45PM -0300, Flavio Leitner wrote:
> My test system and build system doesn't have /etc/resolv.conf file
> and then a warning is printed which breaks a number of tests in
> master and branch-2.10.
> 
> Looks like unbound comes with hardcoded root servers list which could
> be the fallback in case of no /etc/resolv.conf, so that message would
> be an INFO and could be ignored by the testsuite.
> 
> What do you think?

It's easy enough to fix:
https://patchwork.ozlabs.org/patch/954665/

Do we need a similar fix for reading /etc/hosts?

Thanks,

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

[ovs-dev] [PATCH] dns-resolve: Only ask unbound to read /etc/resolv.conf if it exists.

[Ben Pfaff]

The unbound library complains if we ask it to read /etc/resolv.conf but
that file doesn't exist.  It's better to just skip reading it in that case.

Reported-by: Flavio Leitner 
Reporetd-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350751.html
Signed-off-by: Ben Pfaff 
---
 lib/dns-resolve.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/dns-resolve.c b/lib/dns-resolve.c
index f1f91129609a..299ab27ab5ca 100644
--- a/lib/dns-resolve.c
+++ b/lib/dns-resolve.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "hash.h"
 #include "openvswitch/hmap.h"
@@ -81,17 +82,20 @@ dns_resolve_init(bool is_daemon)
 return;
 }
 
-int retval;
 #ifdef __linux__
-retval = ub_ctx_resolvconf(ub_ctx__, "/etc/resolv.conf");
-if (retval != 0) {
-VLOG_WARN_RL(, "Failed to read /etc/resolv.conf: %s",
- ub_strerror(retval));
+const char *filename = "/etc/resolv.conf";
+struct stat s;
+if (!stat(filename, ) || errno != ENOENT) {
+int retval = ub_ctx_resolvconf(ub_ctx__, filename);
+if (retval != 0) {
+VLOG_WARN_RL(, "Failed to read %s: %s",
+ filename, ub_strerror(retval));
+}
 }
 #endif
 
 /* Handles '/etc/hosts' on Linux and 'WINDIR/etc/hosts' on Windows. */
-retval = ub_ctx_hosts(ub_ctx__, NULL);
+int retval = ub_ctx_hosts(ub_ctx__, NULL);
 if (retval != 0) {
 VLOG_WARN_RL(, "Failed to read etc/hosts: %s",
  ub_strerror(retval));
-- 
2.16.1

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

Re: [ovs-dev] [PATCH v3 0/3] Transparent use of daemon for ovn-nbctl

[Ben Pfaff]

Thanks, series applied to master and branch-2.10.

On Tue, Aug 07, 2018 at 02:39:21PM -0400, Mark Michelson wrote:
> Acked-by: Mark Michelson 
> 
> On 08/06/2018 05:45 PM, Ben Pfaff wrote:
> >v1->v2:
> >   - Applied patches 1 and 2; added ack for patch 3 (thanks Alin!)
> >   - Polished up the daemon mode so that it works actually quite well
> > and added tests that show that it behaves equivalently.
> >v2->v3:
> >   - Fix bug in patch 2 pointed out by Mark Michelson (thanks!) and add
> > his acks.
> >
> >Ben Pfaff (3):
> >   unixctl: Make path to unixctl_server socket available to the client.
> >   ovn-nbctl: Separate command-line options parsing and interpretation.
> >   ovn-nbctl: Make daemon mode more transparent.
> >
> >  NEWS  |   6 +-
> >  lib/command-line.c| 108 +++
> >  lib/command-line.h|  10 ++
> >  lib/daemon.h  |  19 +++
> >  lib/stream-ssl.h  |   5 +-
> >  lib/unixctl.c |  52 
> >  lib/unixctl.h |   2 +
> >  ovn/utilities/ovn-nbctl.8.xml |  61 +++--
> >  ovn/utilities/ovn-nbctl.c | 301 
> > +++---
> >  tests/daemon.at   |   4 +-
> >  tests/ovn-nbctl.at| 246 --
> >  11 files changed, 567 insertions(+), 247 deletions(-)
> >
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] DNS async resolve breaks testsuite if no resolv.conf

[0-day Robot]

Bleep bloop.  Greetings Flavio Leitner, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: No signatures found.
WARNING: Line is 95 characters long (recommended limit is 79)
#90 FILE: 
builddir/build/BUILD/ovs-7a78d1c1ad73ae6f8f07d91021b71f8d8a4848d6/tests/testsuite.dir/at-groups/1024/stdout:90:
2018-08-07T17:56:33Z|1|dns_resolve|WARN|Failed to read /etc/resolv.conf: 
error reading file

Lines checked: 94, Warnings: 1, Errors: 1


build:
/bin/sh 
/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/build-aux/missing 
autom4te --language=autotest -I '.' -o tests/system-dpdk-testsuite.tmp 
tests/system-dpdk-testsuite.at
mv tests/system-dpdk-testsuite.tmp tests/system-dpdk-testsuite
\
{ sed -n -e '/%AUTHORS%/q' -e p < ./debian/copyright.in;   \
  sed '34,/^$/d' ./AUTHORS.rst |   \
sed -n -e '/^$/q' -e 's/^/  /p';   \
  sed -e '34,/%AUTHORS%/d' ./debian/copyright.in;  \
} > debian/copyright
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.10.90,g') < 
./rhel/openvswitch-dkms.spec.in > openvswitch-dkms.spec.tmp || exit 1; if cmp 
-s openvswitch-dkms.spec.tmp rhel/openvswitch-dkms.spec; then touch 
rhel/openvswitch-dkms.spec; rm openvswitch-dkms.spec.tmp; else mv 
openvswitch-dkms.spec.tmp rhel/openvswitch-dkms.spec; fi
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.10.90,g') < 
./rhel/kmod-openvswitch-rhel6.spec.in > kmod-openvswitch-rhel6.spec.tmp || exit 
1; if cmp -s kmod-openvswitch-rhel6.spec.tmp rhel/kmod-openvswitch-rhel6.spec; 
then touch rhel/kmod-openvswitch-rhel6.spec; rm 
kmod-openvswitch-rhel6.spec.tmp; else mv kmod-openvswitch-rhel6.spec.tmp 
rhel/kmod-openvswitch-rhel6.spec; fi
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.10.90,g') < 
./rhel/openvswitch-kmod-fedora.spec.in > openvswitch-kmod-fedora.spec.tmp || 
exit 1; if cmp -s openvswitch-kmod-fedora.spec.tmp 
rhel/openvswitch-kmod-fedora.spec; then touch 
rhel/openvswitch-kmod-fedora.spec; rm openvswitch-kmod-fedora.spec.tmp; else mv 
openvswitch-kmod-fedora.spec.tmp rhel/openvswitch-kmod-fedora.spec; fi
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.10.90,g') < 
./rhel/openvswitch.spec.in > openvswitch.spec.tmp || exit 1; if cmp -s 
openvswitch.spec.tmp rhel/openvswitch.spec; then touch rhel/openvswitch.spec; 
rm openvswitch.spec.tmp; else mv openvswitch.spec.tmp rhel/openvswitch.spec; fi
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.10.90,g') < 
./rhel/openvswitch-fedora.spec.in > openvswitch-fedora.spec.tmp || exit 1; if 
cmp -s openvswitch-fedora.spec.tmp rhel/openvswitch-fedora.spec; then touch 
rhel/openvswitch-fedora.spec; rm openvswitch-fedora.spec.tmp; else mv 
openvswitch-fedora.spec.tmp rhel/openvswitch-fedora.spec; fi
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.10.90,g') \
< ./xenserver/openvswitch-xen.spec.in > openvswitch-xen.spec.tmp || 
exit 1; \
if cmp -s openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; then touch 
xenserver/openvswitch-xen.spec; rm openvswitch-xen.spec.tmp; else mv 
openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; fi
make[3]: Entering directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/datapath'
make[3]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/datapath'
The following files are in git but not the distribution:
builddir/build/BUILD/ovs-7a78d1c1ad73ae6f8f07d91021b71f8d8a4848d6/tests/testsuite.dir/at-groups/1024/stdout
make[2]: *** [dist-hook-git] Error 1
make[2]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] raft: Fix use-after-free error in raft_store_snapshot().

[Ben Pfaff]

Thanks, applied to master, branch-2.10, branch-2.9.

On Tue, Aug 07, 2018 at 02:26:11PM -0400, Mark Michelson wrote:
> Looks good to me.
> 
> Acked-by: Mark Michelson 
> 
> On 08/06/2018 05:35 PM, Ben Pfaff wrote:
> >raft_store_snapshot() constructs a new snapshot in a local variable then
> >destroys the current snapshot and replaces it by the new one.  Until now,
> >it has not cloned the data in the new snapshot until it did the
> >replacement.  This led to the unexpected consequence that, if 'servers' in
> >the old and new snapshots was the same, then it would first be freed and
> >later cloned, which could cause a segfault.
> >
> >Multiple people reported the crash.  Gurucharan Shetty provided a
> >reproduction case.
> >
> >Signed-off-by: Ben Pfaff 
> >---
> >  ovsdb/raft.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> >diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> >index c0c1e98977b9..02ba763e5fc4 100644
> >--- a/ovsdb/raft.c
> >+++ b/ovsdb/raft.c
> >@@ -3838,22 +3838,22 @@ raft_store_snapshot(struct raft *raft, const struct 
> >json *new_snapshot_data)
> >  }
> >  uint64_t new_log_start = raft->last_applied + 1;
> >-const struct raft_entry new_snapshot = {
> >+struct raft_entry new_snapshot = {
> >  .term = raft_get_term(raft, new_log_start - 1),
> >-.data = CONST_CAST(struct json *, new_snapshot_data),
> >+.data = json_clone(new_snapshot_data),
> >  .eid = *raft_get_eid(raft, new_log_start - 1),
> >-.servers = CONST_CAST(struct json *,
> >-  raft_servers_for_index(raft, new_log_start - 
> >1)),
> >+.servers = json_clone(raft_servers_for_index(raft, new_log_start - 
> >1)),
> >  };
> >  struct ovsdb_error *error = raft_save_snapshot(raft, new_log_start,
> > _snapshot);
> >  if (error) {
> >+raft_entry_uninit(_snapshot);
> >  return error;
> >  }
> >  raft->log_synced = raft->log_end - 1;
> >  raft_entry_uninit(>snap);
> >-raft_entry_clone(>snap, _snapshot);
> >+raft->snap = new_snapshot;
> >  for (size_t i = 0; i < new_log_start - raft->log_start; i++) {
> >  raft_entry_uninit(>entries[i]);
> >  }
> >
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev,v3] tests: Test for ovs-ofctl snoop command

[Ashish Varma]

Thanks for clarifying.

On Tue, Aug 7, 2018 at 12:07 PM, Aaron Conole  wrote:

> Ashish Varma  writes:
>
> > Hi Aaron,
> >
> > My patch is already committed by Ben. Not sure what is wrong here.
>
> Thanks for the heads up.
>
> This happens - usually if the patch is committed before the bot has run
> against it.  I have it on my list of TODOs, so as soon as my next 'work
> on the bot' time comes, I'll fix it.
>
> > Is there something that I need to do now? (or should have done correctly)
>
> Without looking too deeply, most likely a false positive.  Feel free to
> not worry about it :)  I'll ping if it's a real issue somewhere.
>
> > Thanks,
> > Ashish
> > VMware OVS team
> >
> > On Mon, Aug 6, 2018 at 7:15 PM, 0-day Robot  wrote:
> >
> >  Bleep bloop.  Greetings Ashish Varma, I am a robot and I have tried out
> your patch.
> >  Thanks for your contribution.
> >
> >  I encountered some error that I wasn't expecting.  See the details
> below.
> >
> >  git-am:
> >  Failed to merge in the changes.
> >  Patch failed at 0001 tests: Test for ovs-ofctl snoop command
> >  The copy of the patch that failed is found in:
> > /var/lib/jenkins/jobs/upstream_build_from_pw/
> workspace/.git/rebase-apply/patch
> >  When you have resolved this problem, run "git am --resolved".
> >  If you prefer to skip this patch, run "git am --skip" instead.
> >  To restore the original branch and stop patching, run "git am --abort".
> >
> >  Please check this out.  If you feel there has been an error, please
> email acon...@bytheb.org
> >
> >  Thanks,
> >  0-day Robot
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev,v3] tests: Test for ovs-ofctl snoop command

[Aaron Conole]

Ashish Varma  writes:

> Hi Aaron,
>
> My patch is already committed by Ben. Not sure what is wrong here.

Thanks for the heads up.

This happens - usually if the patch is committed before the bot has run
against it.  I have it on my list of TODOs, so as soon as my next 'work
on the bot' time comes, I'll fix it.

> Is there something that I need to do now? (or should have done correctly)

Without looking too deeply, most likely a false positive.  Feel free to
not worry about it :)  I'll ping if it's a real issue somewhere.

> Thanks,
> Ashish 
> VMware OVS team
>
> On Mon, Aug 6, 2018 at 7:15 PM, 0-day Robot  wrote:
>
>  Bleep bloop.  Greetings Ashish Varma, I am a robot and I have tried out your 
> patch.
>  Thanks for your contribution.
>
>  I encountered some error that I wasn't expecting.  See the details below.
>
>  git-am:
>  Failed to merge in the changes.
>  Patch failed at 0001 tests: Test for ovs-ofctl snoop command
>  The copy of the patch that failed is found in:
> 
> /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
>  When you have resolved this problem, run "git am --resolved".
>  If you prefer to skip this patch, run "git am --skip" instead.
>  To restore the original branch and stop patching, run "git am --abort".
>
>  Please check this out.  If you feel there has been an error, please email 
> acon...@bytheb.org
>
>  Thanks,
>  0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 0/3] Transparent use of daemon for ovn-nbctl

[Mark Michelson]

Acked-by: Mark Michelson 

On 08/06/2018 05:45 PM, Ben Pfaff wrote:

v1->v2:
   - Applied patches 1 and 2; added ack for patch 3 (thanks Alin!)
   - Polished up the daemon mode so that it works actually quite well
 and added tests that show that it behaves equivalently.
v2->v3:
   - Fix bug in patch 2 pointed out by Mark Michelson (thanks!) and add
 his acks.

Ben Pfaff (3):
   unixctl: Make path to unixctl_server socket available to the client.
   ovn-nbctl: Separate command-line options parsing and interpretation.
   ovn-nbctl: Make daemon mode more transparent.

  NEWS  |   6 +-
  lib/command-line.c| 108 +++
  lib/command-line.h|  10 ++
  lib/daemon.h  |  19 +++
  lib/stream-ssl.h  |   5 +-
  lib/unixctl.c |  52 
  lib/unixctl.h |   2 +
  ovn/utilities/ovn-nbctl.8.xml |  61 +++--
  ovn/utilities/ovn-nbctl.c | 301 +++---
  tests/daemon.at   |   4 +-
  tests/ovn-nbctl.at| 246 --
  11 files changed, 567 insertions(+), 247 deletions(-)



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

[ovs-dev] DNS async resolve breaks testsuite if no resolv.conf

[Flavio Leitner]

Hi,

My test system and build system doesn't have /etc/resolv.conf file
and then a warning is printed which breaks a number of tests in
master and branch-2.10.

Looks like unbound comes with hardcoded root servers list which could
be the fallback in case of no /etc/resolv.conf, so that message would
be an INFO and could be ignored by the testsuite.

What do you think?

Details below.
lib/dns-resolve.c:
74 void
 75 dns_resolve_init(bool is_daemon)
 76 {
[...]
 84 int retval;
 85 #ifdef __linux__

 
 86 retval = ub_ctx_resolvconf(ub_ctx__, "/etc/resolv.conf");
 87 if (retval != 0) {
>88 VLOG_WARN_RL(, "Failed to read /etc/resolv.conf: %s",
 89  ub_strerror(retval));
 90 }
 91 #endif


## - ##
## Test results. ##
## - ##
ERROR: 2292 tests were run,
552 failed unexpectedly.
346 tests were skipped.
## -- ##
## testsuite.log was created. ##
## -- ##
Please send `tests/testsuite.log' and all information you think might help:
   To: 
   Subject: [openvswitch 2.10.0] testsuite: 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 
16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 
98 99 100 116 117 118 119 120 121 158 159 160 161 162 416 432 433 434 435 436 
437 438 439 440 441 444 445 446 447 448 454 455 456 473 474 475 476 477 478 479 
480 481 482 483 484 485 554 556 558 789 790 791 792 793 794 795 796 797 798 799 
800 801 802 803 804 805 806 807 808 809 810 811 812 813 814 815 816 817 818 869 
870 871 873 874 875
[the list continues...]


One sample:
1024. ofproto.at:5867: testing ofproto - bundle discard without open (OpenFlow 
1.3) ...
[...]
/1024/db.sock: connecting...
2018-08-07T17:56:33Z|7|netlink_socket|INFO|netlink: could not enable 
listening to all nsid (Operation not permitted)
2018-08-07T17:56:33Z|8|reconnect|INFO|unix:/builddir/build/BUILD/ovs-7a78d1c1ad73ae6f8f07d91021b71f8d8a4848d6/tests/testsuite.dir/1024/db.sock:
 connected
2018-08-07T17:56:33Z|9|dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init 
to enable
./ofproto.at:5869: sed < stderr '
/ovs_numa|INFO|Discovered /d
/vlog|INFO|opened log file/d
/vswitchd|INFO|ovs-vswitchd (Open vSwitch)/d
/reconnect|INFO|/d
/dpif_netlink|INFO|Generic Netlink family .ovs_datapath. does not exist/d
/ofproto|INFO|using datapath ID/d
/netdev_linux|INFO|.*device has unknown hardware address family/d
/ofproto|INFO|datapath ID changed to fedcba9876543210/d
/dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d
/netlink_socket|INFO|netlink: could not enable listening to all nsid/d
/netdev: Flow API/d
/probe tc:/d
/tc: Using policy/d'
--- /dev/null   2018-08-07 17:51:09.647040889 +
+++ 
/builddir/build/BUILD/ovs-7a78d1c1ad73ae6f8f07d91021b71f8d8a4848d6/tests/testsuite.dir/at-groups/1024/stdout
2018-08-07 17:56:33.547867577 +
@@ -0,0 +1 @@
+2018-08-07T17:56:33Z|1|dns_resolve|WARN|Failed to read /etc/resolv.conf: 
error reading file
ovsdb-server.log:
> 2018-08-07T17:56:33.490Z|1|vlog|INFO|opened log file 
> /builddir/build/BUILD/ovs-7a78d1c1ad73ae6f8f07d91021b71f8d8a4848d6/tests/testsuite.dir/1024/ovsdb-server.log
> 2018-08-07T17:56:33.494Z|2|ovsdb_server|INFO|ovsdb-server (Open vSwitch) 
> 2.10.0
ovs-vswitchd.log:


This is the commit introduced the DNS support:
commit 771680d96fb6f996987630cb85bb749478512268
Author: Yifeng Sun 
Date:   Tue Jun 26 14:06:21 2018 -0700

DNS: Add basic support for asynchronous DNS resolving

-- 
Flavio

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

Re: [ovs-dev] [PATCH] raft: Fix use-after-free error in raft_store_snapshot().

[Mark Michelson]

Looks good to me.

Acked-by: Mark Michelson 

On 08/06/2018 05:35 PM, Ben Pfaff wrote:

raft_store_snapshot() constructs a new snapshot in a local variable then
destroys the current snapshot and replaces it by the new one.  Until now,
it has not cloned the data in the new snapshot until it did the
replacement.  This led to the unexpected consequence that, if 'servers' in
the old and new snapshots was the same, then it would first be freed and
later cloned, which could cause a segfault.

Multiple people reported the crash.  Gurucharan Shetty provided a
reproduction case.

Signed-off-by: Ben Pfaff 
---
  ovsdb/raft.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index c0c1e98977b9..02ba763e5fc4 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -3838,22 +3838,22 @@ raft_store_snapshot(struct raft *raft, const struct 
json *new_snapshot_data)
  }
  
  uint64_t new_log_start = raft->last_applied + 1;

-const struct raft_entry new_snapshot = {
+struct raft_entry new_snapshot = {
  .term = raft_get_term(raft, new_log_start - 1),
-.data = CONST_CAST(struct json *, new_snapshot_data),
+.data = json_clone(new_snapshot_data),
  .eid = *raft_get_eid(raft, new_log_start - 1),
-.servers = CONST_CAST(struct json *,
-  raft_servers_for_index(raft, new_log_start - 1)),
+.servers = json_clone(raft_servers_for_index(raft, new_log_start - 1)),
  };
  struct ovsdb_error *error = raft_save_snapshot(raft, new_log_start,
 _snapshot);
  if (error) {
+raft_entry_uninit(_snapshot);
  return error;
  }
  
  raft->log_synced = raft->log_end - 1;

  raft_entry_uninit(>snap);
-raft_entry_clone(>snap, _snapshot);
+raft->snap = new_snapshot;
  for (size_t i = 0; i < new_log_start - raft->log_start; i++) {
  raft_entry_uninit(>entries[i]);
  }



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

Re: [ovs-dev] [PATCH] json: Use unnamed embedded union.

[Flavio Leitner]

On Mon, Aug 06, 2018 at 04:47:22PM -0700, Ben Pfaff wrote:
> On Thu, Aug 02, 2018 at 06:52:55PM -0300, Flavio Leitner wrote:
> > On Thu, Aug 02, 2018 at 06:44:10PM -0300, Flavio Leitner wrote:
> > > Otherwise the code does not build.
> > > 
> > > Fixes: fa37affad362 ("Embrace anonymous unions.")
> > > Signed-off-by: Flavio Leitner 
> > 
> > This needs to go in branch-2.10 as well.
> 
> Apparently I never build this code (oops!).
> 
> I applied this to master and branch-2.10.  I didn't actually build or
> test it or anything though.

Thanks Ben, works for me!

-- 
Flavio

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

[ovs-dev] [PATCH] ofproto-dpif-xlate: Improve log message.

[Ben Pfaff]

Until now, the bridge name was at the end of the log message, after the
flow, which made it easy to miss.  This commit moves it before the flow
where it is easier to spot.

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif-xlate.c | 4 ++--
 tests/ofproto-dpif.at| 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 01f1fafeb356..205178b306ac 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -666,9 +666,9 @@ xlate_report_error(const struct xlate_ctx *ctx, const char 
*format, ...)
 if (ctx->xin->trace) {
 oftrace_report(ctx->xin->trace, OFT_ERROR, ds_cstr());
 } else {
-ds_put_cstr(, " while processing ");
+ds_put_format(, " on bridge %s while processing ",
+  ctx->xbridge->name);
 flow_format(, >base_flow, NULL);
-ds_put_format(, " on bridge %s", ctx->xbridge->name);
 VLOG_WARN("%s", ds_cstr());
 }
 ds_destroy();
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index caa9b434cf62..362c58db437b 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8030,7 +8030,7 @@ 
recirc_id(0),in_port(100),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(src=192.1
 ])
 
 AT_CHECK([grep -e '|ofproto_dpif_xlate|WARN|' ovs-vswitchd.log | sed 
"s/^.*|WARN|//"], [0], [dnl
-stack underflow while processing 
icmp,in_port=LOCAL,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0
 on bridge br1
+stack underflow on bridge br1 while processing 
icmp,in_port=LOCAL,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0
 ])
 
 OVS_VSWITCHD_STOP(["/stack underflow/d"])
-- 
2.16.1

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

Re: [ovs-dev] [PATCH] utilities: Install ovs-tcp{dump, undump} also when only Python3 is enabled

[Ben Pfaff]

On Tue, Aug 07, 2018 at 08:08:32PM +0200, Timothy Redaelli wrote:
> On Sat, 4 Aug 2018 19:19:53 -0700
> Ben Pfaff  wrote:
> [...]  
> > 
> > Thanks for the explanation.  I applied this to master; let me know if
> > you want backports.
> 
> I'd like a backport on branch-2.10, since commit 793bdb6c0500 and
> commit 227abb77d3d1 are included in the branch.

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

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Added logging for when sending out an in port

[Ben Pfaff]

On Tue, Aug 07, 2018 at 10:37:35AM -0700, Zak Whittington wrote:
> VMware-BZ: 2158607
> Signed-off-by: Zak Whittington 
> ---
>  ofproto/ofproto-dpif-xlate.c | 24 
>  1 file changed, 24 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 01f1faf..9b36536 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5007,6 +5007,18 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t 
> port,
>  if (port != ctx->xin->flow.in_port.ofp_port) {
>  compose_output_action(ctx, port, NULL, is_last_action, truncate);
>  } else {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +if (!VLOG_DROP_INFO()) {
> +struct ds s = DS_EMPTY_INITIALIZER;
> +ds_put_cstr(,
> +"Skipping output to input port while processing: ");
> +flow_format(, >base_flow, NULL);
> +ds_put_format(, " on bridge %s", ctx->xbridge->name);
> +VLOG_INFO("%s", ds_cstr());
> +ds_destroy();
> +}
> +
> +/* For when tracing only: */

Thanks for v2!  I have a few remaining comments.

I suggest putting the flow at the very end of the log message: because
it is long, it is hard for the reader to spot anything that come after
it.  (I see that there's a case in this file where we do it the same way
you did.  I'll send a fix for that in a minute.)

I suggest only emitting a message if we have a packet, that is, if
ctx->xin->packet is nonnull.  Otherwise, we'll emit this message even
when revalidating, which is likely to confuse readers.

The two cases look the same, so I'd add a helper.  Maybe we should have
an xlate_report_info() function similar to xlate_report_error() but with
a lower log level.

Thanks,

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

Re: [ovs-dev] [PATCH] utilities: Install ovs-tcp{dump, undump} also when only Python3 is enabled

[Timothy Redaelli]

On Sat, 4 Aug 2018 19:19:53 -0700
Ben Pfaff  wrote:
[...]  
> 
> Thanks for the explanation.  I applied this to master; let me know if
> you want backports.

I'd like a backport on branch-2.10, since commit 793bdb6c0500 and
commit 227abb77d3d1 are included in the branch.

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

Re: [ovs-dev] [PATCH] rhel: Use openvswitch user in the logrotate configuration file

[Timothy Redaelli]

On Tue,  7 Aug 2018 16:18:13 +0300
Markos Chandras  wrote:

> The /var/log/openvswitch directory is owned by the openvswitch user
> but logrotate could be running as root or as another user. As a
> result of which, rpmlint prints the following warning when building
> the spec file on SUSE Linux Enterprise:
> 
> openvswitch.x86_64: W:
> suse-logrotate-user-writable-log-dir /var/log/openvswitch
> openvswitch:openvswitch 0750 The log directory is writable by
> unprivileged users. Please fix the permissions so only root can write
> there or add the 'su' option to your logrotate config
> 
> In order to fix that, we should run the logrotate script as the
> openvswitch user which ensures that the correct user is processing
> the Open vSwitch log files.
> 
> Cc: Aaron Conole 
> Cc: Timothy Redaelli 
> Signed-off-by: Markos Chandras 

Hi Markos,
I agree with you that running logrotate as root is probably bad.

The problem is that, for backward compatibility, we keep OVS as "root"
user if you upgrade OVS from an old version (older than the non-root
user support).

This means that, with this patch and when you launch OVS as root
(after an upgrade or by commenting the OVS_USER_ID
in /etc/sysconfig/openvswitch), the logs are owned by root:root and
so logrotate, as openvswitch:openvswitch, cannot work correctly.

If it's only to avoid the warning, we could change the spec file in
order to do "chown -R openvswitch:openvswitch /var/log/openvswitch",
when it's an upgrade, in %post, instead of using %attr in the spec file.

As alternative we may to do something more complex. For example by
generating another file in /var/run/openvswitch that contains the "su"
line (using OVS_USER_ID) and including it from
/etc/logrotate.d/openvswitch (we already use a similar approach to
set --ovs-user ONLY if OVS_USER_ID is not root, in
/var/run/openvswitch/useropts).

Probably there is a better way to do that, but I can't think of
anything else.

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

[ovs-dev] [PATCH] ofproto-dpif-xlate: Added logging for when sending out an in port

[Zak Whittington]

VMware-BZ: 2158607
Signed-off-by: Zak Whittington 
---
 ofproto/ofproto-dpif-xlate.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 01f1faf..9b36536 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5007,6 +5007,18 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t 
port,
 if (port != ctx->xin->flow.in_port.ofp_port) {
 compose_output_action(ctx, port, NULL, is_last_action, truncate);
 } else {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+if (!VLOG_DROP_INFO()) {
+struct ds s = DS_EMPTY_INITIALIZER;
+ds_put_cstr(,
+"Skipping output to input port while processing: ");
+flow_format(, >base_flow, NULL);
+ds_put_format(, " on bridge %s", ctx->xbridge->name);
+VLOG_INFO("%s", ds_cstr());
+ds_destroy();
+}
+
+/* For when tracing only: */
 xlate_report(ctx, OFT_WARN, "skipping output to input port");
 }
 break;
@@ -5092,6 +5104,18 @@ xlate_output_trunc_action(struct xlate_ctx *ctx,
 ctx->xout->slow |= SLOW_ACTION;
 }
 } else {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+if (!VLOG_DROP_INFO()) {
+struct ds s = DS_EMPTY_INITIALIZER;
+ds_put_cstr(,
+"Skipping output to input port while processing:");
+flow_format(, >base_flow, NULL);
+ds_put_format(, " on bridge %s", ctx->xbridge->name);
+VLOG_INFO("%s", ds_cstr());
+ds_destroy();
+}
+
+/* For when tracing only: */
 xlate_report(ctx, OFT_WARN, "skipping output to input port");
 }
 break;
-- 
2.7.4

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

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Darrell Ball]

On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian  wrote:

> > In its current implementation dp_packet_shift() is also unaware of multi-
> > seg mbufs (that holds data in memory non-contiguously) and assumes that
> > data exists contiguously in memory, memmove'ing data to perform the
> shift.
> >
> > To add support for multi-seg mbuds a new set of functions was introduced,
> > dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These functions are
> > used by dp_packet_shift(), when handling multi-seg mbufs, to shift and
> > write data within a chain of mbufs.
> >
>
> Hi Tiago,
>
> After applying this patch I see the following error during compilation.
>
> lib/conntrack.c: In function 'handle_ftp_ctl':
> lib/conntrack.c:3154:11: error: 'addr_offset_from_ftp_data_start' may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
>  char *pkt_addr_str = ftp_data_start + addr_offset_from_ftp_data_
> start;
>^~~~
> lib/conntrack.c:3173:12: note: 'addr_offset_from_ftp_data_start' was
> declared here
>  size_t addr_offset_from_ftp_data_start;
>
> It can be fixed with the following incremental.
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 974f985..7cd1fc9 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -3170,7 +3170,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
>  struct ip_header *l3_hdr = dp_packet_l3(pkt);
>  ovs_be32 v4_addr_rep = 0;
>  struct ct_addr v6_addr_rep;
> -size_t addr_offset_from_ftp_data_start;
> +size_t addr_offset_from_ftp_data_start = 0;
>  size_t addr_size = 0;
>  char *ftp_data_start;
>  bool do_seq_skew_adj = true;
>
> If there are no objections I can roll this into the patch upon application
> to dpdk merge.
>
> Ian
>


hmm, I test with 4 major versions of GCC (4-7) with different flags,
including O3 and I don't see these errors.
I use 6.4 for the '6' major version

What flags are you using ?

I am building some versions from source; are you doing the same /

Is it possible that your GCC 6.3.1 was not built correctly ?




>
> > Signed-off-by: Tiago Lam 
> > Acked-by: Eelco Chaudron 
> > ---
> >  lib/dp-packet.c | 97
> > +
> >  lib/dp-packet.h | 10 ++
> >  2 files changed, 107 insertions(+)
> >
> > diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 2aaeaae..d6e19eb
> > 100644
> > --- a/lib/dp-packet.c
> > +++ b/lib/dp-packet.c
> > @@ -294,6 +294,97 @@ dp_packet_prealloc_headroom(struct dp_packet *b,
> > size_t size)
> >  }
> >  }
> >
> > +#ifdef DPDK_NETDEV
> > +/* Write len data bytes in a mbuf at specified offset.
> > + *
> > + * 'mbuf', pointer to the destination mbuf where 'ofs' is, and the mbuf
> > +where
> > + * the data will first be written.
> > + * 'ofs', the offset within the provided 'mbuf' where 'data' is to be
> > written.
> > + * 'len', the size of the to be written 'data'.
> > + * 'data', pointer to the to be written bytes.
> > + *
> > + * XXX: This function is the counterpart of the `rte_pktmbuf_read()`
> > +function
> > + * available with DPDK, in the rte_mbuf.h */ void
> > +dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len,
> > + const void *data)
> > +{
> > +char *dst_addr;
> > +uint16_t data_len;
> > +int len_copy;
> > +while (mbuf) {
> > +if (len == 0) {
> > +break;
> > +}
> > +
> > +dst_addr = rte_pktmbuf_mtod_offset(mbuf, char *, ofs);
> > +data_len = MBUF_BUF_END(mbuf->buf_addr, mbuf->buf_len) -
> > + dst_addr;
> > +
> > +len_copy = MIN(len, data_len);
> > +/* We don't know if 'data' is the result of a rte_pktmbuf_read()
> > call,
> > + * in which case we may end up writing to the same region of
> > memory we
> > + * are reading from and overlapping. Hence the use of memmove()
> > here */
> > +memmove(dst_addr, data, len_copy);
> > +
> > +data = ((char *) data) + len_copy;
> > +len -= len_copy;
> > +ofs = 0;
> > +
> > +mbuf = mbuf->next;
> > +}
> > +}
> > +
> > +static void
> > +dp_packet_mbuf_shift_(struct rte_mbuf *dbuf, int16_t dst_ofs,
> > +  const struct rte_mbuf *sbuf, uint16_t src_ofs,
> > +int len) {
> > +char rd[len];
> > +const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd);
> > +
> > +ovs_assert(wd);
> > +
> > +dp_packet_mbuf_write(dbuf, dst_ofs, len, wd); }
> > +
> > +/* Similarly to dp_packet_shift(), shifts the data within the mbufs of
> > +a
> > + * dp_packet of DPBUF_DPDK source by 'delta' bytes.
> > + * Caller must make sure of the following conditions:
> > + * - When shifting left, delta can't be bigger than the data_len
> > available in
> > + *   the last mbuf;
> > + * - When shifting right, delta can't be bigger than the space available
> > in the
> > + *   first mbuf (buf_len - data_off).
> > + * Both these conditions guarantee that a shift 

Re: [ovs-dev] [PATCH v5 08/14] dp-packet: Handle multi-seg mbufs in resize__().

[Darrell Ball]

On Tue, Jul 24, 2018 at 12:36 AM, Lam, Tiago  wrote:

> On 23/07/2018 23:48, Darrell Ball wrote:
> >
> >
> > On Fri, Jul 20, 2018 at 10:10 AM, Lam, Tiago  > > wrote:
> >
> > On 19/07/2018 00:02, Darrell Ball wrote:
> > >
> > >
> > > On Tue, Jul 17, 2018 at 12:59 AM, Lam, Tiago  
> > > >> wrote:
> > >
> > > On 16/07/2018 09:37, Lam, Tiago wrote:
> > > > On 13/07/2018 18:54, Darrell Ball wrote:
> > > >> Thanks for the patch.
> > > >>
> > > >> A few queries inline.
> > > >>
> > > >
> > > > Hi Darrell,
> > > >
> > > > Thanks for your inputs. I've replied in-line as well.
> > > >
> > > >> On Wed, Jul 11, 2018 at 11:23 AM, Tiago Lam <
> tiago@intel.com 
> > > >
> > > >> 
> >  > > >>
> > > >> When enabled with DPDK OvS relies on mbufs allocated by
> > > mempools to
> > > >> receive and output data on DPDK ports. Until now, each
> OvS
> > > dp_packet has
> > > >> had only one mbuf associated, which is allocated with
> > the maximum
> > > >> possible size, taking the MTU into account. This
> approach,
> > > however,
> > > >> doesn't allow us to increase the allocated size in an
> mbuf,
> > > if needed,
> > > >> since an mbuf is allocated and initialised upon mempool
> > > creation. Thus,
> > > >> in the current implementatin this is dealt with by
> calling
> > > >> OVS_NOT_REACHED() and terminating OvS.
> > > >>
> > > >> To avoid this, and allow the (already) allocated space
> > to be
> > > better
> > > >> used, dp_packet_resize__() now tries to use the
> available
> > > room, both the
> > > >> tailroom and the headroom, to make enough space for the
> new
> > > data. Since
> > > >> this happens for packets of source DPBUF_DPDK, the
> > > single-segment mbuf
> > > >> case mentioned above is also covered by this new
> aproach in
> > > resize__().
> > > >>
> > > >> Signed-off-by: Tiago Lam  > 
> > > >
> > > >>  >   >  > > >> Acked-by: Eelco Chaudron  echau...@redhat.com>
> > > >
> > > >>  >   >  > > >> ---
> > > >>  lib/dp-packet.c | 48
> > > ++--
> > > >>  1 file changed, 46 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> > > >> index d6e19eb..87af459 100644
> > > >> --- a/lib/dp-packet.c
> > > >> +++ b/lib/dp-packet.c
> > > >> @@ -237,9 +237,51 @@ dp_packet_resize__(struct
> > dp_packet *b,
> > > size_t
> > > >> new_headroom, size_t new_tailroom
> > > >>  new_allocated = new_headroom + dp_packet_size(b) +
> > > new_tailroom;
> > > >>
> > > >>  switch (b->source) {
> > > >> +/* When resizing mbufs, both a single mbuf and
> > multi-segment
> > > >> mbufs (where
> > > >> + * data is not contigously held in memory), both
> the
> > > headroom
> > > >> and the
> > > >> + * tailroom available will be used to make more
> space
> > > for where
> > > >> data needs
> > > >> + * to be inserted. I.e if there's not enough
> headroom,
> > > data may
> > > >> be shifted
> > > >> + * right if there's enough tailroom.
> > > >> + * However, this is not bulletproof and in some
> cases
> > > the space
> > > >> available
> > > >> + * won't be enough - in those cases, an error
> > should be
> > > >> returned and the
> > > >> + * packet dropped. */
> > > >>  case DPBUF_DPDK:
> > > >> -OVS_NOT_REACHED();
> > > >>
> > > >>
> > > >> Previously, it was a coding error to call this function for
> a
> > > 

[ovs-dev] [PATCH] rhel: Install the network scripts in a new subpackage

[Timothy Redaelli]

Starting from Fedora 29, the legacy network scripts are installed in
the "network-scripts" package and so the network scripts ("ifup-ovs",
"ifdown-ovs") should be installed only when the "network-scripts" package
is installed.

This commit introduces (on Fedora 29+) a new subpackage
(network-scripts-openvswitch). This subpackage is installed, by default, only
if the "network-scripts" package is installed too (reverse weak dependency).

Reported-by: Lubomir Rintel 
Reported-at: https://src.fedoraproject.org/rpms/openvswitch/pull-request/4
Signed-off-by: Timothy Redaelli 
---
 rhel/openvswitch-fedora.spec.in | 20 
 1 file changed, 20 insertions(+)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 9f8664e95..eead10069 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -162,6 +162,18 @@ Provides: openvswitch-static = %{version}-%{release}
 This provides static library, libopenswitch.a and the openvswitch header
 files needed to build an external application.
 
+%if 0%{?rhel} > 7 || 0%{?fedora} > 28
+%package -n network-scripts-%{name}
+Summary: Open vSwitch legacy network service support
+License: ASL 2.0
+Requires: network-scripts
+Supplements: (%{name} and network-scripts)
+
+%description -n network-scripts-%{name}
+This provides the ifup and ifdown scripts for use with the legacy network
+service.
+%endif
+
 %package ovn-central
 Summary: Open vSwitch - Open Virtual Network support
 License: ASL 2.0
@@ -529,6 +541,12 @@ fi
 %{_includedir}/openflow/*
 %{_includedir}/ovn/*
 
+%if 0%{?rhel} > 7 || 0%{?fedora} > 28
+%files -n network-scripts-%{name}
+%{_sysconfdir}/sysconfig/network-scripts/ifup-ovs
+%{_sysconfdir}/sysconfig/network-scripts/ifdown-ovs
+%endif
+
 %files
 %defattr(-,openvswitch,openvswitch)
 %dir %{_sysconfdir}/openvswitch
@@ -546,8 +564,10 @@ fi
 %{_unitdir}/ovs-vswitchd.service
 %{_unitdir}/ovs-delete-transient-ports.service
 %{_datadir}/openvswitch/scripts/openvswitch.init
+%if ! (0%{?rhel} > 7 || 0%{?fedora} > 28)
 %{_sysconfdir}/sysconfig/network-scripts/ifup-ovs
 %{_sysconfdir}/sysconfig/network-scripts/ifdown-ovs
+%endif
 %{_datadir}/openvswitch/bugtool-plugins/
 %{_datadir}/openvswitch/scripts/ovs-bugtool-*
 %{_datadir}/openvswitch/scripts/ovs-check-dead-ifs
-- 
2.17.1

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

[ovs-dev] [PATCH v5 6/6] Documentation: OVN RBAC and IPsec tutorial

[Qiuyu Xiao]

This patch adds step-by-step guide for configuring OVN Role-Based Access
Control and IPsec.

Signed-off-by: Qiuyu Xiao 
---
 Documentation/automake.mk |   2 +
 Documentation/index.rst   |   4 +-
 Documentation/tutorials/index.rst |   2 +
 Documentation/tutorials/ovn-ipsec.rst | 147 ++
 Documentation/tutorials/ovn-rbac.rst  | 134 +++
 5 files changed, 288 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/tutorials/ovn-ipsec.rst
 create mode 100644 Documentation/tutorials/ovn-rbac.rst

diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index 5401b9bad..082438e09 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -29,6 +29,8 @@ DOC_SOURCE = \
Documentation/tutorials/ovn-sandbox.rst \
Documentation/tutorials/ovs-conntrack.rst \
Documentation/tutorials/ipsec.rst \
+   Documentation/tutorials/ovn-ipsec.rst \
+   Documentation/tutorials/ovn-rbac.rst \
Documentation/topics/index.rst \
Documentation/topics/bonding.rst \
Documentation/topics/idl-compound-indexes.rst \
diff --git a/Documentation/index.rst b/Documentation/index.rst
index bab5ba1f1..46261235c 100644
--- a/Documentation/index.rst
+++ b/Documentation/index.rst
@@ -66,7 +66,9 @@ vSwitch? Start here.
   :doc:`tutorials/ovn-sandbox` |
   :doc:`tutorials/ovn-openstack` |
   :doc:`tutorials/ovs-conntrack` |
-  :doc:`tutorials/ipsec`
+  :doc:`tutorials/ipsec` |
+  :doc:`tutorials/ovn-ipsec` |
+  :doc:`tutorials/ovn-rbac`
 
 Deeper Dive
 ---
diff --git a/Documentation/tutorials/index.rst 
b/Documentation/tutorials/index.rst
index b481090a0..35340ee56 100644
--- a/Documentation/tutorials/index.rst
+++ b/Documentation/tutorials/index.rst
@@ -44,4 +44,6 @@ vSwitch.
ovs-advanced
ovn-sandbox
ovn-openstack
+   ovn-rbac
+   ovn-ipsec
ovs-conntrack
diff --git a/Documentation/tutorials/ovn-ipsec.rst 
b/Documentation/tutorials/ovn-ipsec.rst
new file mode 100644
index 0..db3d5bc43
--- /dev/null
+++ b/Documentation/tutorials/ovn-ipsec.rst
@@ -0,0 +1,147 @@
+..
+  Licensed under the Apache License, Version 2.0 (the "License"); you may
+  not use this file except in compliance with the License. You may obtain
+  a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+  WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+  License for the specific language governing permissions and limitations
+  under the License.
+
+  Convention for heading levels in Open vSwitch documentation:
+
+  ===  Heading 0 (reserved for the title in a document)
+  ---  Heading 1
+  ~~~  Heading 2
+  +++  Heading 3
+  '''  Heading 4
+
+  Avoid deeper levels because they do not render well.
+
+==
+OVN IPsec Tutorial
+==
+
+This document provides a step-by-step guide for encrypting tunnel traffic with
+IPsec in Open Virtual Network (OVN). OVN tunnel traffic is transported by
+physical routers and switches. These physical devices could be untrusted
+(devices in public network) or might be compromised.  Enabling IPsec encryption
+for the tunnel traffic can prevent the traffic data from being monitored and
+manipulated. More details about the OVN IPsec design can be found in
+``ovn-architecture``\(7) manpage.
+
+This document assumes OVN is installed in your system and runs normally. Also,
+you need to install OVS IPsec packages in each chassis (refer to
+:ref:`install-ovs-ipsec`).
+
+Generating Certificates and Keys
+
+
+OVN chassis uses CA-signed certificate to authenticate peer chassis for
+building IPsec tunnel. If you have enabled Role-Based Access Control (RBAC) in
+OVN, you can use the RBAC SSL certificates and keys to set up OVN IPsec. Or you
+can generate separate certificates and keys with ``ovs-pki`` (refer to
+:ref:`gen-certs-keys`).
+
+.. note::
+
+   OVN IPsec requires x.509 version 3 certificate with the subjectAltName DNS
+   field setting the same string as the common name (CN) field. CN should be
+   set as the chassis name.  ``ovs-pki`` in Open vSwitch 2.10.90 and later
+   generates such certificates.  Please generate compatible certificates if you
+   use another PKI tool, or an older version of ``ovs-pki``, to manage
+   certificates.
+
+Configuring OVN IPsec
+-
+
+You need to install the CA certificate, chassis certificate and private key in
+each chassis. Use the following command::
+
+$ ovs-vsctl set Open_vSwitch . \
+other_config:certificate=/path/to/chassis-cert.pem \
+other_config:private_key=/path/to/chassis-privkey.pem \
+other_config:ca_cert=/path/to/cacert.pem
+

[ovs-dev] [PATCH v5 5/6] OVN: native support for tunnel encryption

[Qiuyu Xiao]

This patch adds IPsec support for OVN tunnel. Basically, OVN offers a
binary option to its user for encryption configuration. If the IPsec
option is turned on, all tunnels will be encrypted. Otherwise, no tunnel
will be encrypted.

The changes are summarized as below:
1) Added a ipsec column on the NB_Global table and SB_Global table. The
value of ipsec column is propagated by ovn-northd from NB_Global to
SB_Global.

2) ovn-controller monitors the ipsec column in SB_Global. If the ipsec
value is true, ovn-controller sets options of the tunnel interface by
specifying "options:remote_name=". If the ipsec
value is false, ovn-controller removes these options.

3) ovs-monitor-ipsec daemon
(https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348701.html)
monitors the tunnel interface options and configures IKE daemon
accordingly for IPsec encryption.

Signed-off-by: Qiuyu Xiao 
---
 ovn/controller/encaps.c | 31 ++
 ovn/controller/encaps.h |  7 +-
 ovn/controller/ovn-controller.c |  4 +++-
 ovn/northd/ovn-northd.c |  8 +--
 ovn/ovn-architecture.7.xml  | 39 +
 ovn/ovn-nb.ovsschema|  7 +++---
 ovn/ovn-nb.xml  |  6 +
 ovn/ovn-sb.ovsschema|  7 +++---
 ovn/ovn-sb.xml  |  6 +
 9 files changed, 101 insertions(+), 14 deletions(-)

diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index fde017586..2169920ba 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -79,8 +79,9 @@ tunnel_create_name(struct tunnel_ctx *tc, const char 
*chassis_id)
 }
 
 static void
-tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id,
-   const struct sbrec_encap *encap)
+tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
+   const char *new_chassis_id, const struct sbrec_encap *encap,
+   const char *local_ip)
 {
 struct smap options = SMAP_INITIALIZER();
 smap_add(, "remote_ip", encap->ip);
@@ -90,6 +91,16 @@ tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id,
 smap_add(, "csum", csum);
 }
 
+/* Add auth info if ipsec is enabled. */
+if (sbg->ipsec) {
+smap_add(, "remote_name", new_chassis_id);
+if (local_ip) {
+smap_add(, "local_ip", local_ip);
+} else {
+VLOG_INFO("Need to specify encap ip for IPsec tunnels.");
+}
+}
+
 /* If there's an existing chassis record that does not need any change,
  * keep it.  Otherwise, create a new record (if there was an existing
  * record, the new record will supplant it and encaps_run() will delete
@@ -157,7 +168,9 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_bridge_table *bridge_table,
const struct ovsrec_bridge *br_int,
const struct sbrec_chassis_table *chassis_table,
-   const char *chassis_id)
+   const char *chassis_id,
+   const struct sbrec_sb_global *sbg,
+   const struct ovsrec_open_vswitch_table *ovs_table)
 {
 if (!ovs_idl_txn || !br_int) {
 return;
@@ -201,6 +214,16 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 }
 }
 
+/* Get IP address of local chassis. */
+const char *chassis_ip;
+const struct ovsrec_open_vswitch *cfg;
+cfg = ovsrec_open_vswitch_table_first(ovs_table);
+if (cfg) {
+chassis_ip = smap_get(>external_ids, "ovn-encap-ip");
+} else {
+chassis_ip = NULL;
+}
+
 SBREC_CHASSIS_TABLE_FOR_EACH (chassis_rec, chassis_table) {
 if (strcmp(chassis_rec->name, chassis_id)) {
 /* Create tunnels to the other chassis. */
@@ -209,7 +232,7 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 VLOG_INFO("No supported encaps for '%s'", chassis_rec->name);
 continue;
 }
-tunnel_add(, chassis_rec->name, encap);
+tunnel_add(, sbg, chassis_rec->name, encap, chassis_ip);
 }
 }
 
diff --git a/ovn/controller/encaps.h b/ovn/controller/encaps.h
index 054bdfa78..680b62df7 100644
--- a/ovn/controller/encaps.h
+++ b/ovn/controller/encaps.h
@@ -23,13 +23,18 @@ struct ovsdb_idl_txn;
 struct ovsrec_bridge;
 struct ovsrec_bridge_table;
 struct sbrec_chassis_table;
+struct sbrec_sb_global;
+struct ovsrec_open_vswitch_table;
 
 void encaps_register_ovs_idl(struct ovsdb_idl *);
 void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 const struct ovsrec_bridge_table *,
 const struct ovsrec_bridge *br_int,
 const struct sbrec_chassis_table *,
-const char *chassis_id);
+const char *chassis_id,
+const struct sbrec_sb_global *,
+const struct ovsrec_open_vswitch_table *);
+
 bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
 const struct ovsrec_bridge *br_int);
 
diff --git 

[ovs-dev] [PATCH v5 4/6] Documentation: IPsec tunnel tutorial and documentation.

[Qiuyu Xiao]

tutorials/index.rst gives a step-by-setp guide to set up OVS IPsec
tunnel.

tutorials/ipsec.rst gives detailed explanation on the IPsec tunnel
configuration methods and forwarding modes.

Signed-off-by: Qiuyu Xiao 
Signed-off-by: Ansis Atteka 
Co-authored-by: Ansis Atteka 
---
 Documentation/automake.mk |   2 +
 Documentation/howto/index.rst |   1 +
 Documentation/howto/ipsec.rst | 204 +
 Documentation/index.rst   |   3 +-
 Documentation/tutorials/index.rst |   1 +
 Documentation/tutorials/ipsec.rst | 353 ++
 vswitchd/vswitch.xml  | 157 -
 7 files changed, 711 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/howto/ipsec.rst
 create mode 100644 Documentation/tutorials/ipsec.rst

diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index 244479490..5401b9bad 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -28,6 +28,7 @@ DOC_SOURCE = \
Documentation/tutorials/ovn-openstack.rst \
Documentation/tutorials/ovn-sandbox.rst \
Documentation/tutorials/ovs-conntrack.rst \
+   Documentation/tutorials/ipsec.rst \
Documentation/topics/index.rst \
Documentation/topics/bonding.rst \
Documentation/topics/idl-compound-indexes.rst \
@@ -60,6 +61,7 @@ DOC_SOURCE = \
Documentation/howto/docker.rst \
Documentation/howto/dpdk.rst \
Documentation/howto/firewalld.rst \
+   Documentation/howto/ipsec.rst \
Documentation/howto/kvm.rst \
Documentation/howto/libvirt.rst \
Documentation/howto/selinux.rst \
diff --git a/Documentation/howto/index.rst b/Documentation/howto/index.rst
index 201d6936b..9a3487be3 100644
--- a/Documentation/howto/index.rst
+++ b/Documentation/howto/index.rst
@@ -37,6 +37,7 @@ OVS
:maxdepth: 2
 
kvm
+   ipsec
selinux
libvirt
ssl
diff --git a/Documentation/howto/ipsec.rst b/Documentation/howto/ipsec.rst
new file mode 100644
index 0..d3fb190be
--- /dev/null
+++ b/Documentation/howto/ipsec.rst
@@ -0,0 +1,204 @@
+..
+  Licensed under the Apache License, Version 2.0 (the "License"); you may
+  not use this file except in compliance with the License. You may obtain
+  a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+  WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+  License for the specific language governing permissions and limitations
+  under the License.
+
+  Convention for heading levels in Open vSwitch documentation:
+
+  ===  Heading 0 (reserved for the title in a document)
+  ---  Heading 1
+  ~~~  Heading 2
+  +++  Heading 3
+  '''  Heading 4
+
+  Avoid deeper levels because they do not render well.
+
+===
+Encrypt Open vSwitch Tunnels with IPsec
+===
+
+This document gives detailed description on the OVS IPsec tunnel and its
+configuration modes.  If you want to follow a step-by-step guide to run and
+test IPsec tunnel, please refer to :doc:`/tutorials/ipsec`.
+
+Overview
+
+
+Why do encryption?
+~~
+
+OVS tunnel packets are transported from one machine to another. Along the path,
+the packets are processed by physical routers and physical switches.  There are
+risks that these physical devices might read or write the contents of the
+tunnel packets. IPsec encrypts IP payload and prevents the malicious party
+sniffing or manipulating the tunnel traffic.
+
+OVS IPsec
+~
+
+OVS IPsec aims to provide a simple interface for user to add encryption on OVS
+tunnels. It supports GRE, GENEVE, VXLAN, and STT tunnel. The IPsec
+configuration is done by setting options of the tunnel interface and
+other_config of Open_vSwitch. You can choose different authentication methods
+and plaintext tunnel policies based on your requirements.
+
+OVS does not currently provide any support for IPsec encryption for traffic not
+encapsulated in a tunnel.
+
+Configuration
+-
+
+Authentication Methods
+~~
+
+Hosts of the IPsec tunnel need to authenticate each other to build a secure
+channel. There are three authentication methods:
+
+1) You can use a pre-shared key (PSK) to do authentication. In both hosts, set
+   the same PSK value. This PSK is like your password. You should never reveal
+   it to untrusted parties. This method is easier to use but less secure than
+   the certificate-based methods::
+
+  $ ovs-vsctl add-port br0 ipsec_gre0 -- \
+  set interface ipsec_gre0 type=gre \
+ options:local_ip=1.1.1.1 \
+ options:remote_ip=2.2.2.2 \
+ 

[ovs-dev] [PATCH v5 3/6] debian and rhel: Create IPsec package.

[Qiuyu Xiao]

Added rules and files to create debian and rpm ovs-ipsec packages.

Signed-off-by: Qiuyu Xiao 
Signed-off-by: Ansis Atteka 
Co-authored-by: Ansis Atteka 
---
 debian/automake.mk|   3 +
 debian/control|  21 ++
 debian/openvswitch-ipsec.dirs |   1 +
 debian/openvswitch-ipsec.init | 181 ++
 debian/openvswitch-ipsec.install  |   1 +
 rhel/automake.mk  |   1 +
 rhel/openvswitch-fedora.spec.in   |  19 +-
 ...b_systemd_system_openvswitch-ipsec.service |  12 ++
 utilities/ovs-ctl.in  |  18 ++
 9 files changed, 256 insertions(+), 1 deletion(-)
 create mode 100644 debian/openvswitch-ipsec.dirs
 create mode 100644 debian/openvswitch-ipsec.init
 create mode 100644 debian/openvswitch-ipsec.install
 create mode 100644 rhel/usr_lib_systemd_system_openvswitch-ipsec.service

diff --git a/debian/automake.mk b/debian/automake.mk
index 4d8e204bb..8a8d43c9f 100644
--- a/debian/automake.mk
+++ b/debian/automake.mk
@@ -20,6 +20,9 @@ EXTRA_DIST += \
debian/openvswitch-datapath-source.copyright \
debian/openvswitch-datapath-source.dirs \
debian/openvswitch-datapath-source.install \
+   debian/openvswitch-ipsec.dirs \
+   debian/openvswitch-ipsec.init \
+   debian/openvswitch-ipsec.install \
debian/openvswitch-pki.dirs \
debian/openvswitch-pki.postinst \
debian/openvswitch-pki.postrm \
diff --git a/debian/control b/debian/control
index 9ae248f27..cde93f20e 100644
--- a/debian/control
+++ b/debian/control
@@ -322,3 +322,24 @@ Description: Open vSwitch development package
  1000V.
  .
  This package provides openvswitch headers and libopenvswitch for developers.
+
+Package: openvswitch-ipsec
+Architecture: linux-any
+Depends: iproute2,
+ openvswitch-common (= ${binary:Version}),
+ openvswitch-switch (= ${binary:Version}),
+ python,
+ python-openvswitch (= ${source:Version}),
+ strongswan,
+ ${misc:Depends},
+ ${shlibs:Depends}
+Description: Open vSwitch IPsec tunneling support
+ Open vSwitch is a production quality, multilayer, software-based,
+ Ethernet virtual switch. It is designed to enable massive network
+ automation through programmatic extension, while still supporting
+ standard management interfaces and protocols (e.g. NetFlow, IPFIX,
+ sFlow, SPAN, RSPAN, CLI, LACP, 802.1ag). In addition, it is designed
+ to support distribution across multiple physical servers similar to
+ VMware's vNetwork distributed vswitch or Cisco's Nexus 1000V.
+ .
+ This package provides IPsec tunneling support for OVS tunnels.
diff --git a/debian/openvswitch-ipsec.dirs b/debian/openvswitch-ipsec.dirs
new file mode 100644
index 0..fca44aa7b
--- /dev/null
+++ b/debian/openvswitch-ipsec.dirs
@@ -0,0 +1 @@
+usr/share/openvswitch/scripts
\ No newline at end of file
diff --git a/debian/openvswitch-ipsec.init b/debian/openvswitch-ipsec.init
new file mode 100644
index 0..8488beccf
--- /dev/null
+++ b/debian/openvswitch-ipsec.init
@@ -0,0 +1,181 @@
+#!/bin/sh
+#
+# Copyright (c) 2007, 2009 Javier Fernandez-Sanguino 
+#
+# This is free software; you may redistribute it and/or modify
+# it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2,
+# or (at your option) any later version.
+#
+# This is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License with
+# the Debian operating system, in /usr/share/common-licenses/GPL;  if
+# not, write to the Free Software Foundation, Inc., 59 Temple Place,
+# Suite 330, Boston, MA 02111-1307 USA
+#
+### BEGIN INIT INFO
+# Provides:  openvswitch-ipsec
+# Required-Start:$network $local_fs $remote_fs openvswitch-switch
+# Required-Stop: $remote_fs
+# Default-Start: 2 3 4 5
+# Default-Stop:  0 1 6
+# Short-Description: Open vSwitch GRE-over-IPsec daemon
+# Description:   The ovs-monitor-ipsec script provides support for
+#encrypting GRE tunnels with IPsec.
+### END INIT INFO
+
+PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
+
+DAEMON=/usr/share/openvswitch/scripts/ovs-monitor-ipsec # Daemon's location
+NAME=ovs-monitor-ipsec  # Introduce the short server's name here
+LOGDIR=/var/log/openvswitch # Log directory to use
+DATADIR=/usr/share/openvswitch
+
+PIDFILE=/var/run/openvswitch/$NAME.pid
+
+test -x $DAEMON || exit 0
+
+. /lib/lsb/init-functions
+
+DODTIME=10  # Time to wait for the server to die, in seconds
+# If this value is set too low you might not
+# let some servers 

[ovs-dev] [PATCH v5 1/6] datapath: add transport ports in route lookup for geneve

[Qiuyu Xiao]

This patch adds transport ports information for route lookup so that
IPsec can select geneve tunnel traffic to do encryption.

Signed-off-by: Qiuyu Xiao 
Reviewed-by: Greg Rose 
Tested-by: Greg Rose 
---
 datapath/linux/compat/geneve.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c
index 435a23fb7..95a665ddd 100644
--- a/datapath/linux/compat/geneve.c
+++ b/datapath/linux/compat/geneve.c
@@ -836,7 +836,8 @@ free_dst:
 static struct rtable *geneve_get_v4_rt(struct sk_buff *skb,
   struct net_device *dev,
   struct flowi4 *fl4,
-  struct ip_tunnel_info *info)
+  struct ip_tunnel_info *info,
+  __be16 dport, __be16 sport)
 {
bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
struct geneve_dev *geneve = netdev_priv(dev);
@@ -850,6 +851,8 @@ static struct rtable *geneve_get_v4_rt(struct sk_buff *skb,
memset(fl4, 0, sizeof(*fl4));
fl4->flowi4_mark = skb->mark;
fl4->flowi4_proto = IPPROTO_UDP;
+   fl4->fl4_dport = dport;
+   fl4->fl4_sport = sport;
 
if (info) {
fl4->daddr = info->key.u.ipv4.dst;
@@ -895,7 +898,8 @@ static struct rtable *geneve_get_v4_rt(struct sk_buff *skb,
 static struct dst_entry *geneve_get_v6_dst(struct sk_buff *skb,
   struct net_device *dev,
   struct flowi6 *fl6,
-  struct ip_tunnel_info *info)
+  struct ip_tunnel_info *info,
+ __be16 dport, __be16 sport)
 {
bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
struct geneve_dev *geneve = netdev_priv(dev);
@@ -911,6 +915,8 @@ static struct dst_entry *geneve_get_v6_dst(struct sk_buff 
*skb,
memset(fl6, 0, sizeof(*fl6));
fl6->flowi6_mark = skb->mark;
fl6->flowi6_proto = IPPROTO_UDP;
+   fl6->fl6_dport = dport;
+   fl6->fl6_sport = sport;
 
if (info) {
fl6->daddr = info->key.u.ipv6.dst;
@@ -1005,13 +1011,13 @@ static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, 
struct net_device *dev,
goto tx_error;
}
 
-   rt = geneve_get_v4_rt(skb, dev, , info);
+   sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
+   rt = geneve_get_v4_rt(skb, dev, , info, geneve->dst_port, sport);
if (IS_ERR(rt)) {
err = PTR_ERR(rt);
goto tx_error;
}
 
-   sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
skb_reset_mac_header(skb);
 
iip = ip_hdr(skb);
@@ -1097,13 +1103,13 @@ static netdev_tx_t geneve6_xmit_skb(struct sk_buff 
*skb, struct net_device *dev,
}
}
 
-   dst = geneve_get_v6_dst(skb, dev, , info);
+   sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
+   dst = geneve_get_v6_dst(skb, dev, , info, geneve->dst_port, sport);
if (IS_ERR(dst)) {
err = PTR_ERR(dst);
goto tx_error;
}
 
-   sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
skb_reset_mac_header(skb);
 
iip = ip_hdr(skb);
@@ -1232,13 +1238,17 @@ int ovs_geneve_fill_metadata_dst(struct net_device 
*dev, struct sk_buff *skb)
struct geneve_dev *geneve = netdev_priv(dev);
struct rtable *rt;
struct flowi4 fl4;
+   __be16 sport;
 #if IS_ENABLED(CONFIG_IPV6)
struct dst_entry *dst;
struct flowi6 fl6;
 #endif
 
+   sport = udp_flow_src_port(geneve->net, skb,
+1, USHRT_MAX, true);
+
if (ip_tunnel_info_af(info) == AF_INET) {
-   rt = geneve_get_v4_rt(skb, dev, , info);
+   rt = geneve_get_v4_rt(skb, dev, , info, geneve->dst_port, 
sport);
if (IS_ERR(rt))
return PTR_ERR(rt);
 
@@ -1246,7 +1256,7 @@ int ovs_geneve_fill_metadata_dst(struct net_device *dev, 
struct sk_buff *skb)
info->key.u.ipv4.src = fl4.saddr;
 #if IS_ENABLED(CONFIG_IPV6)
} else if (ip_tunnel_info_af(info) == AF_INET6) {
-   dst = geneve_get_v6_dst(skb, dev, , info);
+   dst = geneve_get_v6_dst(skb, dev, , info, geneve->dst_port, 
sport);
if (IS_ERR(dst))
return PTR_ERR(dst);
 
@@ -1257,8 +1267,7 @@ int ovs_geneve_fill_metadata_dst(struct net_device *dev, 
struct sk_buff *skb)
return -EINVAL;
}
 
-   info->key.tp_src = udp_flow_src_port(geneve->net, skb,
-1, USHRT_MAX, true);
+   info->key.tp_src = sport;

[ovs-dev] [PATCH v5 0/6] IPsec support for tunneling

[Qiuyu Xiao]

This patch series reintroduce IPsec support for OVS tunneling and enable OVN to
use IPsec tunnels. GRE, VXLAN, GENEVE, and STT IPsec tunnels are supported.
StrongSwan and LibreSwan IKE daemons are supported.

Changes from v1 to v2
-
1. Merge the ovs-monitor-ipsec code to a single patch. Add LibreSwan IKE
daemon support.
2. Add ovs-monitor-ipsec to flake8 check.
3. Use openssl to extract CN from certificate so that users don't need to
specify the CN information in the configuration interface.
4. Improve documentations as suggested.

Changes from v2 to v3
-
1. Add scripts and rules to create ovs-ipsec RPM package.
2. Add Documentation/tutorials/ipsec.rst which gives a step-by-step OVS IPsec
tutorial. Modify Documentation/howto/ipsec.rst which gives a detailed
description on OVS IPsec configuration modes.
3. Modify ovs-pki to generate x.509 version 3 certificate when do self-sign.
4. IPsec tunnel interface needs 'local_ip' information. Modify ovn-controller
to add 'local_ip' when IPsec is enabled.
5. Add a section on ovn/ovn-architecture.7.xml to introduce ovn IPsec.

Changes from v3 to v4
-
1. Split the datapath patch to three patches (geneve, vxlan, stt).
2. Add tutorial for OVN RBAC and OVN IPsec.

Changes from v4 to v5
-
1. Fix coding style issues in ovs-monitor-ipsec.
2. Improve IPsec and OVN-IPsec tutorials as suggested.
3. Change the description of setting skb_mark in documentation to reflect the
real situation.

Qiuyu Xiao (6):
  datapath: add transport ports in route lookup for geneve
  ipsec: reintroduce IPsec support for tunneling
  debian and rhel: Create IPsec package.
  Documentation: IPsec tunnel tutorial and documentation.
  OVN: native support for tunnel encryption
  Documentation: OVN RBAC and IPsec tutorial

 Documentation/automake.mk |4 +
 Documentation/howto/index.rst |1 +
 Documentation/howto/ipsec.rst |  204 +++
 Documentation/index.rst   |5 +-
 Documentation/tutorials/index.rst |3 +
 Documentation/tutorials/ipsec.rst |  353 +
 Documentation/tutorials/ovn-ipsec.rst |  147 +++
 Documentation/tutorials/ovn-rbac.rst  |  134 ++
 Makefile.am   |1 +
 datapath/linux/compat/geneve.c|   29 +-
 debian/automake.mk|3 +
 debian/control|   21 +
 debian/openvswitch-ipsec.dirs |1 +
 debian/openvswitch-ipsec.init |  181 +++
 debian/openvswitch-ipsec.install  |1 +
 ipsec/automake.mk |   10 +
 ipsec/ovs-monitor-ipsec   | 1173 +
 ovn/controller/encaps.c   |   31 +-
 ovn/controller/encaps.h   |7 +-
 ovn/controller/ovn-controller.c   |4 +-
 ovn/northd/ovn-northd.c   |8 +-
 ovn/ovn-architecture.7.xml|   39 +
 ovn/ovn-nb.ovsschema  |7 +-
 ovn/ovn-nb.xml|6 +
 ovn/ovn-sb.ovsschema  |7 +-
 ovn/ovn-sb.xml|6 +
 rhel/automake.mk  |1 +
 rhel/openvswitch-fedora.spec.in   |   19 +-
 ...b_systemd_system_openvswitch-ipsec.service |   12 +
 utilities/ovs-ctl.in  |   18 +
 vswitchd/vswitch.xml  |  157 ++-
 31 files changed, 2558 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/howto/ipsec.rst
 create mode 100644 Documentation/tutorials/ipsec.rst
 create mode 100644 Documentation/tutorials/ovn-ipsec.rst
 create mode 100644 Documentation/tutorials/ovn-rbac.rst
 create mode 100644 debian/openvswitch-ipsec.dirs
 create mode 100644 debian/openvswitch-ipsec.init
 create mode 100644 debian/openvswitch-ipsec.install
 create mode 100644 ipsec/automake.mk
 create mode 100755 ipsec/ovs-monitor-ipsec
 create mode 100644 rhel/usr_lib_systemd_system_openvswitch-ipsec.service

-- 
2.18.0

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

Re: [ovs-dev] [PATCH v5 08/14] dp-packet: Handle multi-seg mbufs in resize__().

[Darrell Ball]

On Tue, Jul 24, 2018 at 12:06 AM, Lam, Tiago  wrote:

> On 23/07/2018 23:55, Darrell Ball wrote:
> >
> >
> > On Fri, Jul 20, 2018 at 10:09 AM, Lam, Tiago  > > wrote:
> >
> > On 18/07/2018 23:53, Darrell Ball wrote:
> > > sorry, several distractions delayed response.
> > >
> > > On Mon, Jul 16, 2018 at 1:37 AM, Lam, Tiago  
> > > >> wrote:
> > >
> > > On 13/07/2018 18:54, Darrell Ball wrote:
> > > > Thanks for the patch.
> > > >
> > > > A few queries inline.
> > > >
> > >
> > > Hi Darrell,
> > >
> > > Thanks for your inputs. I've replied in-line as well.
> > >
> > > > On Wed, Jul 11, 2018 at 11:23 AM, Tiago Lam <
> tiago@intel.com 
> > >
> > > > 
> >  > > >
> > > > When enabled with DPDK OvS relies on mbufs allocated by
> mempools to
> > > > receive and output data on DPDK ports. Until now, each
> OvS dp_packet has
> > > > had only one mbuf associated, which is allocated with
> the maximum
> > > > possible size, taking the MTU into account. This
> approach, however,
> > > > doesn't allow us to increase the allocated size in an
> mbuf, if needed,
> > > > since an mbuf is allocated and initialised upon mempool
> creation. Thus,
> > > > in the current implementatin this is dealt with by
> calling
> > > > OVS_NOT_REACHED() and terminating OvS.
> > > >
> > > > To avoid this, and allow the (already) allocated space
> to be better
> > > > used, dp_packet_resize__() now tries to use the
> available room, both the
> > > > tailroom and the headroom, to make enough space for the
> new data. Since
> > > > this happens for packets of source DPBUF_DPDK, the
> single-segment mbuf
> > > > case mentioned above is also covered by this new aproach
> in resize__().
> > > >
> > > > Signed-off-by: Tiago Lam  tiago@intel.com>
> > >
> > > > 
> >  > > > Acked-by: Eelco Chaudron  echau...@redhat.com>
> > > >
> > > > 
> >  > > > ---
> > > >  lib/dp-packet.c | 48
> > > ++--
> > > >  1 file changed, 46 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> > > > index d6e19eb..87af459 100644
> > > > --- a/lib/dp-packet.c
> > > > +++ b/lib/dp-packet.c
> > > > @@ -237,9 +237,51 @@ dp_packet_resize__(struct dp_packet
> *b,
> > > size_t
> > > > new_headroom, size_t new_tailroom
> > > >  new_allocated = new_headroom + dp_packet_size(b) +
> > > new_tailroom;
> > > >
> > > >  switch (b->source) {
> > > > +/* When resizing mbufs, both a single mbuf and
> > multi-segment
> > > > mbufs (where
> > > > + * data is not contigously held in memory), both
> > the headroom
> > > > and the
> > > > + * tailroom available will be used to make more
> > space for
> > > where
> > > > data needs
> > > > + * to be inserted. I.e if there's not enough
> headroom,
> > > data may
> > > > be shifted
> > > > + * right if there's enough tailroom.
> > > > + * However, this is not bulletproof and in some
> > cases the
> > > space
> > > > available
> > > > + * won't be enough - in those cases, an error
> should be
> > > > returned and the
> > > > + * packet dropped. */
> > > >  case DPBUF_DPDK:
> > > > -OVS_NOT_REACHED();
> > > >
> > > >
> > > > Previously, it was a coding error to call this function for
> > a DPDK
> > > mbuf
> > > > case, which is pretty
> > > > clear. But with this patch, presumably that is not longer
> > the case and
> > > > the calling the API is
> > > > now ok for DPDK mbufs.
> > > >
> > >
> > > As it stands, 

Re: [ovs-dev] [PATCH] ofctl: Fixup compare_flows function

[Ben Pfaff]

Signed-off-by: Ben Pfaff 

On Tue, Aug 07, 2018 at 03:59:59PM +0300, aserd...@ovn.org wrote:
> Can you also provide a Signed-off-by pls?
> 
> > -Mesaj original-
> > De la: ovs-dev-boun...@openvswitch.org  > boun...@openvswitch.org> În numele Ben Pfaff
> > Trimis: Tuesday, August 7, 2018 12:40 AM
> > Către: Alin Gabriel Serdean 
> > Cc: d...@openvswitch.org
> > Subiect: Re: [ovs-dev] [PATCH] ofctl: Fixup compare_flows function
> > 
> > On Tue, Aug 07, 2018 at 12:34:45AM +0300, Alin Gabriel Serdean wrote:
> > > In the case there was no sorting criteria the flows on Windows were
> > > being rearranged because it was always returning zero.
> > >
> > > Also check if there we need sorting to save a few cycles.
> > >
> > > CC: Ben Pfaff 
> > > Co-authored-by: Ben Pfaff 
> > > Signed-off-by: Alin Gabriel Serdean 
> > 
> > Acked-by: Ben Pfaff 
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.

[Han Zhou]

On Tue, Aug 7, 2018 at 2:03 AM, Justin Pettit  wrote:
>
>
> > On Aug 6, 2018, at 1:27 PM, Han Zhou  wrote:
> >
> > Thanks Justin for the great work!!
> > Sorry that I didn't get time to review the series, just some quick
questions regarding the kernel bug you mentioned.
>
> Yes, I think you were on vacation, and I was running up against my own,
so it all went in pretty quickly.  I'm sorry I cut it so close to the 2.10
release date and couldn't have gotten some initial testing from you and
your team.
>
> > - What's the impact of having meter ID = 0? Does it mean the meters and
ACL rate limit feature can't be used at all, or is it just some limitations
in certain scenarios?
>
> In theory, it would mean that a single meter could be put in the kernel
(there is a layer of indirection in the mapping between the OpenFlow meter
id and the one chosen for the datapath).  However, my current plan is to
add a probe to OVS which just disables meters on those kernels with broken
meters, since otherwise it will just lead to confusion.  I suspect the
issue will be with kernels 4.15, 4.16, and 4.17.  (And hopefully some of
those kernels will be fixed as they do bug fix releases.)
>
> > - For the bug fix, can we get it reviewed (and probably merged) in
datapath/compact first here in the OVS community without having to wait for
kernel upstream? What's the usual practice for kernel module patches?
>
> The patch was committed without comment from David Miller:
>
>
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=25432eba9
>
> The backport patch is ready (I've appended it to the end of this
message), but I've been on vacation and I wanted to get the probe patch in
at the same time.  I plan to send both patches out Tuesday night.  However,
if you want to apply the patch at the bottom and give ACL rate-limiting a
try, I'd love to get some initial feedback.
>
> Thanks,
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-=-
>
> commit bc89eebb0e918d2e9a903d7e4a24ab1b5b522eab (HEAD -> meter-datapath)
> Author: Justin Pettit 
> Date:   Thu Jul 26 22:28:11 2018 -0700
>
> datapath: Fix setting meter id for new entries.
>
> Upstream commit:
>
> openvswitch: meter: Fix setting meter id for new entries
>
> The meter code would create an entry for each new meter.
However, it
> would not set the meter id in the new entry, so every meter would
appear
> to have a meter id of zero.  This commit properly sets the meter
id when
> adding the entry.
>
> Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
> Signed-off-by: Justin Pettit 
> Cc: Andy Zhou 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Justin Pettit 
>
> diff --git a/datapath/meter.c b/datapath/meter.c
> index 1c2ed4626c2a..281d86937555 100644
> --- a/datapath/meter.c
> +++ b/datapath/meter.c
> @@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr
**a)
> if (!meter)
> return ERR_PTR(-ENOMEM);
>
> +   meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
> meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
> meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
> meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
> @@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb,
struct genl_info *info)
> u32 meter_id;
> bool failed;
>
> +   if (!a[OVS_METER_ATTR_ID]) {
> +   return -ENODEV;
> +   }
> +
> meter = dp_meter_create(a);
> if (IS_ERR_OR_NULL(meter))
> return PTR_ERR(meter);
> @@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb,
struct genl_info *info)
> goto exit_unlock;
> }
>
> -   if (!a[OVS_METER_ATTR_ID]) {
> -   err = -ENODEV;
> -   goto exit_unlock;
> -   }
> -
> meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
>
> /* Cannot fail after this. */
>
>
>
>

Thanks Justin! This is very nice and we will try it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Stokes, Ian]

> Hi Ian,
> 
> On 07/08/2018 13:08, Stokes, Ian wrote:
> >> In its current implementation dp_packet_shift() is also unaware of
> >> multi- seg mbufs (that holds data in memory non-contiguously) and
> >> assumes that data exists contiguously in memory, memmove'ing data to
> perform the shift.
> >>
> >> To add support for multi-seg mbuds a new set of functions was
> >> introduced,
> >> dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These functions
> >> are used by dp_packet_shift(), when handling multi-seg mbufs, to
> >> shift and write data within a chain of mbufs.
> >>
> >
> > Hi Tiago,
> >
> > After applying this patch I see the following error during compilation.
> >
> > lib/conntrack.c: In function 'handle_ftp_ctl':
> > lib/conntrack.c:3154:11: error: 'addr_offset_from_ftp_data_start' may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
> >  char *pkt_addr_str = ftp_data_start +
> addr_offset_from_ftp_data_start;
> >^~~~
> > lib/conntrack.c:3173:12: note: 'addr_offset_from_ftp_data_start' was
> declared here
> >  size_t addr_offset_from_ftp_data_start;
> >
> > It can be fixed with the following incremental.
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c index 974f985..7cd1fc9
> > 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -3170,7 +3170,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> >  struct ip_header *l3_hdr = dp_packet_l3(pkt);
> >  ovs_be32 v4_addr_rep = 0;
> >  struct ct_addr v6_addr_rep;
> > -size_t addr_offset_from_ftp_data_start;
> > +size_t addr_offset_from_ftp_data_start = 0;
> >  size_t addr_size = 0;
> >  char *ftp_data_start;
> >  bool do_seq_skew_adj = true;
> >
> > If there are no objections I can roll this into the patch upon
> application to dpdk merge.
> >
> > Ian
> >
> 
> I see no problem in including the incremental (I also see Darrell is
> already CC'ed, since this is in the userspace Conntrack).
> 

Agreed, would be interested in Darrells input here also.

> I noticed this myself during testing (are you perhaps using GCC 5.4?), but
> seems to be a false positive as when I looked there shouldn't be a path
> where `addr_offset_from_ftp_data_start` is used uninitialized, and when
> trying to compile with GCC 4.8 and GCC 7.3 there's no such warning.
> 

I tested it with gcc (GCC) 6.3.1. 

> But I agree the incremental is a better practice.
> 
> Thanks,
> Tiago.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH RFC net-next] openvswitch: Queue upcalls to userspace in per-port round-robin order

[Stefano Brivio]

On Tue, 7 Aug 2018 15:31:11 +0200
Stefano Brivio  wrote:

> I would instead try to address the concerns that you had about the
> original patch adding fairness in the kernel, rather than trying to
> make the issue appear less severe in ovs-vswitchd.

And, by the way, if we introduce a way to configure the possible
fairness algorithms via netlink, we can also rather easily allow
ovs-vswitchd to disable fairness on kernel side altogether, should it
be needed.

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

Re: [ovs-dev] [PATCH RFC net-next] openvswitch: Queue upcalls to userspace in per-port round-robin order

[Stefano Brivio]

Hi Pravin,

On Tue, 31 Jul 2018 16:12:03 -0700
Pravin Shelar  wrote:

> Rather than reducing number of thread down to 1, we could find better
> number of FDs per port.
> How about this simple solution:
> 1. Allocate (N * P) FDs as long as it is under FD limit.
> 2. If FD limit (-EMFILE) is hit reduce N value by half and repeat step 1.

I still see a few quantitative issues with this approach, other than
Ben's observation about design (which, by the way, looks entirely
reasonable to me).

We're talking about a disproportionate amount of sockets in any case.
We can have up to 2^16 vports here, with 5k vports being rather common,
and for any reasonable value of N that manages somehow to perturbate the
distribution of upcalls per thread, we are talking about something well
in excess of 100k sockets. I think this doesn't really scale.

With the current value for N (3/4 * number of threads) this can even get
close to /proc/sys/fs/file-max on some systems, and there raising the
number of allowed file descriptors for ovs-vswitchd isn't a solution
anymore.

I would instead try to address the concerns that you had about the
original patch adding fairness in the kernel, rather than trying to
make the issue appear less severe in ovs-vswitchd.

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

[ovs-dev] [PATCH] rhel: Use openvswitch user in the logrotate configuration file

[Markos Chandras]

The /var/log/openvswitch directory is owned by the openvswitch user but
logrotate could be running as root or as another user. As a result of
which, rpmlint prints the following warning when building the spec file
on SUSE Linux Enterprise:

openvswitch.x86_64: W: suse-logrotate-user-writable-log-dir 
/var/log/openvswitch openvswitch:openvswitch 0750
The log directory is writable by unprivileged users. Please fix the
permissions so only root can write there or add the 'su' option
to your logrotate config

In order to fix that, we should run the logrotate script as the
openvswitch user which ensures that the correct user is processing
the Open vSwitch log files.

Cc: Aaron Conole 
Cc: Timothy Redaelli 
Signed-off-by: Markos Chandras 
---
 rhel/etc_logrotate.d_openvswitch | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rhel/etc_logrotate.d_openvswitch b/rhel/etc_logrotate.d_openvswitch
index ed7d733c9..eaf1fd5bf 100644
--- a/rhel/etc_logrotate.d_openvswitch
+++ b/rhel/etc_logrotate.d_openvswitch
@@ -6,6 +6,7 @@
 # without warranty of any kind.
 
 /var/log/openvswitch/*.log {
+su openvswitch openvswitch
 daily
 compress
 sharedscripts
-- 
2.16.4

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

Re: [ovs-dev] [PATCH] ofctl: Fixup compare_flows function

[aserdean]

Can you also provide a Signed-off-by pls?

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Ben Pfaff
> Trimis: Tuesday, August 7, 2018 12:40 AM
> Către: Alin Gabriel Serdean 
> Cc: d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] ofctl: Fixup compare_flows function
> 
> On Tue, Aug 07, 2018 at 12:34:45AM +0300, Alin Gabriel Serdean wrote:
> > In the case there was no sorting criteria the flows on Windows were
> > being rearranged because it was always returning zero.
> >
> > Also check if there we need sorting to save a few cycles.
> >
> > CC: Ben Pfaff 
> > Co-authored-by: Ben Pfaff 
> > Signed-off-by: Alin Gabriel Serdean 
> 
> Acked-by: Ben Pfaff 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Lam, Tiago]

Hi Ian,

On 07/08/2018 13:08, Stokes, Ian wrote:
>> In its current implementation dp_packet_shift() is also unaware of multi-
>> seg mbufs (that holds data in memory non-contiguously) and assumes that
>> data exists contiguously in memory, memmove'ing data to perform the shift.
>>
>> To add support for multi-seg mbuds a new set of functions was introduced,
>> dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These functions are
>> used by dp_packet_shift(), when handling multi-seg mbufs, to shift and
>> write data within a chain of mbufs.
>>
> 
> Hi Tiago,
> 
> After applying this patch I see the following error during compilation.
> 
> lib/conntrack.c: In function 'handle_ftp_ctl':
> lib/conntrack.c:3154:11: error: 'addr_offset_from_ftp_data_start' may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>  char *pkt_addr_str = ftp_data_start + addr_offset_from_ftp_data_start;
>^~~~
> lib/conntrack.c:3173:12: note: 'addr_offset_from_ftp_data_start' was declared 
> here
>  size_t addr_offset_from_ftp_data_start;
> 
> It can be fixed with the following incremental.
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 974f985..7cd1fc9 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -3170,7 +3170,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
>  struct ip_header *l3_hdr = dp_packet_l3(pkt);
>  ovs_be32 v4_addr_rep = 0;
>  struct ct_addr v6_addr_rep;
> -size_t addr_offset_from_ftp_data_start;
> +size_t addr_offset_from_ftp_data_start = 0;
>  size_t addr_size = 0;
>  char *ftp_data_start;
>  bool do_seq_skew_adj = true;
> 
> If there are no objections I can roll this into the patch upon application to 
> dpdk merge.
> 
> Ian
> 

I see no problem in including the incremental (I also see Darrell is
already CC'ed, since this is in the userspace Conntrack).

I noticed this myself during testing (are you perhaps using GCC 5.4?),
but seems to be a false positive as when I looked there shouldn't be a
path where `addr_offset_from_ftp_data_start` is used uninitialized, and
when trying to compile with GCC 4.8 and GCC 7.3 there's no such warning.

But I agree the incremental is a better practice.

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

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Stokes, Ian]

> In its current implementation dp_packet_shift() is also unaware of multi-
> seg mbufs (that holds data in memory non-contiguously) and assumes that
> data exists contiguously in memory, memmove'ing data to perform the shift.
> 
> To add support for multi-seg mbuds a new set of functions was introduced,
> dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These functions are
> used by dp_packet_shift(), when handling multi-seg mbufs, to shift and
> write data within a chain of mbufs.
> 

Hi Tiago,

After applying this patch I see the following error during compilation.

lib/conntrack.c: In function 'handle_ftp_ctl':
lib/conntrack.c:3154:11: error: 'addr_offset_from_ftp_data_start' may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
 char *pkt_addr_str = ftp_data_start + addr_offset_from_ftp_data_start;
   ^~~~
lib/conntrack.c:3173:12: note: 'addr_offset_from_ftp_data_start' was declared 
here
 size_t addr_offset_from_ftp_data_start;

It can be fixed with the following incremental.

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 974f985..7cd1fc9 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -3170,7 +3170,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct 
conn_lookup_ctx *ctx,
 struct ip_header *l3_hdr = dp_packet_l3(pkt);
 ovs_be32 v4_addr_rep = 0;
 struct ct_addr v6_addr_rep;
-size_t addr_offset_from_ftp_data_start;
+size_t addr_offset_from_ftp_data_start = 0;
 size_t addr_size = 0;
 char *ftp_data_start;
 bool do_seq_skew_adj = true;

If there are no objections I can roll this into the patch upon application to 
dpdk merge.

Ian

> Signed-off-by: Tiago Lam 
> Acked-by: Eelco Chaudron 
> ---
>  lib/dp-packet.c | 97
> +
>  lib/dp-packet.h | 10 ++
>  2 files changed, 107 insertions(+)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 2aaeaae..d6e19eb
> 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -294,6 +294,97 @@ dp_packet_prealloc_headroom(struct dp_packet *b,
> size_t size)
>  }
>  }
> 
> +#ifdef DPDK_NETDEV
> +/* Write len data bytes in a mbuf at specified offset.
> + *
> + * 'mbuf', pointer to the destination mbuf where 'ofs' is, and the mbuf
> +where
> + * the data will first be written.
> + * 'ofs', the offset within the provided 'mbuf' where 'data' is to be
> written.
> + * 'len', the size of the to be written 'data'.
> + * 'data', pointer to the to be written bytes.
> + *
> + * XXX: This function is the counterpart of the `rte_pktmbuf_read()`
> +function
> + * available with DPDK, in the rte_mbuf.h */ void
> +dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len,
> + const void *data)
> +{
> +char *dst_addr;
> +uint16_t data_len;
> +int len_copy;
> +while (mbuf) {
> +if (len == 0) {
> +break;
> +}
> +
> +dst_addr = rte_pktmbuf_mtod_offset(mbuf, char *, ofs);
> +data_len = MBUF_BUF_END(mbuf->buf_addr, mbuf->buf_len) -
> + dst_addr;
> +
> +len_copy = MIN(len, data_len);
> +/* We don't know if 'data' is the result of a rte_pktmbuf_read()
> call,
> + * in which case we may end up writing to the same region of
> memory we
> + * are reading from and overlapping. Hence the use of memmove()
> here */
> +memmove(dst_addr, data, len_copy);
> +
> +data = ((char *) data) + len_copy;
> +len -= len_copy;
> +ofs = 0;
> +
> +mbuf = mbuf->next;
> +}
> +}
> +
> +static void
> +dp_packet_mbuf_shift_(struct rte_mbuf *dbuf, int16_t dst_ofs,
> +  const struct rte_mbuf *sbuf, uint16_t src_ofs,
> +int len) {
> +char rd[len];
> +const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd);
> +
> +ovs_assert(wd);
> +
> +dp_packet_mbuf_write(dbuf, dst_ofs, len, wd); }
> +
> +/* Similarly to dp_packet_shift(), shifts the data within the mbufs of
> +a
> + * dp_packet of DPBUF_DPDK source by 'delta' bytes.
> + * Caller must make sure of the following conditions:
> + * - When shifting left, delta can't be bigger than the data_len
> available in
> + *   the last mbuf;
> + * - When shifting right, delta can't be bigger than the space available
> in the
> + *   first mbuf (buf_len - data_off).
> + * Both these conditions guarantee that a shift operation doesn't fall
> +outside
> + * the bounds of the existing mbufs, so that the first and last mbufs
> +(when
> + * using multi-segment mbufs), remain the same. */ static void
> +dp_packet_mbuf_shift(struct dp_packet *b, int delta) {
> +uint16_t src_ofs;
> +int16_t dst_ofs;
> +
> +struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, >mbuf);
> +struct rte_mbuf *tmbuf = rte_pktmbuf_lastseg(mbuf);
> +
> +if (delta < 0) {
> +ovs_assert(-delta <= tmbuf->data_len);
> +} else {
> +ovs_assert(delta < (mbuf->buf_len - mbuf->data_off));
> +}
> +
> +/* Set the 

Re: [ovs-dev] [PATCH 1/2] ovs python: ovs.stream.open_block() returns success even if the remote is unreachable

[Numan Siddique]

On Sat, Aug 4, 2018 at 3:20 AM Ben Pfaff  wrote:

> On Thu, Jul 12, 2018 at 01:37:33AM +0530, Numan Siddique wrote:
> > On Wed, Jul 11, 2018 at 10:26 PM Ben Pfaff  wrote:
> >
> > > On Wed, Jul 11, 2018 at 12:37:28PM +0530, Numan Siddique wrote:
> > > > On Wed, Jul 11, 2018 at 2:08 AM Ben Pfaff  wrote:
> > > >
> > > > > On Wed, Jul 11, 2018 at 12:56:39AM +0530, nusid...@redhat.com
> wrote:
> > > > > > From: Numan Siddique 
> > > > > >
> > > > > > The python function ovs.socket_util.check_connection_completion()
> > > uses
> > > > > select()
> > > > > > (provided by python) to monitor the socket file descriptor. The
> > > select()
> > > > > > returns 1 when the file descriptor becomes ready. For error cases
> > > like -
> > > > > > 111 (Connection refused) and 113 (No route to host) (POLLERR),
> > > > > ovs.poller._SelectSelect.poll()
> > > > > > expects the exceptfds list to be set by select(). But that is
> not the
> > > > > case.
> > > > > > As per the select() man page, writefds list will be set for
> POLLERR.
> > > > > > Please see "Correspondence between select() and poll()
> notifications"
> > > > > section of select(2)
> > > > > > man page.
> > > > > >
> > > > > > Because of this behavior,
> > > ovs.socket_util.check_connection_completion()
> > > > > returns success
> > > > > > even if the remote is unreachable or not listening on the port.
> > > > > >
> > > > > > This patch fixes this issue by adding a wrapper function -
> > > > > check_connection_completion_status()
> > > > > > which calls sock.connect_ex() to get the status of the
> connection if
> > > > > > ovs.socket_util.check_connection_completion() returns success.
> > > > > >
> > > > > > The test cases added fails without the fix in this patch.
> > > > > >
> > > > > > Signed-off-by: Numan Siddique 
> > > > >
> > > > > Can we just code check_connection_completion() like we do in C, by
> > > > > directly using select() on Windows and poll() everywhere else?  The
> > > > > approach in this patch seems a little indirect to me.
> > > > >
> > > > >
> > > > Thanks for the review. As per the comments here -
> > > >
> https://github.com/openvswitch/ovs/blob/master/python/ovs/poller.py#L51
> > > > *
> > > > # eventlet/gevent doesn't support select.poll. If select.poll is
> used,
> > > > # python interpreter is blocked as a whole instead of switching from
> the
> > > > # current thread that is about to block to other runnable thread.
> > > > # So emulate select.poll by select.select because using python means
> that
> > > > # performance isn't so important.
> > > > ***
> > > >
> > > > we cannot use poll() in python. I tried with this patch to test it
> out -
> > > > https://paste.fedoraproject.org/paste/YUhgVte-BOjgid-ojmHZJw
> > > > All the ovs python idl tests pass. But it doesn't work with openstack
> > > > networking-ovn. The whole neutron-server process just blocks.
> > > >
> > > > I don't see any other way for python. Once select() returns we have
> to
> > > use
> > > > some mechanism to get the error code.
> > > > Does calling sock.connect_ex() bother  you ?
> > >
> > > I don't mean to change what poller.py does.  I mean to implement
> > > ovs.socket_util.check_connection_completion in terms of select.poll and
> > > select.select directly.  The comment in python/ovs/poller.py should not
> > > be relevant because the use of select.poll in
> > > check_connection_completion would never block (it would use a timeout
> of
> > > 0).
> > >
> > >
> > Hi Ben,
> >
> > For python applications which use eventlet, select.poll/epoll is not
> > available. 'select ' python module
> > is monkey patched and poll and few other functions are deleted.
> >
> > https://github.com/eventlet/eventlet/blob/master/NEWS#L155
> >
> https://github.com/eventlet/eventlet/blob/master/eventlet/green/select.py#L9
> >
> https://github.com/eventlet/eventlet/blob/master/eventlet/patcher.py#L305
>
> It looks like it's possible to get the original function.  I think that
> it is a reasonable choice in this case:
> http://www.gevent.org/api/gevent.monkey.html#gevent.monkey.get_original


Thanks for pointing this out. I have submitted v4 as per your suggestion -
https://patchwork.ozlabs.org/project/openvswitch/list/?series=59666

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

[ovs-dev] [PATCH v4 2/2] python jsonrpc: Allow jsonrpc_session to have more than one remote.

[nusiddiq]

From: Numan Siddique 

Python IDL implementation doesn't have the support to connect to the
cluster dbs. This patch adds this support. We are still missing the
support in python idl class to connect to the cluster master. That
support will be added in an upcoming patch.

This patch is similar to the commit 8cf6bbb184 which added multiple remote
support in the C jsonrpc implementation.

Signed-off-by: Numan Siddique 
---
 python/ovs/db/idl.py  | 20 +++-
 python/ovs/jsonrpc.py | 39 ---
 tests/ovsdb-idl.at| 54 +++
 tests/test-ovsdb.py   | 13 ---
 4 files changed, 114 insertions(+), 12 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 64eb1a886..d5bd21535 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -101,6 +101,9 @@ class Idl(object):
 ovs.jsonrpc.session.open().  The connection will maintain an in-memory
 replica of the remote database.
 
+'remote' can be comma separated multiple remotes and each remote
+should be in a form acceptable to ovs.jsonrpc.session.open().
+
 'schema_helper' should be an instance of the SchemaHelper class which
 generates schema for the remote database. The caller may have cut it
 down by removing tables or columns that are not of interest.  The IDL
@@ -127,7 +130,8 @@ class Idl(object):
 self.tables = schema.tables
 self.readonly = schema.readonly
 self._db = schema
-self._session = ovs.jsonrpc.Session.open(remote,
+remotes = self._parse_remotes(remote)
+self._session = ovs.jsonrpc.Session.open_multiple(remotes,
 probe_interval=probe_interval)
 self._monitor_request_id = None
 self._last_seqno = None
@@ -155,6 +159,20 @@ class Idl(object):
 table.condition = [True]
 table.cond_changed = False
 
+def _parse_remotes(self, remote):
+# If remote is -
+# "tcp:10.0.0.1:6641,unix:/tmp/db.sock,t,s,tcp:10.0.0.2:6642"
+# this function returns
+# ["tcp:10.0.0.1:6641", "unix:/tmp/db.sock,t,s", tcp:10.0.0.2:6642"]
+remotes = []
+for r in remote.split(','):
+r_length = len(remotes)
+if r.find(":") == -1 and r_length:
+remotes[r_length - 1] = remotes[r_length - 1] + "," + r
+else:
+remotes.append(r)
+return remotes
+
 def index_create(self, table, name):
 """Create a named multi-column index on a table"""
 return self.tables[table].rows.index_create(name)
diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
index 7c24e574a..4873cff98 100644
--- a/python/ovs/jsonrpc.py
+++ b/python/ovs/jsonrpc.py
@@ -14,6 +14,7 @@
 import codecs
 import errno
 import os
+import random
 import sys
 
 import ovs.json
@@ -368,12 +369,17 @@ class Connection(object):
 class Session(object):
 """A JSON-RPC session with reconnection."""
 
-def __init__(self, reconnect, rpc):
+def __init__(self, reconnect, rpc, remotes):
 self.reconnect = reconnect
 self.rpc = rpc
 self.stream = None
 self.pstream = None
 self.seqno = 0
+if type(remotes) != list:
+remotes = [remotes]
+self.remotes = remotes
+random.shuffle(self.remotes)
+self.next_remote = 0
 
 @staticmethod
 def open(name, probe_interval=None):
@@ -393,28 +399,38 @@ class Session(object):
 feature. If non-zero the value will be forced to at least 1000
 milliseconds. If None it will just use the default value in OVS.
 """
+return Session.open_multiple([name], probe_interval=probe_interval)
+
+@staticmethod
+def open_multiple(remotes, probe_interval=None):
 reconnect = ovs.reconnect.Reconnect(ovs.timeval.msec())
-reconnect.set_name(name)
+session = Session(reconnect, None, remotes)
+session.pick_remote()
 reconnect.enable(ovs.timeval.msec())
-
-if ovs.stream.PassiveStream.is_valid_name(name):
+reconnect.set_backoff_free_tries(len(remotes))
+if ovs.stream.PassiveStream.is_valid_name(reconnect.get_name()):
 reconnect.set_passive(True, ovs.timeval.msec())
 
-if not ovs.stream.stream_or_pstream_needs_probes(name):
+if not ovs.stream.stream_or_pstream_needs_probes(reconnect.get_name()):
 reconnect.set_probe_interval(0)
 elif probe_interval is not None:
 reconnect.set_probe_interval(probe_interval)
 
-return Session(reconnect, None)
+return session
 
 @staticmethod
 def open_unreliably(jsonrpc):
 reconnect = ovs.reconnect.Reconnect(ovs.timeval.msec())
+session = Session(reconnect, None, [jsonrpc.name])
 reconnect.set_quiet(True)
-reconnect.set_name(jsonrpc.name)
+session.pick_remote()
 

[ovs-dev] [PATCH v4 0/2] Partial cluster support in Python IDL client

[nusiddiq]

From: Numan Siddique 

Python IDL library is lacking the functionality to connect to the
clustered db servers by providing multiple remotes (like -
"tcp:10.0.0.1:6641, tcp:10.0.0.2:6641, tcp:10.0.0.3:6641") in the
connection string.

This patch adds this functionality to the python idl library.
It still lacks the feature to connect to the master of the cluster.
To add this
  - python idl client should monitor and read the '_Server' schema
  - connect to the master of the cluster.

I will submit the patch once that is ready. But for now I think this
is good enough for the clients to connect to the cluster dbs.


v3 -> v4

p1 -> As per Ben's suggestion, used the select.poll() to 
know the connection status. In case eventlet/gevent is used and 
select.poll is monkey patched, then get the original select.poll()
using eventlet.patcher.original/gevent.monkey.get_original
functions.

v2 -> v3

Addressed the review comments from Ben to parse the remote in
db/idl.py

v1 -> v2

Deleted the debug code which I forgot to cleanup when sending v1.

Numan Siddique (2):
  ovs python: ovs.stream.open_block() returns success even if the remote
is unreachable
  python jsonrpc: Allow jsonrpc_session to have more than one remote.

 python/ovs/db/idl.py  | 20 ++-
 python/ovs/jsonrpc.py | 39 +-
 python/ovs/poller.py  | 34 +--
 python/ovs/socket_util.py |  6 ++--
 python/ovs/stream.py  | 11 --
 tests/automake.mk |  1 +
 tests/ovsdb-idl.at| 70 +++
 tests/test-ovsdb.py   | 13 ++--
 tests/test-stream.py  | 32 ++
 9 files changed, 208 insertions(+), 18 deletions(-)
 create mode 100644 tests/test-stream.py

-- 
2.17.1

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

[ovs-dev] [PATCH v4 1/2] ovs python: ovs.stream.open_block() returns success even if the remote is unreachable

[nusiddiq]

From: Numan Siddique 

The python function ovs.socket_util.check_connection_completion() uses select()
(provided by python) to monitor the socket file descriptor. The select()
returns 1 when the file descriptor becomes ready. For error cases like -
111 (Connection refused) and 113 (No route to host) (POLLERR), 
ovs.poller._SelectSelect.poll()
expects the exceptfds list to be set by select(). But that is not the case.
As per the select() man page, writefds list will be set for POLLERR.
Please see "Correspondence between select() and poll() notifications" section 
of select(2)
man page.

Because of this behavior, ovs.socket_util.check_connection_completion() returns 
success
even if the remote is unreachable or not listening on the port.

This patch fixes this issue by using poll() to check the connection status 
similar to
the C implementation of check_connection_completion().

A new function 'get_system_poll() is added in ovs/poller.py which returns the
select.poll() object. If select.poll is monkey patched by eventlet/gevent, it
gets the original select.poll() and returns it.

The test cases added in this patch fails without the fix.

Suggested-by: Ben Pfaff 
Signed-off-by: Numan Siddique 
---
 python/ovs/poller.py  | 34 --
 python/ovs/socket_util.py |  5 +++--
 python/ovs/stream.py  | 11 +--
 tests/automake.mk |  1 +
 tests/ovsdb-idl.at| 16 
 tests/test-stream.py  | 32 
 6 files changed, 93 insertions(+), 6 deletions(-)
 create mode 100644 tests/test-stream.py

diff --git a/python/ovs/poller.py b/python/ovs/poller.py
index 2f3fcf9b6..ef67e6763 100644
--- a/python/ovs/poller.py
+++ b/python/ovs/poller.py
@@ -31,14 +31,21 @@ except ImportError:
 SSL = None
 
 try:
-import eventlet.patcher
+from eventlet import patcher as eventlet_patcher
 
 def _using_eventlet_green_select():
-return eventlet.patcher.is_monkey_patched(select)
+return eventlet_patcher.is_monkey_patched(select)
 except:
+eventlet_patcher = None
 def _using_eventlet_green_select():
 return False
 
+try:
+from gevent import monkey as gevent_monkey
+except:
+gevent_monkey = None
+
+
 vlog = ovs.vlog.Vlog("poller")
 
 POLLIN = 0x001
@@ -257,3 +264,26 @@ class Poller(object):
 def __reset(self):
 self.poll = SelectPoll()
 self.timeout = -1
+
+
+"""
+Returns the original select.poll() object. If select.poll is monkey patched
+by eventlet or gevent library, it gets the original select.poll and returns
+an object of it using the eventlet.patcher.original/gevent.monkey.get_original
+functions.
+
+As a last resort, if there is any exception it returns the SelectPoll() object.
+"""
+def get_system_poll():
+try:
+if _using_eventlet_green_select():
+_system_poll = eventlet_patcher.original("select").poll
+elif gevent_monkey and gevent_monkey.is_object_patched(
+'select', 'poll'):
+_system_poll = gevent_monkey.get_original('select', 'poll')
+else:
+_system_poll = select.poll
+except:
+_system_poll = SelectPoll
+
+return _system_poll()
diff --git a/python/ovs/socket_util.py b/python/ovs/socket_util.py
index 403104936..8e582fe91 100644
--- a/python/ovs/socket_util.py
+++ b/python/ovs/socket_util.py
@@ -162,8 +162,8 @@ def make_unix_socket(style, nonblock, bind_path, 
connect_path, short=False):
 
 
 def check_connection_completion(sock):
-p = ovs.poller.SelectPoll()
 if sys.platform == "win32":
+p = ovs.poller.SelectPoll()
 event = winutils.get_new_event(None, False, True, None)
 # Receive notification of readiness for writing, of completed
 # connection or multipoint join operation, and of socket closure.
@@ -173,6 +173,7 @@ def check_connection_completion(sock):
  win32file.FD_CLOSE)
 p.register(event, ovs.poller.POLLOUT)
 else:
+p = ovs.poller.get_system_poll()
 p.register(sock, ovs.poller.POLLOUT)
 pfds = p.poll(0)
 if len(pfds) == 1:
@@ -180,7 +181,7 @@ def check_connection_completion(sock):
 if revents & ovs.poller.POLLERR:
 try:
 # The following should raise an exception.
-socket.send("\0", socket.MSG_DONTWAIT)
+sock.send("\0".encode(), socket.MSG_DONTWAIT)
 
 # (Here's where we end up if it didn't.)
 # XXX rate-limit
diff --git a/python/ovs/stream.py b/python/ovs/stream.py
index 5996497a5..ca0d84425 100644
--- a/python/ovs/stream.py
+++ b/python/ovs/stream.py
@@ -191,8 +191,15 @@ class Stream(object):
 if error:
 return error, None
 else:
-status = ovs.socket_util.check_connection_completion(sock)
-return 0, cls(sock, name, status)
+err = ovs.socket_util.check_connection_completion(sock)
+   

Re: [ovs-dev] [PATCH v2] stream-ssl: Define SSL_OP_NO_SSL_MASK for OpenSSL versions that lack it.

[Timothy Redaelli]

On Mon,  6 Aug 2018 15:39:44 -0700
Ben Pfaff  wrote:

> 10 of the travis builds are failing such as
> TESTSUITE=1 KERNEL=3.16.54 for gcc and clang.
> 
> Fixes: ab16d2c2871b ("stream-ssl: Don't enable new TLS versions by
> default") CC: Timothy Redaelli 
> Signed-off-by: Darrell Ball 
> Signed-off-by: Ben Pfaff 
> ---
> v1->v2: Add SSL_OP_NO_SSLv2 (thanks Han!).
> 
>  lib/stream-ssl.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index f3d623c035f8..fed71801b823 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -1188,6 +1188,12 @@ stream_ssl_set_protocols(const char *arg)
>  }
>  
>  /* Start with all the flags off and turn them on as requested. */
> +#ifndef SSL_OP_NO_SSL_MASK
> +/* For old OpenSSL without this macro, this is the correct
> value.  */ +#define SSL_OP_NO_SSL_MASK (SSL_OP_NO_SSLv2 |
> SSL_OP_NO_SSLv3 | \
> +SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1 | \
> +SSL_OP_NO_TLSv1_2)
> +#endif
>  long protocol_flags = SSL_OP_NO_SSL_MASK;
>  
>  char *s = xstrdup(arg);

I'm sorry for this, I tested my patch with OpenSSL 1.0.2k (RHEL7) and
OpenSSL 1.1.0h (Fedora 28).

I checked right now and some distributions (for example Ubuntu 14.04)
uses 1.0.1, that doesn't have SSL_OP_NO_SSL_MASK (introduced in 1.0.2,
6 years ago [1]).

Reviewed-by: Timothy Redaelli 

[1]
https://github.com/openssl/openssl/commit/49ef33fa3463d6b6001009024c9aed09f814cb7c#diff-4b59eddb1c722b1dc3d17b5f64149e12R620
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.

[Justin Pettit]

> On Aug 6, 2018, at 1:27 PM, Han Zhou  wrote:
> 
> Thanks Justin for the great work!!
> Sorry that I didn't get time to review the series, just some quick questions 
> regarding the kernel bug you mentioned.

Yes, I think you were on vacation, and I was running up against my own, so it 
all went in pretty quickly.  I'm sorry I cut it so close to the 2.10 release 
date and couldn't have gotten some initial testing from you and your team.

> - What's the impact of having meter ID = 0? Does it mean the meters and ACL 
> rate limit feature can't be used at all, or is it just some limitations in 
> certain scenarios?

In theory, it would mean that a single meter could be put in the kernel (there 
is a layer of indirection in the mapping between the OpenFlow meter id and the 
one chosen for the datapath).  However, my current plan is to add a probe to 
OVS which just disables meters on those kernels with broken meters, since 
otherwise it will just lead to confusion.  I suspect the issue will be with 
kernels 4.15, 4.16, and 4.17.  (And hopefully some of those kernels will be 
fixed as they do bug fix releases.)

> - For the bug fix, can we get it reviewed (and probably merged) in 
> datapath/compact first here in the OVS community without having to wait for 
> kernel upstream? What's the usual practice for kernel module patches?

The patch was committed without comment from David Miller:


https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=25432eba9

The backport patch is ready (I've appended it to the end of this message), but 
I've been on vacation and I wanted to get the probe patch in at the same time.  
I plan to send both patches out Tuesday night.  However, if you want to apply 
the patch at the bottom and give ACL rate-limiting a try, I'd love to get some 
initial feedback.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-=-

commit bc89eebb0e918d2e9a903d7e4a24ab1b5b522eab (HEAD -> meter-datapath)
Author: Justin Pettit 
Date:   Thu Jul 26 22:28:11 2018 -0700

datapath: Fix setting meter id for new entries.

Upstream commit:

openvswitch: meter: Fix setting meter id for new entries

The meter code would create an entry for each new meter.  However, it
would not set the meter id in the new entry, so every meter would appear
to have a meter id of zero.  This commit properly sets the meter id when
adding the entry.

Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
Signed-off-by: Justin Pettit 
Cc: Andy Zhou 
Signed-off-by: David S. Miller 

Signed-off-by: Justin Pettit 

diff --git a/datapath/meter.c b/datapath/meter.c
index 1c2ed4626c2a..281d86937555 100644
--- a/datapath/meter.c
+++ b/datapath/meter.c
@@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr **a)
if (!meter)
return ERR_PTR(-ENOMEM);
 
+   meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
@@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
u32 meter_id;
bool failed;
 
+   if (!a[OVS_METER_ATTR_ID]) {
+   return -ENODEV;
+   }
+
meter = dp_meter_create(a);
if (IS_ERR_OR_NULL(meter))
return PTR_ERR(meter);
@@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
goto exit_unlock;
}
 
-   if (!a[OVS_METER_ATTR_ID]) {
-   err = -ENODEV;
-   goto exit_unlock;
-   }
-
meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
 
/* Cannot fail after this. */




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