On Wed, Jun 13, 2018 at 08:29:28PM -0700, Han Zhou wrote: > On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff <[email protected]> wrote: > > > > To make ovn-controller recompute incrementally, we need accurate > > dependencies for each function that reads or writes a table. It's > > currently difficult to be sure about these dependencies, and certainly > > difficult to maintain them over time, because there's no way to actually > > enforce them. > > > > This commit experiments with an approach that allows for fairly > > fine-grained access control within ovn-controller to tables and columns. > > It's based on generating a new version of the IDL data structures for each > > case where we want different access control. All of these data structures > > have the same format, but the columns that a given piece of code is not > > supposed to touch are renamed to discourage programmers from using them, > > e.g. they're given names suffixed with "__accessdenied". (This means > > that there is no runtime overhead to the access control since it only > > requires a cast to convert between the regular and restricted versions.) > > In addition, when a columns is supposed to be read-only, functions for > > modifying the column are not supplied. > > > > This commit only tries out this experiment for a single file within > > ovn-controller, the BFD implementation (mostly because that's > > alphabetically first, no other real reason). It would require a little > > more work to apply it everywhere, but it's probably not a huge deal. > > > > Comments? > > > > CC: Han Zhou <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > ovn/controller/automake.mk | 5 + > > ovn/controller/bfd-vswitch-idl.def | 21 ++++ > > ovn/controller/bfd.c | 20 ++-- > > ovn/controller/bfd.h | 8 +- > > ovn/controller/ovn-controller.c | 13 ++- > > ovsdb/ovsdb-idlc.in | 223 ++++++++++++++++++++++++++++++ > ++++++- > > 6 files changed, 268 insertions(+), 22 deletions(-) > > create mode 100644 ovn/controller/bfd-vswitch-idl.def > > > > I wanted to have a quick test but it didn't pass the compile: > In file included from ovn/controller/bfd.c:17:0: > ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl.h: > No such file or directory
Hmm. That's a little puzzling. This file should get automatically generated by "make". I assume that I messed up a dependency somewhere, but at first glance I can't see the problem. > I didn't debug, but by looking at the code I got the idea. It ensures the > purpose is declared through the generated data type and violations are > captured at compile time. > I think this is a brilliant way to achieve the check with such a small > change, however, I am not sure how is it supposed to help for generating > the dependency. I think instead of caring about whether it is read-only, we > should care about whether it is write-only. > > For example, a engine node A reads on Table T1's column C1, reads & writes > on Table T2's column C2, and writes on T3's C3. In this case A depends on > T1 and T2, but not depends on T3. > So I think what matters to the dependency generation is whether it is > Write-Only. Read-Only is no different from ReadWrite. > > If my understanding is correct (that we don't care about the difference > between RO and RW), for WO columns, we can simply just don't monitor its > change. Then we don't even need this feature from dependency generation > point of view. > Did I miss something? It's easy enough to add support for write-only columns here. I'll add that in the next version. > Another thing, however, I think is really important for the dependency > generation is the columns that reference other tables, e.g. bfd_run() > doesn't has table "ovsrec_port" as input, but in fact it depends on this > table by referencing from ovsrec_bridge->ports. We need the table being > referenced appear in the input parameters. > The approach in the patch may not solve that problem directly, but > something similar may work: we could declare the usage for a referenced > column as "direct" or "indirect". "indirect" means the column can be > accessed only with "get functions" instead of pointers. The "get functions" > requires an additional parameter that is the instance of the table being > referenced. To call the "get function" to access the column, the caller > function needs to have the table being referenced passed in as parameter, > so we will be able to catch the dependency. I am not sure if this approach > is too heavy, or is tricky to implement. I'd think that the patch solves this problem by declaring that bfd.c depends only on the tables and columns that bfd-vswitch-idl.def allows it to access. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
