[ovs-dev] [gc 13/13] ovsdb: Implement garbage collection.
Ethan Jackson
ethan at nicira.com
Mon Mar 7 21:55:26 UTC 2011
Looks like a good patch. Only a few minor aesthetic comments.
I didn't carefully review the dot2pic changes.
Out of curiosity, why do we want to make the QoS and Queue table part
of the root set? Are they useful when unreferenced?
> + options = {}
> + if table.is_root:
> + options['style'] = 'bold'
> + print "\t%s [%s];" % (
> + tableName,
> + ', '.join(['%s=%s' % (k,v) for k,v in options.items()]))
There is trailing whitespace after tableName.
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index b0c1026..a3f93a5 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
There is some trailing whitespace strewn about this file.
> @@ -3402,8 +3346,20 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
> "with \"-- --id=%s create ...\")",
> node->name, node->name);
> }
> + if (!symbol->strong_ref) {
> + if (!symbol->weak_ref) {
> + VLOG_WARN("row id \"%s\" was created but no reference to it "
> + "was inserted, so it will not actually appear in "
> + "the database", node->name);
> + } else {
> + VLOG_WARN("row id \"%s\" was created but only a weak "
> + "reference to it was inserted, so it will not "
> + "actually appear in the database", node->name);
> + }
> + }
> }
>
> +
Extra newline added here.
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 3537d28..739952b 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
I think it's fine how it is, but my inclination would be to make
isRoot = false the default and force the user to specify true on root
nodes. Most nodes we create will not be root nodes, so it seems a bit
less error prone.
> <?xml version="1.0" encoding="utf-8"?>
> <database title="Open vSwitch Configuration Database">
> <p>A database with this schema holds the configuration for one Open
> - vSwitch daemon. The root of the configuration for the daemon is
> - the <ref table="Open_vSwitch"/> table, which must have exactly one
> - record. Records in other tables are significant only when they
> - can be reached directly or indirectly from the
> - <ref table="Open_vSwitch"/> table.</p>
> + vSwitch daemon. The top-level configuration for the daemon is the
> + <ref table="Open_vSwitch"/> table, which must have exactly one
> + record. Records in other tables are significant only when they can
> + be reached directly or indirectly from the <ref
> + table="Open_vSwitch"/> table. Records that are not reachable from
> + the <ref table="Open_vSwitch"/> table are automatically deleted from
> + the database, except for records in a few distinguished ``root set''
I would phrase this as "except for a few records in a distinguished"
The above paragraph should be left shifted two spaces to be consistent
with the indentation in the rest of the file.
Ethan
More information about the dev
mailing list