[PATCH 0 of 2] vif hotplug script configuration fixes

[PATCH 0 of 2] vif hotplug script configuration fixes

[Ian Campbell]

The follow short series makes it possible to use alternative vif
hotplug scripts with tap devices. Previously the default vif-bridge
hotplug script would be run for such devices even if /etc/xen/xl.conf,
/etc/xen/xend-config.sxp or the domain cfg said otherwise.

The series also improves xl compatibility with the xm vif script syntax
which does not require the user to enter an absolute path.

[PATCH 1 of 2] libxl: use full path to vif hotplug script script

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1299232872 0
# Node ID 61357fd45fd75880c2683fdc643bf2563659e37e
# Parent  0126a6729e8d8cf19ae5fedab2a0c79570e3ddbb
libxl: use full path to vif hotplug script script.

This improves compatibility with xm styke config files since xend does
this (see NetifController.getDeviceDetails() in
tools/python/xen/xend/server/netif.py) and
tools/hotplug/Linux/vif-setup expects an absolute path to the script.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 0126a6729e8d -r 61357fd45fd7 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Mar 03 16:51:19 2011 +0000
+++ b/tools/libxl/libxl.c	Fri Mar 04 10:01:12 2011 +0000
@@ -1228,7 +1228,8 @@ int libxl_device_nic_add(libxl_ctx *ctx,
     flexarray_append(back, "state");
     flexarray_append(back, libxl__sprintf(&gc, "%d", 1));
     flexarray_append(back, "script");
-    flexarray_append(back, nic->script);
+    flexarray_append(back, libxl__sprintf(&gc, "%s/%s",
+                                                libxl_xen_script_dir_path(), nic->script));
     flexarray_append(back, "mac");
     flexarray_append(back, libxl__sprintf(&gc, "%02x:%02x:%02x:%02x:%02x:%02x",
                                                  nic->mac[0], nic->mac[1], nic->mac[2],

Recent xl and network-route scripts

[W. Michael Petullo]

I've been testing recent Xen 4.1 release candidates, and it seems that
"xl" does not want to play nicely with the vif-route script.

First, the udev subsystem executes vif-setup after Xen creates the
vif device. For some reason, this script does not have $script defined
and so it defaults to running vif-bridge (even though xend-config.sxp
specifies vif-route). Once I changed vif-setup by hand to run vif-route
things went further. I'm not entirely sure how $script is supposed to
be set, but it worked fine with xm.

Next, vif-route does not pick up $ip from my domain configuration's
"vif = [ 'ip=w.x.y.z' ]." When using xm, vif-route finds $ip set and
creates a route based on its value.

I have most recently tested this with Xen 4.1.0 RC4.

-- 
Mike

:wq

Re: Recent xl and network-route scripts

[Pasi Kärkkäinen]

On Wed, Feb 16, 2011 at 07:13:31PM -0600, W. Michael Petullo wrote:
> I've been testing recent Xen 4.1 release candidates, and it seems that
> "xl" does not want to play nicely with the vif-route script.
> 
> First, the udev subsystem executes vif-setup after Xen creates the
> vif device. For some reason, this script does not have $script defined
> and so it defaults to running vif-bridge (even though xend-config.sxp
> specifies vif-route).

I don't think xl/libxl reads/parses xend-config.sxp .. 

> Once I changed vif-setup by hand to run vif-route
> things went further. I'm not entirely sure how $script is supposed to
> be set, but it worked fine with xm.
> 
> Next, vif-route does not pick up $ip from my domain configuration's
> "vif = [ 'ip=w.x.y.z' ]." When using xm, vif-route finds $ip set and
> creates a route based on its value.
> 
> I have most recently tested this with Xen 4.1.0 RC4.
> 

-- Pasi

Re: Recent xl and network-route scripts

[Ian Campbell]

On Thu, 2011-02-17 at 07:29 +0000, Pasi Kärkkäinen wrote:
> On Wed, Feb 16, 2011 at 07:13:31PM -0600, W. Michael Petullo wrote:
> > I've been testing recent Xen 4.1 release candidates, and it seems that
> > "xl" does not want to play nicely with the vif-route script.
> > 
> > First, the udev subsystem executes vif-setup after Xen creates the
> > vif device. For some reason, this script does not have $script defined
> > and so it defaults to running vif-bridge (even though xend-config.sxp
> > specifies vif-route).
> 
> I don't think xl/libxl reads/parses xend-config.sxp .. 

There is an /etc/xl.conf but I don't believe it contains a setting for
the default vif script. It probably could/should though.

As a workaround I believe adding ",script=foo" to the vif entry might
work?

> > Once I changed vif-setup by hand to run vif-route
> > things went further. I'm not entirely sure how $script is supposed to
> > be set, but it worked fine with xm.
> > 
> > Next, vif-route does not pick up $ip from my domain configuration's
> > "vif = [ 'ip=w.x.y.z' ]." When using xm, vif-route finds $ip set and
> > creates a route based on its value.

It looks like xl parses the ip option but that libxl doesn't do anything
with it.

I'll see if I can out a patch together for both of these issues shortly.

Ian.

Re: Recent xl and network-route scripts

[Ian Campbell]

On Thu, 2011-02-17 at 09:19 +0000, Ian Campbell wrote:
> I'll see if I can out a patch together for both of these issues shortly.

Does this work for you?

I don't have a routed setup handy to try it myself but I have verified
that the things appear in xenstore as I would expect.

You will need to edit/create /etc/xen/xl.conf and add:
	vifscript = "vif-route"

You will also need to ensure your host network is setup appropriately
since one difference between xm and xl is that xl does not attempt to
reconfigure the host network instead leaving this up to distro provided
tools etc.

Looking at the network-route script it seems that it should be
sufficient to (assuming eth0 is your physical device):
	echo 1 >/proc/sys/net/ipv4/ip_forward
	echo 1 >/proc/sys/net/ipv4/conf/eth0/proxy_arp
which I think is equivalent to adding the following to /etc/sysctl.conf
and running "sysctl -f" (this method will persist over reboot too)
	sys.net.ipv4.ip_forward = 1
	sys.net.ipv4.conf.eth0.proxy_arp = 1

(Pasi, Todd: this would be a useful addition to the
http://wiki.xensource.com/xenwiki/MigrationGuideToXen4.1+
page)

If this doesn't help then it would be useful to see the result of 
	xenstore-ls /local/domain/0/backends/vif/<domid>/0
	echo
	xenstore-ls /local/domain/<domid>/device/vif/0
when the domain is running from both xl and xm.

Ian.

8<------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1297936972 0
# Node ID c529d4d7d79ba3fe69ffb6cbacac5c9e3e5cf246
# Parent  1728ed4bbec9e82ca13c2639c8e4ef8b4dc231b6
libxl/xl: enable support for routed network configurations.

Add "vifscript" option to xl.conf which configures the default vif
script to use (default remains "vif-bridge")

Write each VIFs "ip" option to xenstore so the vif-route script can
pick it up.

Reported by W. Michael Petullo <mike@flyn.org>.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 1728ed4bbec9 -r c529d4d7d79b tools/examples/xl.conf
--- a/tools/examples/xl.conf	Wed Feb 16 11:47:54 2011 +0000
+++ b/tools/examples/xl.conf	Thu Feb 17 10:02:52 2011 +0000
@@ -6,3 +6,6 @@ autoballoon=0
 
 # full path of the lockfile used by xl during domain creation
 #lockfile="/var/lock/xl"
+
+# default vif script 
+#vifscript="vif-bridge"
diff -r 1728ed4bbec9 -r c529d4d7d79b tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Feb 16 11:47:54 2011 +0000
+++ b/tools/libxl/libxl.c	Thu Feb 17 10:02:52 2011 +0000
@@ -30,6 +30,8 @@
 #include <stdint.h>
 #include <inttypes.h>
 #include <assert.h>
+
+#include <arpa/inet.h>
 
 #include "libxl.h"
 #include "libxl_utils.h"
@@ -1123,6 +1125,7 @@ int libxl_device_nic_init(libxl_device_n
     nic_info->mac[5] = r[2];
     nic_info->ifname = NULL;
     nic_info->bridge = strdup("xenbr0");
+    nic_info->ip.s_addr = 0UL;
     if ( asprintf(&nic_info->script, "%s/vif-bridge",
                libxl_xen_script_dir_path()) < 0 )
         return ERROR_FAIL;
@@ -1182,6 +1185,16 @@ int libxl_device_nic_add(libxl_ctx *ctx,
     flexarray_append(back, libxl__sprintf(&gc, "%02x:%02x:%02x:%02x:%02x:%02x",
                                                  nic->mac[0], nic->mac[1], nic->mac[2],
                                                  nic->mac[3], nic->mac[4], nic->mac[5]));
+    if (nic->ip.s_addr != 0UL) {
+        char dst[INET_ADDRSTRLEN];
+        const char *addr = inet_ntop(AF_INET, &nic->ip.s_addr, &dst[0], INET_ADDRSTRLEN);
+        if (addr) {
+            flexarray_append(back, "ip");
+            flexarray_append(back, libxl__strdup(&gc, addr));
+        } else {
+            LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "Unable to format IP address");
+        }
+    }
     flexarray_append(back, "bridge");
     flexarray_append(back, libxl__strdup(&gc, nic->bridge));
     flexarray_append(back, "handle");
diff -r 1728ed4bbec9 -r c529d4d7d79b tools/libxl/xl.c
--- a/tools/libxl/xl.c	Wed Feb 16 11:47:54 2011 +0000
+++ b/tools/libxl/xl.c	Thu Feb 17 10:02:52 2011 +0000
@@ -35,6 +35,7 @@ xentoollog_logger_stdiostream *logger;
 xentoollog_logger_stdiostream *logger;
 int autoballoon = 1;
 char *lockfile;
+char *default_vifscript = NULL;
 
 static xentoollog_level minmsglevel = XTL_PROGRESS;
 
@@ -71,6 +72,9 @@ static void parse_global_config(const ch
             exit(1);
         }
     }
+
+    if (!xlu_cfg_get_string (config, "vifscript", &buf))
+        default_vifscript = strdup(buf);
 
     xlu_cfg_destroy(config);
 }
diff -r 1728ed4bbec9 -r c529d4d7d79b tools/libxl/xl.h
--- a/tools/libxl/xl.h	Wed Feb 16 11:47:54 2011 +0000
+++ b/tools/libxl/xl.h	Thu Feb 17 10:02:52 2011 +0000
@@ -101,5 +101,6 @@ extern xentoollog_logger_stdiostream *lo
 /* global options */
 extern int autoballoon;
 extern char *lockfile;
+extern char *default_vifscript;
 
 #endif /* XL_H */
diff -r 1728ed4bbec9 -r c529d4d7d79b tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Feb 16 11:47:54 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Thu Feb 17 10:02:52 2011 +0000
@@ -782,6 +782,11 @@ static void parse_config_data(const char
             d_config->vifs = (libxl_device_nic *) realloc(d_config->vifs, sizeof (libxl_device_nic) * (d_config->num_vifs+1));
             nic = d_config->vifs + d_config->num_vifs;
             CHK_ERRNO( libxl_device_nic_init(nic, d_config->num_vifs) );
+
+            if (default_vifscript) {
+                free(nic->script);
+                nic->script = strdup(default_vifscript);
+            }
 
             p = strtok(buf2, ",");
             if (!p)

Re: Recent xl and network-route scripts

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] Recent xl and network-route scripts"):
> libxl/xl: enable support for routed network configurations.

Applied, thanks (sorry for apparently missing this one yesterday).

Ian.

Re: Recent xl and network-route scripts

[W. Michael Petullo]

>> libxl/xl: enable support for routed network configurations.
 
> Applied, thanks (sorry for apparently missing this one yesterday).

I finally had a change to test this and things work well. However,
may I suggest changing the default xl.conf to:

	#vifscript="/etc/xen/scripts/vif-bridge"

instead of:

	#vifscript="vif-bridge"

The former pattern is what "vif-setup" needs.

-- 
Mike

:wq

Re: Recent xl and network-route scripts

[Ian Campbell]

On Sun, 2011-03-13 at 18:53 +0000, W. Michael Petullo wrote:
> >> libxl/xl: enable support for routed network configurations.
>  
> > Applied, thanks (sorry for apparently missing this one yesterday).
> 
> I finally had a change to test this and things work well. However,
> may I suggest changing the default xl.conf to:
> 
> 	#vifscript="/etc/xen/scripts/vif-bridge"
> 
> instead of:
> 
> 	#vifscript="vif-bridge"
> 
> The former pattern is what "vif-setup" needs.

Thanks, I sent out a patch last week "libxl: use full path to vif
hotplug script script" which addresses this discrepancy.

I thought it has gone it but evidently not. The patch was part 1 of 2 in
a series, I don't think 2/2 is suitable for 4.1 (which is likely why the
whole series was missed), but the first patch most probably is.

Ian.

Re: Recent xl and network-route scripts

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] Recent xl and network-route scripts"):
> On Sun, 2011-03-13 at 18:53 +0000, W. Michael Petullo wrote:
> > The former pattern is what "vif-setup" needs.
> 
> Thanks, I sent out a patch last week "libxl: use full path to vif
> hotplug script script" which addresses this discrepancy.

I think we should _allow_ the user to specify the complete path to a
script, but not require them to.  Your existing patch 1/2 doesn't do
that.

How about something like:

-    flexarray_append(back, nic->script);
+    flexarray_append(back, nic->script[0]=='/' ? nic->script :
+     libxl__sprintf(&gc, "%s/%s", libxl_xen_script_dir_path(), nic->script));

?

> I thought it has gone it but evidently not. The patch was part 1 of 2 in
> a series, I don't think 2/2 is suitable for 4.1 (which is likely why the
> whole series was missed), but the first patch most probably is.

I didn't realise you intended it for 4.1, but I think this is an
important enough compatibility problem that we should fix it along the
lines I suggest above.

Ian.

Re: Recent xl and network-route scripts

[Ian Campbell]

On Tue, 2011-03-15 at 18:12 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] Recent xl and network-route scripts"):
> > On Sun, 2011-03-13 at 18:53 +0000, W. Michael Petullo wrote:
> > > The former pattern is what "vif-setup" needs.
> > 
> > Thanks, I sent out a patch last week "libxl: use full path to vif
> > hotplug script script" which addresses this discrepancy.
> 
> I think we should _allow_ the user to specify the complete path to a
> script, but not require them to.  Your existing patch 1/2 doesn't do
> that.

It's over and above whatr xend supports but I agree it's a good idea.

> How about something like:
> 
> -    flexarray_append(back, nic->script);
> +    flexarray_append(back, nic->script[0]=='/' ? nic->script :
> +     libxl__sprintf(&gc, "%s/%s", libxl_xen_script_dir_path(), nic->script));

Assuming there's an "if (nic-script)" above this context I think makes
sense.

Ian

Re: Recent xl and network-route scripts [and 1 more messages]

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] Recent xl and network-route scripts"):
> On Tue, 2011-03-15 at 18:12 +0000, Ian Jackson wrote:
> > How about something like:
> > 
> > -    flexarray_append(back, nic->script);
> > +    flexarray_append(back, nic->script[0]=='/' ? nic->script :
> > +     libxl__sprintf(&gc, "%s/%s", libxl_xen_script_dir_path(), nic->script));

I've applied the patch below.

> Assuming there's an "if (nic-script)" above this context I think makes
> sense.

There wasn't, so I added one.  Looking at the code I think it's not
necessary but now is not the time to take the risk ...

Ian.

# HG changeset patch
# User Ian Jackson <Ian.Jackson@eu.citrix.com>
# Date 1300293865 0
# Node ID 4ca13a170482edc642b8a7d8c68556c4194f11d0
# Parent  8074dff66e9791cef6be06dd9a24176a59f88c5e
libxl: provide full path to vif hotplug script script

This improves compatibility with xm style config files since xend does
this (see NetifController.getDeviceDetails() in
tools/python/xen/xend/server/netif.py) and
tools/hotplug/Linux/vif-setup expects an absolute path to the script.

This patch supports both absolute and relative paths in the
configuration.  Also, if somehow nic->script==NULL, don't add write
the entry to xenstore at all (rather than crashing due to the new
check for absolute vs relative path).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff -r 8074dff66e97 -r 4ca13a170482 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Mar 16 09:35:32 2011 +0000
+++ b/tools/libxl/libxl.c	Wed Mar 16 16:44:25 2011 +0000
@@ -1234,8 +1234,13 @@ int libxl_device_nic_add(libxl_ctx *ctx,
     flexarray_append(back, "1");
     flexarray_append(back, "state");
     flexarray_append(back, libxl__sprintf(&gc, "%d", 1));
-    flexarray_append(back, "script");
-    flexarray_append(back, nic->script);
+    if (nic->script) {
+        flexarray_append(back, "script");
+        flexarray_append(back, nic->script[0]=='/' ? nic->script
+                         : libxl__sprintf(&gc, "%s/%s",
+                                          libxl_xen_script_dir_path(),
+                                          nic->script));
+    }
     flexarray_append(back, "mac");
     flexarray_append(back, libxl__sprintf(&gc, "%02x:%02x:%02x:%02x:%02x:%02x",
                                                  nic->mac[0], nic->mac[1], nic->mac[2],

[PATCH 2 of 2] tools/hotplug/Linux: read script for tapX.Y devices from xenstore

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1299232872 0
# Node ID 37b2d5372baab8b2fac860f9a9fff7348f74153c
# Parent  61357fd45fd75880c2683fdc643bf2563659e37e
tools/hotplug/Linux: read script for tapX.Y devices from xenstore.

This variable is not present in the $environment for tap hotplug events
in the way it is for vif hotplug events. Therefore we need to look up
the script in xenbus.

vif-common.sh already takes care of most of the environmental
differences but we need the script much earlier.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 61357fd45fd7 -r 37b2d5372baa tools/hotplug/Linux/vif-setup
--- a/tools/hotplug/Linux/vif-setup	Fri Mar 04 10:01:12 2011 +0000
+++ b/tools/hotplug/Linux/vif-setup	Fri Mar 04 10:01:12 2011 +0000
@@ -1,4 +1,18 @@
 #!/bin/bash
+
+# tap devices do not get the same environment as vif devices so we may
+# need to discover the script from xenbus.
+#
+# Try and only match the tap<domid>.<devid> naming scheme which Xen
+# uses, to avoid interfering with other uses of tap devices.
+if [ -z "$script" ] && echo $* | grep -qw type_if=tap ; then
+    xbpath=$(echo ${INTERFACE} | sed -ne 's,tap\([0-9][0-9]\?\)\.\([0-9][0-9]\?\),backend/vif/\1/\2,p')
+    if [ -n "$xbpath" ] ; then
+	script=`xenstore-read "${xbpath}/script"`
+    fi
+
+    export script
+fi
 
 if test "$script"
 then