[ovs-dev] [PATCH 1/6] datapath: Return vport configuration when queried.

Jesse Gross jesse at nicira.com
Wed Dec 22 15:27:59 PST 2010


On Wed, Dec 22, 2010 at 3:04 AM, Justin Pettit <jpettit at nicira.com> wrote:
> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> ---

I don't think this patch is trivial enough to skip having a proper
commit message.  In particular, I would expect it to explain *why* the
change is being made.

> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index eac3fa3..0c9ebbd 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> @@ -1524,6 +1524,15 @@ const unsigned char *tnl_get_addr(const struct vport *vport)
>        return rcu_dereference_rtnl(tnl_vport->mutable)->eth_addr;
>  }
>
> +void tnl_get_config(const struct vport *vport, void *config)
> +{
> +       const struct tnl_vport *tnl_vport = tnl_vport_priv(vport);
> +       struct tnl_port_config *tpc;

In other places we use actually spell this out as 'port_config' and in
general have avoided using initialisms like this as they are hard to
read.

> +
> +       tpc = &rcu_dereference_rtnl(tnl_vport->mutable)->port_config;
> +       memcpy(config, tpc, sizeof *tpc);

Kernel style is to use parenthesis with sizeof.

> diff --git a/datapath/vport-patch.c b/datapath/vport-patch.c
> index 4fdbcf5..032c0ac 100644
> --- a/datapath/vport-patch.c
> +++ b/datapath/vport-patch.c
> @@ -232,6 +232,12 @@ static const unsigned char *patch_get_addr(const struct vport *vport)
>        return rcu_dereference_rtnl(patch_vport->devconf)->eth_addr;
>  }
>
> +static void patch_get_config(const struct vport *vport, void *config)
> +{
> +       const struct patch_vport *patch_vport = patch_vport_priv(vport);
> +       strlcpy(config, patch_vport->peer_name, VPORT_CONFIG_SIZE);
> +}

This isn't SMP safe.  Previously, peer_name was only accessed from
places where the port config was being changed and in those places
RTNL mutex is always held.  However, when this is called we don't hold
RTNL, so it is possible for peer_name to change during the copy.  The
right way to do it is to move peer_name into struct device_config and
use RCU to access it.

>  /**
> + *     vport_get_config - retrieve device configuration
> + *
> + * @vport: vport from which to retrieve the configuration.
> + * @config: buffer to store config, which must be at least the length
> + *          of VPORT_CONFIG_SIZE.
> + *
> + * Retrieves the configuration of the given device.  Either RTNL lock or
> + * rcu_read_lock must be held for the entire duration that the
> + * configuration is in use.

This comment isn't accurate: since the caller provides the buffer,
there's no need hold locks after the function call.  The need to
continue holding the lock only applies to cases where a pointer into
an internal data structure is returned.




More information about the dev mailing list