[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs



On Thu, Jun 19, 2008 at 08:59:52AM -0700, Dan Smith wrote:
> # HG changeset patch
> # User Dave Leskovec <dlesko linux vnet ibm com>
> # Date 1213891164 25200
> # Node ID 386c067de8995028dd11f70602081c31682dd293
> # Parent  8d2afc533c91c4796512e1e71c8283e86eafd18a
> [LXC] Add functions to manage veth device pairs
> 
> This gives us the ability to create a veth pair so that we can move one
> into the network namespace of an LXC container.

  Basically it's executing ip commands, so the real code changes are outside
of libvirt.

> diff -r 8d2afc533c91 -r 386c067de899 configure.in
> --- a/configure.in	Tue Jun 17 15:55:03 2008 +0000
> +++ b/configure.in	Thu Jun 19 08:59:24 2008 -0700
> @@ -301,6 +301,20 @@
>  if test "$with_qemu" = "yes" ; then
>    AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],,
>                     AC_MSG_ERROR([You must install kernel-headers in order to compile libvirt]))
> +fi
> +
> +dnl
> +dnl check for patched iproute2 for lxc network support
> +dnl
> +if test "$with_lxc" = "yes" ; then
> +   AC_MSG_CHECKING([for NETNS support])
> +   if ip link help 2>&1 | grep -q netns; then
> +     with_lxc_netns="yes"
> +     AC_DEFINE([HAVE_NETNS], [], [Kernel has NETNS support])
> +   else
> +     with_lxc_netns="no"
> +   fi
> +   AC_MSG_RESULT($with_lxc_netns)
>  fi

 Hum, it's not only a kernel feature, it looks more like a dependancy on
iproute, maybe that can be checked at runtime when initializing the lxc 
module, no ?

[...]
> +static int getFreeVethName(char *veth, int maxLen, int startDev)
> +{
> +    int rc = -1;
> +    int devNum = startDev;
> +    char path[PATH_MAX];
> +
> +    snprintf(path, PATH_MAX, "/sys/class/net/veth%d/", devNum);
> +    while (virFileExists(path)) {
> +        ++devNum;
> +        sprintf(path, "/sys/class/net/veth%d/", devNum);

  use snprintf there too

> +    }
> +
> +    snprintf(veth, maxLen, "veth%d", devNum);
> +
> +    rc = devNum;
> +
> +    return rc;
> +}
> +
> +/**
> + * vethCreate:
> + * @veth1: name for one end of veth pair
> + * @veth1MaxLen: max length of veth1 name
> + * @veth2: name for one end of veth pair
> + * @veth2MaxLen: max length of veth1 name
> + *
> + * Creates a veth device pair using the ip command:
> + * ip link add veth1 type veth peer name veth2
> + * NOTE: If veth1 and veth2 names are not specified, ip will auto assign
> + *       names.  There seems to be two problems here -
> + *       1) There doesn't seem to be a way to determine the names of the
> + *          devices that it creates.  They show up in ip link show and
> + *          under /sys/class/net/ however there is no guarantee that they
> + *          are the devices that this process just created.
> + *       2) Once one of the veth devices is moved to another namespace, it
> + *          is no longer visible in the parent namespace.  This seems to
> + *          confuse the name assignment causing it to fail with File exists.
> + *       Because of these issues, this function currently forces the caller
> + *       to fully specify the veth device names.
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +int vethCreate(char* veth1, int veth1MaxLen,
> +               char* veth2, int veth2MaxLen)
> +{
> +    int rc = -1;
> +    const char *argv[] = {

I think Jim insists on having const char const * [], but I bet he will explain
this better than me :-)

> +        "ip", "link", "add", veth1, "type", "veth", "peer", "name", veth2, NULL
> +    };
[...]
> +/**
> + * moveInterfaceToNetNs:
> + * @interface: name of device
> + * @pidInNs: PID of process in target net namespace
> + *
> + * Moves the given device into the target net namespace specified by the given
> + * pid using this command:
> + *     ip link set interface netns pidInNs
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +int moveInterfaceToNetNs(const char* interface, int pidInNs)
> +{
> +    int rc;
> +    /* offset of the pid field in the following args */
> +    const int pidArgvOffset = 5;
> +    const char *argv[] = {
> +        "ip", "link", "set", interface, "netns", NULL, NULL
> +    };
> +    int cmdResult;
> +    int len;
> +
> +    if (NULL == interface) {
> +        goto error_out;
> +    }
> +
> +    if (0 != VIR_ALLOC_N(argv[pidArgvOffset], (sizeof(int) * 3) + 1)) {
> +        goto error_out;
> +    }

   Hum, here i don't understand, if argv is defined as local stack data,
I find a bit confusing to use it in the argument for VIR_ALLOC_N. I would
use an intermediate local variable to make this cleaner/ easier to read.

> +    len = snprintf(argv[pidArgvOffset], (sizeof(int) * 3) + 1, "%d", pidInNs);

  yeah I'm getting very confused by that code :-)

> +    if (len >= (sizeof(int) * 3) + 1) {
> +        goto cleanup;
> +    }
> +
> +    rc = virRun(NULL, (char**)argv, &cmdResult);
> +
> +    if (0 == rc) {
> +       rc = cmdResult;
> +    }
> +
> +cleanup:
> +    VIR_FREE(argv[pidArgvOffset]);
> +
> +error_out:
> +    return rc;
> +}
> +
> +#endif /* HAVE_NETNS */
> +
> diff -r 8d2afc533c91 -r 386c067de899 src/veth.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/veth.h	Thu Jun 19 08:59:24 2008 -0700
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright IBM Corp. 2008
> + *
> + * veth.h: file description
> + *
> + * Authors:
> + *  David L. Leskovec <dlesko at linux.vnet.ibm.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */

  Oh can you use

/*
 * veth.h: .... a proper file description :-) ....
 *
 * Copyright IBM Corp. 2008
 *
 * See COPYING.LIB for the License of this software
 *
 * Authors:
 *  David L. Leskovec <dlesko at linux.vnet.ibm.com>
 */

as the header template for the two new files to match the other files.
COPYING.LIB is of course LGPL 2.1


   thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]