On 06/13/2018 11:29 PM, 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
Here's a different datapoint in the same category.
I got a slightly different error when I tried to compile.
ovn/controller/bfd-vswitch-idl.h was auto-generated and everything
worked up until the very end:
"The following files are in git but not the distribution:
ovn/controller/bfd-vswitch-idl.def"
The make command I ran was `make sandbox SANDBOXFLAGS="--ovn"`
I tried running `make distclean` then reconfiguring, but this didn't help.
For comparison, Han, these are my software versions, in case that might
be why auto-generation worked for me but not you:
gcc version is 7.3.1
make version is 4.2.1
autoconf version is 2.69
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?
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.
Thanks,
Han
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
