On Mon, Aug 13, 2018 at 10:48:01AM -0700, Han Zhou wrote:
> There are places with the pattern:
> if (!ovs_list_is_empty(&row->track_node)) {
> ovs_list_remove(&row->track_node);
> }
> ovs_list_push_back(&row->table->track_list,
> &row->track_node);
>
> It seems to be trying to prevent double removing the node from a list,
> but actually it doesn't, and may be misleading.
>
> The function ovs_list_is_empty() returns true only if the node has been
> initialized with ovs_list_init(). If a node is deleted but not
> initialized, the check will return false and the above code will continue
> deleting it again. But if a node is already initialized, calling
> ovs_list_remove() is a no-op. So the check is not necessary and misleading.
>
> In fact there is already a double removal resulted by this code: the function
> ovsdb_idl_db_clear() removes the node but it then calls
> ovsdb_idl_row_destroy()
> immediately which removes the node again. It should not result in any real
> issue yet since the list was not changed so the second removal just
> assigns the pointers with same value.
>
> It is in fact not necessary to remove and then add back to the list, because
> the purpose of the change tracking is just to tell if a row is changed
> or not. So this patch removes the "check and remove" code before adding
> the node to a list, but instead, adding it to the list only if it is
> not in a list. This way it ensures the node is added to a list only once
> in one idl loop. (ovsdb_idl_db_track_clear() will be called in each
> iteration and the check ovs_list_is_empty() will return true at the first
> check in next iteration).
>
> Signed-off-by: Han Zhou <[email protected]>
Thanks. Applied to master.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
