[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