[ovs-dev] [PATCH] bridge: store system interface status information in Interface table

[Ben Pfaff]

On Sat, Jan 15, 2011 at 11:25:08PM -0800, Andrew Evans wrote:
> This patch adds columns to the Interface table to store state
> information about physical network interfaces. It adds code to the
> bridge to populate those colums when the Interface table is
> refreshed. It also stores certain other state information in the
> existing status map in the Interface table.

Thank you for writing this up!  It looks good, but I have some comments
below.

The 'const' change to netdev_get_features() is logically separate and
should be broken apart into a separate commit.

You should add yourself to AUTHORS as part of your first commit.

> @@ -1964,6 +1964,25 @@ netdev_linux_get_next_hop(const struct
> in_addr *host, struct in_addr *next_hop,
>      return ENXIO;
>  }
> 
> +static int
> +netdev_linux_get_status(const struct netdev *netdev, struct shash *sh)
> +{
> +    struct ethtool_drvinfo drvinfo;
> +    int error;
> +
> +    memset(&drvinfo, 0, sizeof drvinfo);
> +    if (!(error = netdev_linux_do_ethtool(netdev_get_name(netdev),
> +                                          (struct ethtool_cmd *)&drvinfo,
> +                                          ETHTOOL_GDRVINFO,
> +                                          "ETHTOOL_GDRVINFO"))) {

Please don't put an assignment into an 'if' statement's condition.  Use
a separate expression statement instead.

> +    if (!netdev_get_flags(iface->netdev, &flags)) {
> +        ovsrec_interface_set_admin_state(iface->cfg, xstrdup(flags
> & NETDEV_UP
> +                                                             ? "up"
> : "down"));
> +    }

The xstrdup() here should go because ovsrec_interface_set_admin_state()
makes an internal copy of its argument.  Ditto for the other xstrdup()
calls just below this one (I see two others).

We should clear these columns, to empty, if the values cannot be
obtained (but see below for schema update first).

> +         "ephemeral": true},
> +       "admin_state": {
> +         "type": "string",
> +         "ephemeral": true},
> +       "link_state": {
> +         "type": "string",
> +         "ephemeral": true},
> +       "link_speed": {
> +         "type": "integer",
> +         "ephemeral": true},
> +       "duplex": {
> +         "type": "string",
> +         "ephemeral": true},
> +       "mtu": {
> +         "type": "integer",
>           "ephemeral": true}}},

All of the above should have "min: 0, max: 1" so that they are initially
empty but can be assigned a value.  admin_state, link_state, and duplex
should have enum constraints to restrict the allowed values.  It might
not be obvious how to do those things.  There are examples elsewhere in
the schema, documentation in ovsdb/SPECS, or if none of that makes
enough sense then just let know and I'll write it up for you myself.

In vswitch.xml, from the patch it looks like it doesn't follow the
indentation style used in the rest of the file; that might be patch
formatting breakage though.

Also in vswitch.xml, I would put the new columns into a new column
group.  Maybe 'status' should go into that group too.

Also in vswitch.xml, I don't think that we should say that the values
are valid only for "system" interfaces.  I would instead say that not
every interface necessarily has these properties, give an example
(e.g. virtual interfaces don't have a link speed), and say that the
column will be empty if there is no value.  Since this remark applies to
every column in the group, you can just say it once in the introduction
to the group (between <group> and the first <column> inside it).

Thanks,

Ben.