On Fri, Jan 08, 2021 at 02:59:24PM +0100, Dumitru Ceara wrote:
> On 12/19/20 3:44 AM, Ben Pfaff wrote:
> > This change breaks the IDL into two layers: the IDL proper, whose
> > interface to its client is unchanged, and a low-level library called
> > the OVSDB "client synchronization" (CS) library. There are two
> > reasons for this change. First, the IDL is big and complicated and
> > I think that this change factors out some of that complication into
> > a simpler lower layer. Second, the OVN northd implementation based
> > on DDlog can benefit from the client synchronization library even
> > though it would actually be made increasingly complicated by the IDL.
> >
> > Signed-off-by: Ben Pfaff <[email protected]>
> We leak both 'table' and 'table->name' because we don't free them here.
Thanks, I fixed this with:
@@ -777,11 +777,11 @@ ovsdb_cs_set_probe_interval(const struct ovsdb_cs *cs,
int probe_interval)
* At the CS layer, the only thing we care about, table-wise, is the conditions
* we're using for monitoring them, so there's little here. We only create
* these table structures at all for tables that have conditions. */
struct ovsdb_cs_db_table {
struct hmap_node hmap_node; /* Indexed by 'name'. */
- const char *name; /* Table name. */
+ char *name; /* Table name. */
/* Each of these is a null pointer if it is empty, or JSON [<condition>*]
* or [true] or [false] otherwise. [true] could be represented as a null
* pointer, but we want to distinguish "empty slot" from "a condition that
* is always true" in a slot. */
@@ -877,10 +877,12 @@ ovsdb_cs_db_destroy_tables(struct ovsdb_cs_db *db)
HMAP_FOR_EACH_SAFE (table, next, hmap_node, &db->tables) {
json_destroy(table->ack_cond);
json_destroy(table->req_cond);
json_destroy(table->new_cond);
hmap_remove(&db->tables, &table->hmap_node);
+ free(table->name);
+ free(table);
}
hmap_destroy(&db->tables);
}
> > + } else {
> > + /* 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 fixed this with:
@@ -1044,23 +1046,21 @@ ovsdb_cs_db_sync_condition(struct ovsdb_cs_db *db)
json_destroy(table->ack_cond);
table->ack_cond = table->req_cond;
table->req_cond = NULL;
}
} else {
- /* 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) {
+ /* There was an in-flight monitor_cond_change request. It's no
+ * longer relevant in the restarted FSM, so clear it. */
if (table->new_cond) {
+ /* We will send a new monitor_cond_change with the new
+ * condition. The previously in-flight condition is
+ * irrelevant and we can just forget about it. */
json_destroy(table->req_cond);
} else {
+ /* The restarted FSM needs to again send a request for the
+ * previously in-flight condition. */
table->new_cond = table->req_cond;
}
table->req_cond = NULL;
db->cond_changed = true;
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
