On Tue, Sep 20, 2016 at 4:39 PM, Joe Stringer <j...@ovn.org> wrote: > On 20 September 2016 at 13:27, Andy Zhou <az...@ovn.org> 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 0x0000000000453edd in ovsdb_datum_compare_3way > > (a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1626 > > #1 0x0000000000453ea4 in ovsdb_datum_equals > > (a=0x5efbe60, b=0x0, type=0x5e6a848) at lib/ovsdb-data.c:1616 > > #2 0x000000000041b651 in update_monitor_row_data > > (mt=0x5eda4a0, row=0x5efbe00, data=0x0) at > ovsdb/monitor.c:310 > > #3 0x000000000041ed14 in ovsdb_monitor_changes_update > > (old=0x0, new=0x5efbe00, mt=0x5eda4a0, changes=0x5ef7180) > > at ovsdb/monitor.c:1255 > > #4 0x000000000041f12e in ovsdb_monitor_change_cb > > (old=0x0, new=0x5efbe00, changed=0x5efc218, aux_=0xffefff040) > > at ovsdb/monitor.c:1339 > > #5 0x000000000042ded9 in ovsdb_txn_for_each_change > > (txn=0x5efbd90, cb=0x41ef50 <ovsdb_monitor_change_cb>, > > aux=0xffefff040) at ovsdb/transaction.c:906 > > #6 0x0000000000420155 in ovsdb_monitor_commit > > (replica=0x5eda2c0, txn=0x5efbd90, durable=false) > > at ovsdb/monitor.c:1553 > > #7 0x000000000042dc04 in ovsdb_txn_commit_ > > (txn=0x5efbd90, durable=false) at ovsdb/transaction.c:868 > > #8 0x000000000042ddd4 in ovsdb_txn_commit (txn=0x5efbd90, > durable=false) > > at ovsdb/transaction.c:893 > > #9 0x0000000000422e0c in process_notification > > (table_updates=0x5efad10, db=0x5e6bd40) at > ovsdb/replication.c:575 > > #10 0x0000000000420ff3 in replication_run () at > ovsdb/replication.c:184 > > #11 0x0000000000405cc8 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 0x0000000000406edb in main (argc=1, argv=0xffefff550) > > at ovsdb/ovsdb-server.c:429 > > > > Reported-by: Joe Stringer <j...@ovn.org> > > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/ > 079315.html > > Reported-by: Alin Serdean <aserd...@cloudbasesolutions.com> > > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/ > 079586.html > > Co-authored-by: Joe Stringer <j...@ovn.org> > > Signed-off-by: Andy Zhou <az...@ovn.org> > > Signed-off-by: Joe Stringer <j...@ovn.org> > > > --- > > 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
