[ovs-dev] [PATCH v4] netdev-dpdk: add dpdk vhost-user ports

[Panu Matilainen]

On 05/13/2015 04:15 PM, Ciara Loftus wrote:
> This patch adds support for a new port type to the userspace
> datapath called dpdkvhostuser.
>
> A new dpdkvhostuser port will create a unix domain socket which
> when provided to QEMU is used to facilitate communication between
> the virtio-net device on the VM and the OVS port on the host.
>
> vhost-cuse ('dpdkvhost') ports are still available, and will be
> enabled if vhost-cuse support is detected in the DPDK build
> specified during compilation of the switch. Otherwise, vhost-user
> ports are enabled.
>
> v4:
> - Included helper function for the new_device callbacks to minimise
> code duplication.
> - Fixed indentation & line-wrap.
> - Simplified and corrected the processing of vhost ovs-vswitchd flags.
>
> Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>
[...]
> @@ -1896,6 +1960,32 @@ unlock_dpdk:
>       NULL,                       /* rxq_drain */               \
>   }
>
> +static int
> +process_vhost_flags(char* flag, char* default_val, int size, char** argv, char** new_val)
> +{
> +    int changed = 0;
> +
> +    /* Depending on which version of vhost is in use, process the vhost-specific
> +     * flag if it is provided on the vswitchd command line, otherwise resort to
> +     * a default value.
> +     *
> +     * For vhost-user: Process "--cuse_dev_name" to set the custom location of
> +     * the vhost-user socket(s).
> +     * For vhost-cuse: Process "--vhost_sock_dir" to set the custom name of the
> +     * vhost-cuse character device.
> +     */
> +    if (!strcmp(argv[1], flag) && (strlen(argv[2]) <= size)) {
> +        *new_val = strdup(argv[2]);
> +        VLOG_INFO("User-provided %s in use: %s", flag, *new_val);
> +        changed = 1;
> +    } else {
> +        *new_val = strdup(default_val);
> +        VLOG_INFO("No %s provided - defaulting to %s", flag, default_val);
> +    }
> +
> +    return changed;
> +}
> +
>   int
>   dpdk_init(int argc, char **argv)
>   {
> @@ -1910,27 +2000,20 @@ dpdk_init(int argc, char **argv)
>       argc--;
>       argv++;
>
> -    /* If the cuse_dev_name parameter has been provided, set 'cuse_dev_name' to
> -     * this string if it meets the correct criteria. Otherwise, set it to the
> -     * default (vhost-net).
> -     */
> -    if (!strcmp(argv[1], "--cuse_dev_name") &&
> -        (strlen(argv[2]) <= NAME_MAX)) {
> -
> -        cuse_dev_name = strdup(argv[2]);
> -
> -        /* Remove the cuse_dev_name configuration parameters from the argument
> +#ifdef VHOST_CUSE
> +    if(process_vhost_flags("--cuse_dev_name", strdup("vhost-net"),
> +            PATH_MAX, argv, &cuse_dev_name)) {
> +#else
> +    if (process_vhost_flags("--vhost_sock_dir", strdup(ovs_rundir()),
> +            NAME_MAX, argv, &vhost_sock_dir)) {
> +#endif

This introduces a tiny memory leak as process_vhost_flags() also 
strdup()'s the argument. Not that it really matters for functionality or 
anything, just a matter of being clean. Also the cuse-if is missing a 
space after it, while we're down to nitpicking.

I'd prefer the memleak fixed but other than that ACK from me as well.

	- Panu -