[ovs-dev] [PATCH] netdev: Prevent using reserved names
Ben Pfaff
blp at nicira.com
Thu May 16 18:53:17 UTC 2013
On Wed, May 15, 2013 at 02:33:57PM -0700, Alex Wang wrote:
> This commit adds a function to 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).
>
> Bug #15077
>
> Signed-off-by: Alex Wang <alexw at nicira.com>
Thanks, this looks pretty good! A few comments follow.
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 699ed71..31daafc 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -115,6 +115,17 @@ netdev_vport_needs_dst_port(const struct netdev *dev)
> }
>
> const char *
> +netdev_vport_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;
I think that netdev_vport_get_dpif_port() could be written as a single
return statement, without an intermediate variable.
> +/* Check that the network device name is not the same as any of the registered
> + * vport providers' dpif_port name (dpif_port is NULL if the vport provider
> + * does not define it) or the datapath internal port name (e.g. ovs-system).
> + *
> + * false is returned if there is no naming conflict.
> + * true is returned otherwise.
> + */
> +bool
> +netdev_is_reserved_name(const char *name)
> +{
> + struct shash_node *node;
> + struct sset types;
> + const char *type;
> +
> + netdev_initialize();
> + SHASH_FOR_EACH(node, &netdev_classes) {
> + const char *dpif_port;
> + dpif_port = netdev_vport_class_get_dpif_port(node->data);
> + if (dpif_port && !strcmp(dpif_port, name)) {
> + return true;
> + }
> + }
> +
> + sset_init(&types);
> + dp_enumerate_types(&types);
> + SSET_FOR_EACH (type, &types) {
> + if ((strstr(name, "ovs-") == name)
> + && (strstr(name, type) == (name + 4))) {
This is a creative use of strstr(). However, I think it would be
better to use strncmp() instead, because it is more commonly used for
this purpose.
Also, I believe that the check for an "ovs-" prefix on 'name' can be
moved entirely outside the loop; I think that we don't even have to
enumerate the types if 'name' does not begin with "ovs-".
Thank you for writing a test.
> @@ -1418,6 +1418,13 @@ iface_do_create(const struct bridge *br,
> struct netdev *netdev;
> int error;
>
> + if (netdev_is_reserved_name(iface_cfg->name)) {
> + VLOG_WARN("could not create interface %s, name is reserved (%s)",
> + iface_cfg->name, strerror(EINVAL));
I don't think that adding strerror(EINVAL) adds value to the error
message here.
> + error = EINVAL;
> + goto error;
> + }
More information about the dev
mailing list