[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