Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
JM> However, argv is one place where it pays to relax JM> const-correctness guidelines, at least in C, because so many JM> interfaces require non-const "char **argv" pointers. IMHO, it's JM> better to avoid const altogether in this limited case than to be JM> forced to litter the code with ugly and dangerous const-adjusting JM> casts to accommodate "correctness". I agree with the last statement, but in this case, leaving the const off the declaration results in a compiler warning about discarding the inherent const qualifier from a literal string. A *very* quick look in src/*.c doesn't reveal any other examples of all-literal argv[] lists like this. I'm inclined to leave the const and the cast, but if you have a better suggestion, I'll be glad to make the change. Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: [EMAIL PROTECTED] pgpBGD38giQUJ.pgp Description: PGP signature -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
Daniel Veillard <[EMAIL PROTECTED]> wrote:
...
>> +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
You're right that I encourage the "const correct" approach ;-)
To tell the compiler (and more importantly, the reader) that you
have a const array of const strings, you'd declare it like this:
const char *const argv[] = {
or, equivalently, the const can go after the type name of "char":
char const *const argv[] = {
However, argv is one place where it pays to relax const-correctness
guidelines, at least in C, because so many interfaces require non-const
"char **argv" pointers. IMHO, it's better to avoid const altogether in
this limited case than to be forced to litter the code with ugly and
dangerous const-adjusting casts to accommodate "correctness".
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
Dan Smith wrote:
>>> +if (1 > strlen(veth1)) {
>
> DL> Why do you check with strlen > 1 ?
>
> To be honest, I have no idea why Dave did that. I'll check with him
> to see if there's something we're missing. It's probably worth a
> comment if so.
>
So that's checking if veth1 is an empty string and hence needs to have a name
assigned. I don't recall any special reason for using (1 > strlen()).
--
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
>> +#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
>> +#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
DL> Do you know ##__VA_ARGS ?
Yes, and I agree this not my preferred way. However, this isn't my
code and when I went to look for other instances, I found many so I
decided not to change it.
% cat *.c | grep DEBUG0 -c
47
DL> Is this function safe for concurrent access ? eg. getFreeVethName
DL> called in parallel by two processes or another process creates a
DL> pair device just after you exit the loop ?
No, there is indeed a race. However, since this just makes a
suggestion and relies on the subsequent call to try to instantiate the
interface, I don't think there's a chance for anything other than a
spurious failure.
This is specifically why I wanted to get this on the list sooner than
later. I figured (or hoped) that given the experience in this group
that someone could offer some educated suggestions before I jumped in
and started trying to fix it.
>> +if (1 > strlen(veth1)) {
DL> Why do you check with strlen > 1 ?
To be honest, I have no idea why Dave did that. I'll check with him
to see if there's something we're missing. It's probably worth a
comment if so.
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: [EMAIL PROTECTED]
pgpP69YKohLy3.pgp
Description: PGP signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
Dan Smith wrote:
# HG changeset patch
# User Dave Leskovec <[EMAIL PROTECTED]>
# 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.
diff -r 8d2afc533c91 -r 386c067de899 configure.in
--- a/configure.in Tue Jun 17 15:55:03 2008 +
+++ 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
dnl Need to test if pkg-config exists
diff -r 8d2afc533c91 -r 386c067de899 src/Makefile.am
--- a/src/Makefile.am Tue Jun 17 15:55:03 2008 +
+++ b/src/Makefile.am Thu Jun 19 08:59:24 2008 -0700
@@ -64,6 +64,7 @@
lxc_driver.c lxc_driver.h \
lxc_conf.c lxc_conf.h \
lxc_container.c lxc_container.h \
+ veth.c veth.h \
nodeinfo.h nodeinfo.c \
util.c util.h
diff -r 8d2afc533c91 -r 386c067de899 src/veth.c
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/src/veth.cThu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,247 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.c: file description
+ *
+ * Authors:
+ * David L. Leskovec
+ *
+ * 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
+ */
+
+#include
+
+#ifdef HAVE_NETNS
+
+#include
+
+#include "veth.h"
+#include "internal.h"
+#include "memory.h"
+#include "util.h"
+
+#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
+#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
Do you know ##__VA_ARGS ?
+/* Functions */
+/**
+ * getFreeVethName:
+ * @veth: name for veth device (NULL to find first open)
+ * @maxLen: max length of veth name
+ * @startDev: device number to start at (x in vethx)
+ *
+ * Looks in /sys/class/net/ to find the first available veth device
+ * name.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+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);
You can perhaps, use do { ... } while () here.
+while (virFileExists(path)) {
+++devNum;
+sprintf(path, "/sys/class/net/veth%d/", devNum);
+}
Is this function safe for concurrent access ? eg. getFreeVethName called
in parallel by two processes or another process creates a pair device
just after you exit the loop ?
+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 f
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
On Thu, Jun 19, 2008 at 10:46:40AM -0700, Dan Smith wrote: > DV> Hum, it's not only a kernel feature, it looks more like a > DV> dependancy on iproute, maybe that can be checked at runtime when > DV> initializing the lxc module, no ? > > I suppose I could actually let it fail when it tries to do the actual > move. Would you rather a specific test during the define stage to > reject the if the non-netns 'ip' command is found? Well i was wondering if you could not try to detect if the feature is available for example in lxcProbe() then keep the status. > DV> yeah I'm getting very confused by that code :-) > > Agreed, I'll fix it up :) okay :-) > DV> as the header template for the two new files to match the other files. > DV> COPYING.LIB is of course LGPL 2.1 > > Okay, I'll change it. Cool, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
DS> # HG changeset patch DS> # User Dave Leskovec <[EMAIL PROTECTED]> DS> # Date 1213891164 25200 DS> # Node ID 386c067de8995028dd11f70602081c31682dd293 DS> # Parent 8d2afc533c91c4796512e1e71c8283e86eafd18a DS> [LXC] Add functions to manage veth device pairs My apologies for the HG headers. I forgot the --plain when I sent it out... -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: [EMAIL PROTECTED] pgpQ4fsElwxWG.pgp Description: PGP signature -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
DV> Hum, it's not only a kernel feature, it looks more like a
DV> dependancy on iproute, maybe that can be checked at runtime when
DV> initializing the lxc module, no ?
I suppose I could actually let it fail when it tries to do the actual
move. Would you rather a specific test during the define stage to
reject the if the non-netns 'ip' command is found?
>> +if (0 != VIR_ALLOC_N(argv[pidArgvOffset], (sizeof(int) * 3) + 1)) {
>> +goto error_out;
>> +}
DV>Hum, here i don't understand, if argv is defined as local stack data,
DV> I find a bit confusing to use it in the argument for VIR_ALLOC_N. I would
DV> use an intermediate local variable to make this cleaner/ easier to read.
>> +len = snprintf(argv[pidArgvOffset], (sizeof(int) * 3) + 1, "%d",
>> pidInNs);
DV> yeah I'm getting very confused by that code :-)
Agreed, I'll fix it up :)
DV> Oh can you use
DV> /*
DV> * veth.h: a proper file description :-)
DV> *
DV> * Copyright IBM Corp. 2008
DV> *
DV> * See COPYING.LIB for the License of this software
DV> *
DV> * Authors:
DV> * David L. Leskovec
DV> */
DV> as the header template for the two new files to match the other files.
DV> COPYING.LIB is of course LGPL 2.1
Okay, I'll change it.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: [EMAIL PROTECTED]
pgpS5wIMskw77.pgp
Description: PGP signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 <[EMAIL PROTECTED]>
> # 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.inTue Jun 17 15:55:03 2008 +
> +++ b/configure.inThu 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:
[libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
# HG changeset patch
# User Dave Leskovec <[EMAIL PROTECTED]>
# 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.
diff -r 8d2afc533c91 -r 386c067de899 configure.in
--- a/configure.in Tue Jun 17 15:55:03 2008 +
+++ 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
dnl Need to test if pkg-config exists
diff -r 8d2afc533c91 -r 386c067de899 src/Makefile.am
--- a/src/Makefile.am Tue Jun 17 15:55:03 2008 +
+++ b/src/Makefile.am Thu Jun 19 08:59:24 2008 -0700
@@ -64,6 +64,7 @@
lxc_driver.c lxc_driver.h \
lxc_conf.c lxc_conf.h \
lxc_container.c lxc_container.h \
+ veth.c veth.h \
nodeinfo.h nodeinfo.c \
util.c util.h
diff -r 8d2afc533c91 -r 386c067de899 src/veth.c
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/src/veth.cThu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,247 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.c: file description
+ *
+ * Authors:
+ * David L. Leskovec
+ *
+ * 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
+ */
+
+#include
+
+#ifdef HAVE_NETNS
+
+#include
+
+#include "veth.h"
+#include "internal.h"
+#include "memory.h"
+#include "util.h"
+
+#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
+#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
+
+/* Functions */
+/**
+ * getFreeVethName:
+ * @veth: name for veth device (NULL to find first open)
+ * @maxLen: max length of veth name
+ * @startDev: device number to start at (x in vethx)
+ *
+ * Looks in /sys/class/net/ to find the first available veth device
+ * name.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+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);
+}
+
+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[] = {
+"
