[ovs-dev] [PATCH v3 11/16] ovsdb-idl: Tracking - preserve data for deleted rows.

Han Zhou zhouhan at gmail.com
Tue Jun 5 00:25:36 UTC 2018


On Mon, Jun 4, 2018 at 3:48 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> On Fri, Jun 01, 2018 at 10:42:01AM -0700, Han Zhou wrote:
> > OVSDB IDL can track changes, but for deleted rows, the data is
> > destroyed and only uuid is tracked. In some cases we need to
> > check the data of the deleted rows. This patch preserves data
> > for deleted rows until track clear is called.
> >
> > Signed-off-by: Han Zhou <hzhou8 at ebay.com>
>
> I like the idea of preserving data, but preserving it in the parsed form
> concerns me because the parsed data might contain pointers to other
> structures that don't exist anymore.  How important is it to preserve it
> in parsed form?

Thanks for pointing out. I understand your concern, but I also wonder how
would it help if it is not in parsed form. My use case is actually simple:
in patch https://patchwork.ozlabs.org/patch/924270/, I just need to access
as->name:

+    SBREC_ADDRESS_SET_FOR_EACH_TRACKED (as, ctx->ovnsb_idl) {
+        if (sbrec_address_set_is_deleted(as)) {
+            expr_const_sets_remove(addr_sets, as->name);
+            sset_add(deleted, as->name);

Without parsed form, I will have to access the row's
header_.tracked_old_datum, and parse it myself like: name =
tracked_old_datum->keys[0].string. Even if it works this way, it seems not
a general approach for the tracking feature and the code might be broken if
schema changes.

Meanwhile, even if we unparse it, we can't prevent someone to access the
fields of the generated data structure. It has always been a problem if a
tracked but deleted row is accessed. For example, without this patch, we
can't prevent someone to access as->name at this point, and it is just
random data because the string has been freed already. This patch makes the
access to the direct fields valid, but accessing referenced rows remains
invalid.

I think we can at least add some comment in the code warning the developers
to avoid accessing references using tracked but deleted rows.

A better approach might be preserving all the referenced and to-be-deleted
rows, even if those rows are not being tracked, and destroy them also in
the ovsdb_idl_db_track_clear(). I am not sure though, is there any known
traps for this approach. If it is just about adding the referenced rows in
a list and make sure they are checked and destroyed in the end, it
shouldn't be a big change.

Maybe I can keep this patch (adding some comments) for now since the next
patch depends on it, and then work on preserving all referenced rows until
ovsdb_idl_db_track_clear() in a separate patch. What do you think?

Thanks,
Han


More information about the dev mailing list