Looks good.
--Justin
On Feb 22, 2011, at 4:28 PM, Ben Pfaff wrote:
> The stricter validation requires updates to the calls to test-multipath
> to supply a valid n_links value. test-multipath doesn't actually use
> that value (it runs over different values in an internal "for" loop), so
> this
Looks good. Thanks for fixing Coverity issue #10712, too. :-)
--Justin
On Feb 22, 2011, at 4:28 PM, Ben Pfaff wrote:
> Some actions checked that 'arg' was nonnull before attempting to parse it
> but a lot of them didn't. This commit avoids the segfault by substituting
> an empty string when
Looks good.
--Justin
On Feb 22, 2011, at 4:28 PM, Ben Pfaff wrote:
> Multipath actions only support registers as destinations, but this was
> defined by reference in nicira-ext.h and the referenced text changed.
> ---
> include/openflow/nicira-ext.h |5 +++--
> 1 files changed, 3 insertions(
Coverity #10705
---
lib/ovsdb-idl.c |6 +-
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index df5aff5..e7f19e4 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -282,7 +282,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
assert(!idl->t
When an NXM wildcard entry that includes a multicast address is parsed,
it would fall through to the next case statement, which would also set
an inappropriate source mac address match.
Coverity #10717
---
lib/nx-match.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/li
Coverity #10722
---
lib/table.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/lib/table.c b/lib/table.c
index 86366d0..a503bcc 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -55,6 +55,7 @@ cell_to_text(struct cell *cell, const struct table_style
*style)
Coverity #10727
---
tests/test-ovsdb.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index aca68dc..40d8a8d 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1344,7 +1344,7 @@ static struct ovsdb_table *do_transact_tabl
Coverity complains that we're copying the unitialized "sin_zero" member
from "sin" into "r". I don't think this is an actual problem, but
there's no harm in zeroing out the structure, either.
Coverity #10916
---
lib/netdev-linux.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff
Coverity #10730
---
utilities/ovs-vsctl.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 9fca1c8..7ae45f5 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -2869,6 +2869,7 @@ cmd_find(struct vsctl_context
Here are some additional fixes for issues that Coverity flagged. With
this series, the remaining issues Coverity marks as "High Impact" that
appear to me to be real are addressed.* This also addresses most of the
issues that Coverity marks as "Medium Impact" that looked real to me,
but there are
Coverity #10726
---
ovsdb/ovsdb-tool.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index 4f55b6a..2754909 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -459,6 +459,7 @@ do_show_log(int argc OVS_UNUSED, char *argv[])
Looks good to me.
--Justin
On Feb 22, 2011, at 4:58 PM, Ben Pfaff wrote:
> Someone (I can't remember who) asked me about this a while ago. I hope
> this clarifies.
> ---
> utilities/ovs-vsctl.8.in |3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/utilities/ovs-vsc
Someone (I can't remember who) asked me about this a while ago. I hope
this clarifies.
---
utilities/ovs-vsctl.8.in |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 94e0c7c..c0143c7 100644
--- a/utilities/ovs-vsct
On Tue, Feb 22, 2011 at 03:48:32PM -0800, Andrew Evans wrote:
> On 2/22/11 2:47 PM, Ben Pfaff wrote:
> > Suggested-by: Andrew Evans
> > ---
> > acinclude.m4 | 26 ++
> > configure.ac |1 +
> > 2 files changed, 27 insertions(+), 0 deletions(-)
>
> Looks good to me. T
I'll do that.
On Tue, Feb 22, 2011 at 04:25:14PM -0800, Justin Pettit wrote:
> Thanks for doing this. The series looks good to me. Did you want to
> include the Coverity bug numbers that this addresses
> (10697,10696,10695,10694,10693,10692,10691,10690)?
>
> --Justin
>
>
> On Feb 22, 2011, at
The stricter validation requires updates to the calls to test-multipath
to supply a valid n_links value. test-multipath doesn't actually use
that value (it runs over different values in an internal "for" loop), so
this doesn't change any behavior.
Also adds a test to exercise each possible multip
Multipath actions only support registers as destinations, but this was
defined by reference in nicira-ext.h and the referenced text changed.
---
include/openflow/nicira-ext.h |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/openflow/nicira-ext.h b/include/openfl
Some actions checked that 'arg' was nonnull before attempting to parse it
but a lot of them didn't. This commit avoids the segfault by substituting
an empty string when no argument is given. It also updates a few of the
action implementations to correspond.
Reported-by: Reid Price
Bug #4462.
--
Thanks for doing this. The series looks good to me. Did you want to include
the Coverity bug numbers that this addresses
(10697,10696,10695,10694,10693,10692,10691,10690)?
--Justin
On Feb 22, 2011, at 11:01 AM, Ben Pfaff wrote:
> Static analyzers hate strncpy(). This new function shares it
On 2/22/11 2:47 PM, Ben Pfaff wrote:
> Suggested-by: Andrew Evans
> ---
> acinclude.m4 | 26 ++
> configure.ac |1 +
> 2 files changed, 27 insertions(+), 0 deletions(-)
Looks good to me. Thanks for implementing this.
-Andrew
___
Suggested-by: Andrew Evans
---
acinclude.m4 | 26 ++
configure.ac |1 +
2 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 6a829d5..e2bd2c6 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -306,3 +306,29 @@ AC_DEFUN([OVS_
On Tue, Feb 22, 2011 at 02:13:06PM -0800, Jesse Gross wrote:
> On Thu, Feb 17, 2011 at 10:44 AM, Ben Pfaff wrote:
> > It seems to me that vlan_deaccel_tag() needs to increase csum_start by
> > 4 in the CHECKSUM_PARTIAL case, something like this following. I
> > don't see anything like this in the
On Tue, Feb 22, 2011 at 02:33:13PM -0800, Jesse Gross wrote:
> On Fri, Feb 18, 2011 at 4:10 PM, Ben Pfaff wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index 940a581..dcff05f 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -709,6 +709,15 @@ static
On Fri, Feb 18, 2011 at 4:10 PM, Ben Pfaff wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 940a581..dcff05f 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -709,6 +709,15 @@ static int odp_packet_cmd_execute(struct sk_buff *skb,
> struct genl_info *info
On Thu, Feb 17, 2011 at 10:44 AM, Ben Pfaff wrote:
> It seems to me that vlan_deaccel_tag() needs to increase csum_start by
> 4 in the CHECKSUM_PARTIAL case, something like this following. I
> don't see anything like this in the other callers of __vlan_put_tag(),
> but it looks like most of those
Also adds a test and moves some code around in tests/ to make sure that
OFPROTO_START and OFPROTO_STOP are available in tests/ovs-ofctl.at.
Reported-by: Michael Mao
Bug #4566.
---
tests/automake.mk |1 +
tests/ofproto-macros.at | 16
tests/ofproto.at| 17 --
Only NXM supports 64-bit cookies, but this code didn't properly check
for that. This commit fixes the problem and makes the code much more
explicit about what it is checking.
This will hide bug #4566, but the following commit actually fixes it.
---
lib/ofp-util.c | 23 +--
Oh I didn't realize the connection. I don't mind not reviewing it.
Ethan
On Tue, Feb 22, 2011 at 11:09 AM, Ben Pfaff wrote:
> I was assuming that Justin would want to take a look, since it's
> essentially a counterproposal to some of his patches from the coverity
> series.
>
> On Tue, Feb 22, 2
I was assuming that Justin would want to take a look, since it's
essentially a counterproposal to some of his patches from the coverity
series.
On Tue, Feb 22, 2011 at 11:04:20AM -0800, Ethan Jackson wrote:
> I'll review this series.
>
> Ethan
>
> On Tue, Feb 22, 2011 at 11:01 AM, Ben Pfaff wro
I'll review this series.
Ethan
On Tue, Feb 22, 2011 at 11:01 AM, Ben Pfaff wrote:
> This series of patches improves the safety of string handling in
> Open vSwitch. I don't think that it actually fixes any real bugs,
> but it removes all references to strncpy() from the source code,
> which mak
Static analyzers hate strncpy(). This new function shares its property of
initializing an entire buffer, without its nasty habit of failing to
null-terminate long strings.
---
lib/automake.mk|1 +
lib/netdev-linux.c | 16
lib/socket-util.c |3 +--
lib/util.c
This series of patches improves the safety of string handling in
Open vSwitch. I don't think that it actually fixes any real bugs,
but it removes all references to strncpy() from the source code,
which makes people and static checkers happier.
Ben Pfaff (3):
string: Implement strnlen() if it is
The blind replacement of strncpy() by ovs_strlcpy() is risky because
strncpy() never reads more bytes from its source string than necessary to
write its destination string, but ovs_strlcpy() and the OpenBSD function
that inspired it both read the entire source string. This avoids that
problem.
Gi
---
configure.ac |2 +-
lib/{string.h => string.c} | 26 ++
lib/string.h |8 +++-
3 files changed, 18 insertions(+), 18 deletions(-)
copy lib/{string.h => string.c} (50%)
diff --git a/configure.ac b/configure.ac
index 28af2fe..47d
On Feb 22, 2011, at 10:01 AM, Ben Pfaff wrote:
> On Mon, Feb 21, 2011 at 05:44:57PM -0800, Justin Pettit wrote:
>> Coverity #10702
>
> Wow, another good catch.
>
> Thanks, this looks good.
I held back the two strncpy commits to think about further, and then pushed
with the changes I already me
On Mon, Feb 21, 2011 at 05:44:57PM -0800, Justin Pettit wrote:
> Coverity #10702
Wow, another good catch.
Thanks, this looks good.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:56PM -0800, Justin Pettit wrote:
> Coverity #10716
Ouch. Good catch, Coverity.
Thanks, this looks good.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:55PM -0800, Justin Pettit wrote:
> There's no indication that "date" is optional in the description of
> ovsdb_file_txn_from_json(), and the one caller always passes it in, so
> don't bother checking whether it exists.
>
> Coverity #10732
Looks good, thanks.
Coverit
On Tue, Feb 22, 2011 at 09:48:01AM -0800, Justin Pettit wrote:
> On Feb 22, 2011, at 9:43 AM, Ben Pfaff wrote:
>
> > On Mon, Feb 21, 2011 at 05:44:53PM -0800, Justin Pettit wrote:
> >> Coverity #10697,10696,10695,10694,10693,10692,10691,10690
> >
> > I'd prefer to use strncpy() here instead of ov
On Mon, Feb 21, 2011 at 05:44:54PM -0800, Justin Pettit wrote:
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1315,7 +1315,7 @@ ofp_print_ofpst_table_reply(struct ds *string, const
> struct ofp_header *oh,
>
> for (; n--; ts++) {
> char name[OFP_MAX_TABLE_NAME_LEN + 1];
> -
On Feb 22, 2011, at 9:43 AM, Ben Pfaff wrote:
> On Mon, Feb 21, 2011 at 05:44:53PM -0800, Justin Pettit wrote:
>> Coverity #10697,10696,10695,10694,10693,10692,10691,10690
>
> I'd prefer to use strncpy() here instead of ovs_strlcpy() because the
> former initializes every byte in the buffer. If
On Mon, Feb 21, 2011 at 05:44:53PM -0800, Justin Pettit wrote:
> Coverity #10697,10696,10695,10694,10693,10692,10691,10690
I'd prefer to use strncpy() here instead of ovs_strlcpy() because the
former initializes every byte in the buffer. If we don't do that then
I imagine we will get some complai
On Feb 22, 2011, at 9:37 AM, Ben Pfaff wrote:
> On Mon, Feb 21, 2011 at 05:44:51PM -0800, Justin Pettit wrote:
>> Coverity #10723
>
> This is the right idea but the correct fix is to use
> ovsdb_error_destroy().
Argh. I thought I had caught my errors doing that. Fixed.
--Justin
___
On Mon, Feb 21, 2011 at 05:44:52PM -0800, Justin Pettit wrote:
> Coverity #10721,10720
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:51PM -0800, Justin Pettit wrote:
> Coverity #10723
This is the right idea but the correct fix is to use
ovsdb_error_destroy().
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/
On Feb 22, 2011, at 9:30 AM, Ben Pfaff wrote:
> On Mon, Feb 21, 2011 at 05:44:45PM -0800, Justin Pettit wrote:
>> Coverity complains of a use after free error in hook_free(). Since this
>> is only printing the pointer address it should be fine, but at the very
>> least it looks odd.
>>
>> Coveri
On Mon, Feb 21, 2011 at 05:44:50PM -0800, Justin Pettit wrote:
> Coverity #10724
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:49PM -0800, Justin Pettit wrote:
> Coverity #10725
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:48PM -0800, Justin Pettit wrote:
> Coverity #10728
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:47PM -0800, Justin Pettit wrote:
> Coverity #10729
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Feb 22, 2011, at 7:36 AM, Ben Pfaff wrote:
> On Mon, Feb 21, 2011 at 11:57 PM, Justin Pettit wrote:
>> +import ovs.json
>
> Looks fine to me, thanks Justin and Giuseppe.
Thanks, I pushed it.
--Justin
___
dev mailing list
dev@openvswitch.org
ht
On Mon, Feb 21, 2011 at 05:44:46PM -0800, Justin Pettit wrote:
> Coverity #10731
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 05:44:45PM -0800, Justin Pettit wrote:
> Coverity complains of a use after free error in hook_free(). Since this
> is only printing the pointer address it should be fine, but at the very
> least it looks odd.
>
> Coverity #11069
Applying this patch will break the leak che
On Mon, Feb 21, 2011 at 05:44:44PM -0800, Justin Pettit wrote:
> Coverity #11066
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Mon, Feb 21, 2011 at 11:57 PM, Justin Pettit wrote:
> +import ovs.json
Looks fine to me, thanks Justin and Giuseppe.
--
"I don't normally do acked-by's. I think it's my way of avoiding
getting blamed when it all blows up." Andrew Morton
In my opinion, we should remove that requirement from OpenFlow that exact match
rules always have the highest priority. It sounds like it's already been
removed in OpenFlow 1.1. Unfortunately, we're kind of stuck in OpenFlow 1.0,
since I know controllers are written to expect that behavior.
-
56 matches
Mail list logo
