Re: [ovs-dev] [PATCH] OVSDB: Fix segfalut during replication.

2016-09-27 Thread Andy Zhou 
On Mon, Sep 26, 2016 at 8:30 PM, Ben Pfaff  wrote:

> On Tue, Sep 20, 2016 at 01:27:08PM -0700, Andy Zhou wrote:
> > The newly added replication logic makes it possible for a monitor to
> > receive delete and insertion of the same row back to back, which
> > was not possible before. Add logic (and comment) to handle this
> > case to avoid follow crash reported by Valgrind:
>
> On Tue, Sep 20, 2016 at 01:27:08PM -0700, Andy Zhou wrote:
> > The newly added replication logic makes it possible for a monitor to
> > receive delete and insertion of the same row back to back, which
> > was not possible before. Add logic (and comment) to handle this
> > case to avoid follow crash reported by Valgrind:
>
> Thanks a lot.
>
> Acked-by: Ben Pfaff 
>
Thanks for the review. Pushed to master and branch-2.6.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] OVSDB: Fix segfalut during replication.

2016-09-26 Thread Ben Pfaff 
I'm really puzzled about how this is getting to the list.
d...@openvswitch.com bounces for me.  I had to change it to
dev@openvswitch.org to avoid that.  I see that openvswitch.com is
registered to the Linux Foundation though.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] OVSDB: Fix segfalut during replication.

2016-09-26 Thread Ben Pfaff 
On Tue, Sep 20, 2016 at 01:27:08PM -0700, Andy Zhou wrote:
> The newly added replication logic makes it possible for a monitor to
> receive delete and insertion of the same row back to back, which
> was not possible before. Add logic (and comment) to handle this
> case to avoid follow crash reported by Valgrind:

On Tue, Sep 20, 2016 at 01:27:08PM -0700, Andy Zhou wrote:
> The newly added replication logic makes it possible for a monitor to
> receive delete and insertion of the same row back to back, which
> was not possible before. Add logic (and comment) to handle this
> case to avoid follow crash reported by Valgrind:

Thanks a lot.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] OVSDB: Fix segfalut during replication.

2016-09-20 Thread Andy Zhou 
On Tue, Sep 20, 2016 at 4:39 PM, Joe Stringer  wrote:

> On 20 September 2016 at 13:27, Andy Zhou  wrote:
> > The newly added replication logic makes it possible for a monitor to
> > receive delete and insertion of the same row back to back, which
> > was not possible before. Add logic (and comment) to handle this
> > case to avoid follow crash reported by Valgrind:
> >
> > #0  0x00453edd in ovsdb_datum_compare_3way
> > (a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1626
> > #1  0x00453ea4 in ovsdb_datum_equals
> > (a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1616
> > #2  0x0041b651 in update_monitor_row_data
> > (mt=0x5eda4a0, row=0x5efbe00, data=0x0) at
> ovsdb/monitor.c:310
> > #3  0x0041ed14 in ovsdb_monitor_changes_update
> > (old=0x0, new=0x5efbe00, mt=0x5eda4a0, changes=0x5ef7180)
> > at ovsdb/monitor.c:1255
> > #4  0x0041f12e in ovsdb_monitor_change_cb
> > (old=0x0, new=0x5efbe00, changed=0x5efc218, aux_=0xffefff040)
> > at ovsdb/monitor.c:1339
> > #5  0x0042ded9 in ovsdb_txn_for_each_change
> > (txn=0x5efbd90, cb=0x41ef50 ,
> >  aux=0xffefff040) at ovsdb/transaction.c:906
> > #6  0x00420155 in ovsdb_monitor_commit
> > (replica=0x5eda2c0, txn=0x5efbd90, durable=false)
> > at ovsdb/monitor.c:1553
> > #7  0x0042dc04 in ovsdb_txn_commit_
> > (txn=0x5efbd90, durable=false) at ovsdb/transaction.c:868
> > #8  0x0042ddd4 in ovsdb_txn_commit (txn=0x5efbd90,
> durable=false)
> > at ovsdb/transaction.c:893
> > #9  0x00422e0c in process_notification
> > (table_updates=0x5efad10, db=0x5e6bd40) at
> ovsdb/replication.c:575
> > #10 0x00420ff3 in replication_run () at
> ovsdb/replication.c:184
> > #11 0x00405cc8 in main_loop
> > (jsonrpc=0x5e67770, all_dbs=0xffefff3a0, unixctl=0x5ebd980,
> >  remotes=0xffefff360, run_process=0x0, exiting=0xffefff3c0,
> > is_backup=0xffefff2de) at ovsdb/ovsdb-server.c:198
> > #12 0x00406edb in main (argc=1, argv=0xffefff550)
> > at ovsdb/ovsdb-server.c:429
> >
> > Reported-by: Joe Stringer 
> > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/
> 079315.html
> > Reported-by: Alin Serdean 
> > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/
> 079586.html
> > Co-authored-by: Joe Stringer 
> > Signed-off-by: Andy Zhou 
>
> Signed-off-by: Joe Stringer 
>
> > ---
> >  ovsdb/monitor.c | 41 -
> >  1 file changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> > index a590943..7e6ddcb 100644
> > --- a/ovsdb/monitor.c
> > +++ b/ovsdb/monitor.c
> > @@ -1252,7 +1252,46 @@ ovsdb_monitor_changes_update(const struct
> ovsdb_row *old,
> >  change->new = clone_monitor_row_data(mt, new);
> >  } else {
> >  if (new) {
> > -update_monitor_row_data(mt, new, change->new);
> > +if (!change->new) {
>
> Usually we do "if (positive condition) { ... } else { ... }", so
> readers don't have to double-negate in their mind to figure out what
> the "else" condition is.
>

I was trying to point out the "interesting" case first.
On the other hand, I won't object to flip them around, since the comment
section
is bigger than usual.

>
> I'll leave the review of the reasoning below to others.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] OVSDB: Fix segfalut during replication.

2016-09-20 Thread Joe Stringer 
On 20 September 2016 at 13:27, Andy Zhou  wrote:
> The newly added replication logic makes it possible for a monitor to
> receive delete and insertion of the same row back to back, which
> was not possible before. Add logic (and comment) to handle this
> case to avoid follow crash reported by Valgrind:
>
> #0  0x00453edd in ovsdb_datum_compare_3way
> (a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1626
> #1  0x00453ea4 in ovsdb_datum_equals
> (a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1616
> #2  0x0041b651 in update_monitor_row_data
> (mt=0x5eda4a0, row=0x5efbe00, data=0x0) at ovsdb/monitor.c:310
> #3  0x0041ed14 in ovsdb_monitor_changes_update
> (old=0x0, new=0x5efbe00, mt=0x5eda4a0, changes=0x5ef7180)
> at ovsdb/monitor.c:1255
> #4  0x0041f12e in ovsdb_monitor_change_cb
> (old=0x0, new=0x5efbe00, changed=0x5efc218, aux_=0xffefff040)
> at ovsdb/monitor.c:1339
> #5  0x0042ded9 in ovsdb_txn_for_each_change
> (txn=0x5efbd90, cb=0x41ef50 ,
>  aux=0xffefff040) at ovsdb/transaction.c:906
> #6  0x00420155 in ovsdb_monitor_commit
> (replica=0x5eda2c0, txn=0x5efbd90, durable=false)
> at ovsdb/monitor.c:1553
> #7  0x0042dc04 in ovsdb_txn_commit_
> (txn=0x5efbd90, durable=false) at ovsdb/transaction.c:868
> #8  0x0042ddd4 in ovsdb_txn_commit (txn=0x5efbd90, durable=false)
> at ovsdb/transaction.c:893
> #9  0x00422e0c in process_notification
> (table_updates=0x5efad10, db=0x5e6bd40) at ovsdb/replication.c:575
> #10 0x00420ff3 in replication_run () at ovsdb/replication.c:184
> #11 0x00405cc8 in main_loop
> (jsonrpc=0x5e67770, all_dbs=0xffefff3a0, unixctl=0x5ebd980,
>  remotes=0xffefff360, run_process=0x0, exiting=0xffefff3c0,
> is_backup=0xffefff2de) at ovsdb/ovsdb-server.c:198
> #12 0x00406edb in main (argc=1, argv=0xffefff550)
> at ovsdb/ovsdb-server.c:429
>
> Reported-by: Joe Stringer 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-September/079315.html
> Reported-by: Alin Serdean 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-September/079586.html
> Co-authored-by: Joe Stringer 
> Signed-off-by: Andy Zhou 

Signed-off-by: Joe Stringer 

> ---
>  ovsdb/monitor.c | 41 -
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index a590943..7e6ddcb 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -1252,7 +1252,46 @@ ovsdb_monitor_changes_update(const struct ovsdb_row 
> *old,
>  change->new = clone_monitor_row_data(mt, new);
>  } else {
>  if (new) {
> -update_monitor_row_data(mt, new, change->new);
> +if (!change->new) {

Usually we do "if (positive condition) { ... } else { ... }", so
readers don't have to double-negate in their mind to figure out what
the "else" condition is.

I'll leave the review of the reasoning below to others.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] OVSDB: Fix segfalut during replication.

2016-09-20 Thread Andy Zhou 
The newly added replication logic makes it possible for a monitor to
receive delete and insertion of the same row back to back, which
was not possible before. Add logic (and comment) to handle this
case to avoid follow crash reported by Valgrind:

#0  0x00453edd in ovsdb_datum_compare_3way
(a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1626
#1  0x00453ea4 in ovsdb_datum_equals
(a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1616
#2  0x0041b651 in update_monitor_row_data
(mt=0x5eda4a0, row=0x5efbe00, data=0x0) at ovsdb/monitor.c:310
#3  0x0041ed14 in ovsdb_monitor_changes_update
(old=0x0, new=0x5efbe00, mt=0x5eda4a0, changes=0x5ef7180)
at ovsdb/monitor.c:1255
#4  0x0041f12e in ovsdb_monitor_change_cb
(old=0x0, new=0x5efbe00, changed=0x5efc218, aux_=0xffefff040)
at ovsdb/monitor.c:1339
#5  0x0042ded9 in ovsdb_txn_for_each_change
(txn=0x5efbd90, cb=0x41ef50 ,
 aux=0xffefff040) at ovsdb/transaction.c:906
#6  0x00420155 in ovsdb_monitor_commit
(replica=0x5eda2c0, txn=0x5efbd90, durable=false)
at ovsdb/monitor.c:1553
#7  0x0042dc04 in ovsdb_txn_commit_
(txn=0x5efbd90, durable=false) at ovsdb/transaction.c:868
#8  0x0042ddd4 in ovsdb_txn_commit (txn=0x5efbd90, durable=false)
at ovsdb/transaction.c:893
#9  0x00422e0c in process_notification
(table_updates=0x5efad10, db=0x5e6bd40) at ovsdb/replication.c:575
#10 0x00420ff3 in replication_run () at ovsdb/replication.c:184
#11 0x00405cc8 in main_loop
(jsonrpc=0x5e67770, all_dbs=0xffefff3a0, unixctl=0x5ebd980,
 remotes=0xffefff360, run_process=0x0, exiting=0xffefff3c0,
is_backup=0xffefff2de) at ovsdb/ovsdb-server.c:198
#12 0x00406edb in main (argc=1, argv=0xffefff550)
at ovsdb/ovsdb-server.c:429

Reported-by: Joe Stringer 
Reported-at: http://openvswitch.org/pipermail/dev/2016-September/079315.html
Reported-by: Alin Serdean 
Reported-at: http://openvswitch.org/pipermail/dev/2016-September/079586.html
Co-authored-by: Joe Stringer 
Signed-off-by: Andy Zhou 
---
 ovsdb/monitor.c | 41 -
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index a590943..7e6ddcb 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1252,7 +1252,46 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old,
 change->new = clone_monitor_row_data(mt, new);
 } else {
 if (new) {
-update_monitor_row_data(mt, new, change->new);
+if (!change->new) {
+/* Reinsert the row that was just deleted.
+ *
+ * This path won't be hit without replication.  Whenever OVSDB
+ * server inserts a new row, It always generates a new UUID
+ * that is different from the row just deleted.
+ *
+ * With replication, this path can be hit in a corner
+ * case when two OVSDB servers are set up to replicate
+ * each other. Not that is a useful set up, but can
+ * happen in practice.
+ *
+ * An example of how this path can be hit is documented below.
+ * The details is not as important to the correctness of the
+ * logic, but added here to convince ourselves that this path
+ * can be hit.
+ *
+ * Imagine two OVSDB servers that replicates from each
+ * other. For each replication session, there is a
+ * corresponding monitor at the other end of the replication
+ * JSONRPC connection.
+ *
+ * The events can lead to a back to back deletion and
+ * insertion operation of the same row for the monitor of
+ * the first server are:
+ *
+ * 1. A row is inserted in the first OVSDB server.
+ * 2. The row is then replicated to the remote OVSDB server.
+ * 3. The row is now  deleted by the local OVSDB server. This
+ *deletion operation is replicated to the local monitor
+ *of the OVSDB server.
+ * 4. The monitor now receives the same row, as an insertion,
+ *from the replication server. Because of
+ *replication, the row carries the same UUID as the row
+ *just deleted.
+ */
+change->new = clone_monitor_row_data(mt, new);
+} else {
+