[ovs-dev] [PATCH v4 3/3] ovsdb-idl: Break into two layers.

[Ben Pfaff]

On Wed, Jan 13, 2021 at 02:56:45AM +0100, Ilya Maximets wrote:
> On 1/8/21 2:59 PM, Dumitru Ceara wrote:
> > On 12/19/20 3:44 AM, Ben Pfaff wrote:
> >> +            /* If there was no "unsent" condition but instead a
> >> +             * monitor_cond_change request was in flight, move table->req_cond
> >> +             * to table->new_cond and set db->cond_changed to trigger a new
> >> +             * monitor_cond_change request.
> >> +             *
> >> +             * However, if a new condition has been set by the CS client,
> >> +             * monitor_cond_change will be sent anyway and will use the most
> >> +             * recent table->new_cond so there's no need to update it here.
> >> +             */
> >> +            if (table->req_cond) {
> >> +                if (table->new_cond) {
> >> +                    json_destroy(table->req_cond);
> >> +                } else {
> >> +                    table->new_cond = table->req_cond;
> >> +                }
> >> +                table->req_cond = NULL;
> >> +                db->cond_changed = true;
> >> +            }
> > 
> > This used to be:
> > 
> > if (table->req_cond && !table->new_cond) {
> >     /* Move "req_cond" to "new_cond". */
> >     ovsdb_idl_condition_move(&table->new_cond, &table->req_cond);
> >     db->cond_changed = true;
> > }
> > 
> > Which is what the comment above tried to explain.
> > 
> > Is there a case that was missed in the old code?  If so, can we factor
> > out the fix in a separate patch to make it easier to track?
> 
> I suppose that this additional check here is to clear 'req_cond', so
> we will not assert inside ovsdb_cs_db_compose_cond_change().
> Previously we had ovsdb_idl_condition_move() function that took care
> of destroying the destination, but now we have no such function and
> have an assert to be sure that we're not leaking the 'req_cond'.
> 
> This should not actually change the logic.  But I think that we need
> to rephrase part of the comment that starts with 'However' as it
> doesn't reflect the code anymore.

Yes.  I folded in the incremental that I sent in reply to Dumitru to fix
this (just the comments, not the logic).

> Otherwise, the patch looks ok to me.

Thanks,

Ben.