On Wed, Feb 23, 2011 at 8:14 PM, Ben Pfaff wrote:
> On Wed, Feb 23, 2011 at 8:07 PM, Jesse Gross wrote:
>> Currently we explicitly zero out each of the fields in the OVS_CB for
>> executed packets. However, it seems simpler and more robust to just
>> memset the whole thing to zero.
>>
>> Signed-
On Wed, Feb 23, 2011 at 8:07 PM, Jesse Gross wrote:
> Currently we explicitly zero out each of the fields in the OVS_CB for
> executed packets. However, it seems simpler and more robust to just
> memset the whole thing to zero.
>
> Signed-off-by: Jesse Gross
Acked-by: Ben Pfaff
I don't know w
On Tue, Feb 22, 2011 at 2:40 PM, Ben Pfaff wrote:
> 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
>> >
Currently we explicitly zero out each of the fields in the OVS_CB for
executed packets. However, it seems simpler and more robust to just
memset the whole thing to zero.
Signed-off-by: Jesse Gross
---
datapath/datapath.c |8 +---
1 files changed, 1 insertions(+), 7 deletions(-)
diff --
Thanks. I pushed this.
On Wed, Feb 23, 2011 at 03:54:15PM -0800, Ethan Jackson wrote:
> Looks Good.
>
> On Wed, Feb 23, 2011 at 3:43 PM, Ben Pfaff wrote:
> > exit(EXIT_FAILURE) will make a monitoring process (the one created by
> > --monitor) think that it should exit. But the most likely reas
Looks Good.
On Wed, Feb 23, 2011 at 3:43 PM, Ben Pfaff wrote:
> exit(EXIT_FAILURE) will make a monitoring process (the one created by
> --monitor) think that it should exit. But the most likely reason for
> out_of_memory() to be called is a bug: probably, the process is trying
> to allocate more
exit(EXIT_FAILURE) will make a monitoring process (the one created by
--monitor) think that it should exit. But the most likely reason for
out_of_memory() to be called is a bug: probably, the process is trying
to allocate more memory than there is available address space, e.g.
something like mallo
Thanks for all the reviews.
I ran "make distcheck" and then pushed this.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
Looks Fine.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> The return value isn't interesting here: it will always be 0.
>
> Coverity #10698.
> ---
> ovsdb/ovsdb.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
> index 2a54a7b..
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> Coverity #10699.
> ---
> tests/test-ovsdb.c | 6 --
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index aca68dc..3a34c09 100644
> --- a/tests/test-ovsdb.c
> +++
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> timeout() has no side effects so calling it without looking at the return
> value is pointless.
>
> Coverity #10700.
> ---
> lib/dhcp-client.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/lib/dhcp-cl
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> Coverity #10701.
> ---
> utilities/ovs-vsctl.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 6db4362..b9981aa 100644
> --- a/utilities/ovs-vsct
Looks Good assuming it make distchecks.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> This utility isn't used anywhere (except INSTALL.Linux), so remove it.
>
> Signed-off-by: Ben Pfaff
> Coverity #10708.
> ---
> INSTALL.Linux | 2 +-
> debian/openvswitch-switch.in
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> Coverity pointed out some inconsistencies on tests for whether columnp and
> keyp were nonnull. These tests were, at best, confusing, but in fact every
> caller always passed nonnull for both parameters, so this commit drops all
> o
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> This code was baffling and it confused Coverity, too.
>
> Coverity #11070.
> ---
> vswitchd/ovs-brcompatd.c | 45 -
> 1 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> A JSONRPC_REPLY message always have a nonnull 'id' member, as ensured by
> jsonrpc_msg_is_valid(). Checking for NULL here confused Coverity into
> believing that the call to ovsdb_idl_txn_process_reply() just below could
> cause a n
Looks Fine.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> At first glance the vconn_wait() call looks risky because this function
> checked whether rc->vconn is nonnull at the top. In fact it's OK because
> rc->state will be S_ACTIVE or S_IDLE only if rc->vconn is nonnull, but
> there's no
Seems fine to me. I don't quite understand what exactly we are
worried will break with this approach.
Ethan
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> I think that this will work OK, and it should avoid complaints from static
> checkers about using a freed pointer.
>
> Coverity #11069.
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> A negative size probably means that a system call failed. The caller could
> set that to 0 but we might as well just tolerate it in
> stream_report_content() by making the parameter type signed.
>
> Coverity #10718.
> ---
> lib/str
Looks Good.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> It is (very slightly) risky to open /dev/null every time that we need it,
> because open can fail. So this commit opens /dev/null in advance instead.
>
> Coverity #10719.
> ---
> lib/process.c | 24 ++--
> 1 f
Thanks, I pushed this one.
On Wed, Feb 23, 2011 at 01:34:36PM -0800, Ethan Jackson wrote:
> Ah oops, that's my bug. Thanks for fixing it, looks good. Go ahead and push.
>
> On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> > 'ea' here is a function parameter declared as an array, so "sizeof e
Ah oops, that's my bug. Thanks for fixing it, looks good. Go ahead and push.
On Wed, Feb 23, 2011 at 1:24 PM, Ben Pfaff wrote:
> 'ea' here is a function parameter declared as an array, so "sizeof ea" is
> sizeof(uint8_t *), which is either 4 or 8.
>
> Coverity #10689, 10735.
> ---
> vswitchd/br
Coverity #10701.
---
utilities/ovs-vsctl.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 6db4362..b9981aa 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -3126,7 +3126,9 @@ post_create(struct vsctl_
The return value isn't interesting here: it will always be 0.
Coverity #10698.
---
ovsdb/ovsdb.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index 2a54a7b..e76544e 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -123,7 +123,7 @@ static
This code was baffling and it confused Coverity, too.
Coverity #11070.
---
vswitchd/ovs-brcompatd.c | 45 -
1 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c
index fe953f4..6074462 100
It is (very slightly) risky to open /dev/null every time that we need it,
because open can fail. So this commit opens /dev/null in advance instead.
Coverity #10719.
---
lib/process.c | 24 ++--
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/lib/process.c b/
Coverity #10699.
---
tests/test-ovsdb.c |6 --
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index aca68dc..3a34c09 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2009, 2010 Nicira Netw
A JSONRPC_REPLY message always have a nonnull 'id' member, as ensured by
jsonrpc_msg_is_valid(). Checking for NULL here confused Coverity into
believing that the call to ovsdb_idl_txn_process_reply() just below could
cause a null pointer dereference, since ovsdb_idl_txn_process_reply() uses
the 'i
timeout() has no side effects so calling it without looking at the return
value is pointless.
Coverity #10700.
---
lib/dhcp-client.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/lib/dhcp-client.c b/lib/dhcp-client.c
index e59dc34..a0999bf 100644
--- a/lib/dhcp-client.c
I think that this will work OK, and it should avoid complaints from static
checkers about using a freed pointer.
Coverity #11069.
---
lib/leak-checker.c |3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/lib/leak-checker.c b/lib/leak-checker.c
index 8b78182..42b3818 100644
'ea' here is a function parameter declared as an array, so "sizeof ea" is
sizeof(uint8_t *), which is either 4 or 8.
Coverity #10689, 10735.
---
vswitchd/bridge.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index c2c4579..a448e05
I believe that this finishes out all of the Coverity-related
issues. Only the first 3 of these fix actual bugs, as far as
I can tell. The rest are nice (and generally minor) code
improvements.
Ben Pfaff (13):
bridge: Ethernet address is 6 bytes, not 4 or 8.
process: Avoid late failure if /de
This utility isn't used anywhere (except INSTALL.Linux), so remove it.
Signed-off-by: Ben Pfaff
Coverity #10708.
---
INSTALL.Linux |2 +-
debian/openvswitch-switch.install |1 -
debian/openvswitch-switch.manpages |1 -
utilities/.gitignore |2 -
A negative size probably means that a system call failed. The caller could
set that to 0 but we might as well just tolerate it in
stream_report_content() by making the parameter type signed.
Coverity #10718.
---
lib/stream.c |4 ++--
lib/stream.h |5 +++--
2 files changed, 5 insertions(+
At first glance the vconn_wait() call looks risky because this function
checked whether rc->vconn is nonnull at the top. In fact it's OK because
rc->state will be S_ACTIVE or S_IDLE only if rc->vconn is nonnull, but
there's no harm in putting that check inside the block that only runs if
rc->vconn
Coverity pointed out some inconsistencies on tests for whether columnp and
keyp were nonnull. These tests were, at best, confusing, but in fact every
caller always passed nonnull for both parameters, so this commit drops all
of those tests.
Coverity #10715, 10710.
---
utilities/ovs-vsctl.c | 6
On Feb 23, 2011, at 10:48 AM, Ben Pfaff wrote:
> On Wed, Feb 23, 2011 at 10:47:00AM -0800, Justin Pettit wrote:
>> ovsdb_txn_commit() may return a ovsdb_error structure, which should be
>> freed by the caller. The only remaining caller that discards the result
>> is in ovsdb_file_open__(), which
On Feb 23, 2011, at 9:30 AM, Ben Pfaff wrote:
> On Tue, Feb 22, 2011 at 10:18 PM, Justin Pettit wrote:
>> 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.* Th
On Feb 23, 2011, at 9:19 AM, Ben Pfaff wrote:
> On Tue, Feb 22, 2011 at 10:18:30PM -0800, Justin Pettit wrote:
>> Coverity #10727
>
> This looks good but we should also add WARN_UNUSED_RESULT to the
> prototype for ovsdb_txn_commit() in transaction.h.
I just sent a new patch that does that and f
On Feb 23, 2011, at 9:15 AM, Ben Pfaff wrote:
> On Tue, Feb 22, 2011 at 10:18:27PM -0800, Justin Pettit wrote:
>> 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
>> ---
On Wed, Feb 23, 2011 at 10:47:00AM -0800, Justin Pettit wrote:
> ovsdb_txn_commit() may return a ovsdb_error structure, which should be
> freed by the caller. The only remaining caller that discards the result
> is in ovsdb_file_open__(), which this fixes.
>
> Suggested-by: Ben Pfaff
Thanks, th
ovsdb_txn_commit() may return a ovsdb_error structure, which should be
freed by the caller. The only remaining caller that discards the result
is in ovsdb_file_open__(), which this fixes.
Suggested-by: Ben Pfaff
---
ovsdb/file.c|2 +-
ovsdb/transaction.h |3 ++-
2 files changed,
On Wed, Feb 23, 2011 at 04:06:01PM +0100, Gaetano Catalli wrote:
> I am reading the netdev_linux_cache_cb() function. In the 'else'
> branch, the cached values for all the netdev_devs need to be
> invalidated.
> To do so, you use the netdev_dev_get_devices() function to store in a
> temporary shash
Thanks. I added that to the log message and I'll push this series soon.
On Tue, Feb 22, 2011 at 11:03:33PM -0800, Justin Pettit wrote:
> Looks good. Thanks for fixing Coverity issue #10712, too. :-)
>
> --Justin
>
>
> On Feb 22, 2011, at 4:28 PM, Ben Pfaff wrote:
>
> > Some actions checked
On Wed, Feb 23, 2011 at 04:30:12PM +0100, Gaetano Catalli wrote:
> On Fri, Feb 11, 2011 at 6:09 PM, Ben Pfaff wrote:
> > On Fri, Feb 11, 2011 at 06:05:08PM +0100, Gaetano Catalli wrote:
> >> I have some doubts to resolve about switch local port.
> >> Firstly, in the ovs-openflowd manpage I can rea
On Tue, Feb 22, 2011 at 10:18 PM, Justin Pettit wrote:
> 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
On Tue, Feb 22, 2011 at 10:18:33PM -0800, Justin Pettit wrote:
> Coverity #10705
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Tue, Feb 22, 2011 at 10:18:32PM -0800, Justin Pettit wrote:
> 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
Ouch. Thank you, this
On Tue, Feb 22, 2011 at 10:18:31PM -0800, Justin Pettit wrote:
> Coverity #10726
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Tue, Feb 22, 2011 at 10:18:30PM -0800, Justin Pettit wrote:
> Coverity #10727
This looks good but we should also add WARN_UNUSED_RESULT to the
prototype for ovsdb_txn_commit() in transaction.h.
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
On Tue, Feb 22, 2011 at 10:18:29PM -0800, Justin Pettit wrote:
> Coverity #10730
Looks good, thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
On Tue, Feb 22, 2011 at 10:18:28PM -0800, Justin Pettit wrote:
> 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
Can't hurt. Th
On Tue, Feb 22, 2011 at 10:18:27PM -0800, Justin Pettit wrote:
> 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 @@
Thanks, pushed.
On Tue, Feb 22, 2011 at 05:23:28PM -0800, Justin Pettit wrote:
> 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.
On Fri, Feb 11, 2011 at 6:09 PM, Ben Pfaff wrote:
> On Fri, Feb 11, 2011 at 06:05:08PM +0100, Gaetano Catalli wrote:
>> I have some doubts to resolve about switch local port.
>> Firstly, in the ovs-openflowd manpage I can read: "...The Open???Flow
>> local port is a virtual network port that ovs-o
Hi all,
I am reading the netdev_linux_cache_cb() function. In the 'else'
branch, the cached values for all the netdev_devs need to be
invalidated.
To do so, you use the netdev_dev_get_devices() function to store in a
temporary shash structure the list of devices and then you walk
through this list
Hi all,
I am reading the netdev_linux_cache_cb() function. In the 'else'
branch, the cached values for all the netdev_devs need to be
invalidated.
To do so, you use the netdev_dev_get_devices() function to store in a
temporary shash structure the list of devices and then you walk
through this list
57 matches
Mail list logo
