Thanks for writing this up. It looks like we're on the right track.
Just a couple of comments before we get this merged.
In the subject I would change the subject to:
"netdev: Prevent using reserved names."
I.E, the generally for the prefix we use the module most affected, and
after the prefix we use a sentence with capitalization and a period.
> This commit adds function in lib/netdev.c to check that the interface name
> is not the same as any of the registered vport providers' dpif_port name
> (e.g. gre_system) or the datapath's internal port name (e.g. ovs-system).
"This commit adds function in" => "This commit adds a function to"
> This function is called in iface_do_create() function in vswitchd/bridge.c.
> If there is a name conflict, no interface and of_port will be created. And
> the warning will be given in the ovs-vswitchd.log
This second paragraph of the commit message is implementation detail
which is clear from the patch. I'd just get rid of it.
I'd go ahead and add the bug number to the commit message. See git
log for an example.
> const char *
> +netdev_class_get_dpif_port(const struct netdev_class *class)
> +{
> + const char *dpif_port;
> +
> + dpif_port = (is_vport_class(class)
> + ? vport_class_cast(class)->dpif_port
> + : NULL);
> + return dpif_port;
> +}
In keeping with the naming convention for the file, I would calls this
netdev_vport_class_get_dpif_port(). One could argue that we could
dispense with with this function and add the vport name to struct
netdev_class directly, but I'm not sure if it's worth it or not.
Perhaps someone else may have a strong opinion?
> +int
> +netdev_check_reserved_word(const char *name)
Perhaps we should call this "netdev_is_reserved_name()" so it's clear
that the 'word' we're speaking of is the devices name? I'd just have
it return a bool and let the upper layer handle the warning.
> +{
> + struct shash_node *node;
> + struct sset types;
> + const char *dpif_port;
> + const char *type;
'dpif_port' and 'type' can be defined in they SHASH_FOR_EACH loops
closer to where they're used.
> +
> + dpif_port = NULL;
This assignment is redundant.
> + error = netdev_check_reserved_word(iface_cfg->name);
> + if (error) {
> + goto error;
> + }
> +
As mentioned above, I'd do the warning here instead of in the netdev
layer. Also I think the warning message could be a bit more
descriptive. Something like "could not create interface %s, name is
reserved"
Ethan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
