Re: [patch] xen udev rule interfering with openvpn

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2012-04-25 at 11:14 +0100, Ian Campbell wrote:
> On Wed, 2012-04-25 at 11:11 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [patch] xen udev rule interfering with openvpn"):
> > > So for vifname="foo" it is a no-brainer to call the vif "foo" and the
> > > emulated tap device "foo-emu".
> > > 
> > > But what about the case where no vifname is given, in that case vif is
> > > named "vif<DOM>.<DEV>". But what to call the tap? Previously I was
> > > changing the name from tap<DOM>.<DEV> to xentap<DOM>.<DEV> but perhaps
> > > now "vif<DOM>.<DEV>-emu" makes more sense/is more consistent?
> > 
> > I think either is fine.  While we're changing it it probably makes
> > sense to use "vif..." as indeed it is more consistent.
> 
> OK, vifX.Y and vifX.Y-emu it is...

This turned out to be a bit more complex than I was expecting, mostly
because the use of "vifname" with tap devices was already broken. I
think this does the right things with each type of device.

Roger, not sure what knock on effect this has on your series. The main
thing is likely to be that you needn't find the vifname since you
actually want the standard name not the user supplied once on most
occasions.

Ian.

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

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1335354957 -3600
# Node ID 44fc76485f797bdd726fed4a2a7c4b06363fdb88
# Parent  a14b1dd0feeee300c37c44564381f4e8ea837c45
libxl/xend: name tap devices vifX.Y-emu

This prevents the udev scripts from operating on other tap devices (e.g.
openvpn etc)

Correct the documentation for the "vifname" option which suggested it applied
to HVM tap devices only, which is not the case.

Reported by Michael Young.

Also fix the use of vifname with emulated devices. This is slightly complex.
The current hotplug scripts rely on being able to parse the "tapX.Y" (now
"vifX.Y-emu") name in order to locate the xenstore backend dir relating to the
corresponding vif. This is because we cannot inject our own environment vars
into the tap hotplug events. However this means that if the tap is initially
named with a user specified name (which will not match the expected scheme) we
fail to do anything useful with the device. So now we create the initial tap
device with the standard "vifX.Y-emu" name and the hotplug script will handle
the rename to the desired name. This is also how PV vif devices work -- they
are always created by netback with the name vifX.Y and renamed in the script.

Lastly also move libxl__device_* to a better place in the header, otherwise the
comment about evgen stuff isn't next to the associated functions (noticed jsut
because I was going to add nic_devname near to the setdefault functions)

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

diff -r a14b1dd0feee -r 44fc76485f79 docs/misc/xl-network-configuration.markdown
--- a/docs/misc/xl-network-configuration.markdown	Wed Apr 25 10:53:52 2012 +0100
+++ b/docs/misc/xl-network-configuration.markdown	Wed Apr 25 12:55:57 2012 +0100
@@ -93,11 +93,14 @@ are:
 
 ### vifname
 
-This keyword is valid for HVM guest devices with `type=ioemu` only.
+Specifies the backend device name for the virtual device.
 
-Specifies the backend device name for an emulated device. The default
-is `tapDOMID.DEVID` where `DOMID` is the guest domain ID and `DEVID`
-is the device number.
+If the domain is an HVM domain then the associated emulated (tap)
+device will have a "-emu" suffice added.
+
+The default name for the virtual device is `vifDOMID.DEVID` where
+`DOMID` is the guest domain ID and `DEVID` is the device
+number. Likewise the default tap name is `vifDOMID.DEVID-emu`.
 
 ### script
 
diff -r a14b1dd0feee -r 44fc76485f79 tools/hotplug/Linux/vif-common.sh
--- a/tools/hotplug/Linux/vif-common.sh	Wed Apr 25 10:53:52 2012 +0100
+++ b/tools/hotplug/Linux/vif-common.sh	Wed Apr 25 12:55:57 2012 +0100
@@ -85,12 +85,23 @@ elif [ "$type_if" = tap ]; then
     : ${INTERFACE:?}
 
     # Get xenbus_path from device name.
-    # The name is built like that: "tap${domid}.${devid}".
-    dev_=${dev#tap}
+    # The name is built like that: "vif${domid}.${devid}-emu".
+    dev_=${dev#vif}
+    dev_=${dev_%-emu}
     domid=${dev_%.*}
     devid=${dev_#*.}
 
     XENBUS_PATH="/local/domain/0/backend/vif/$domid/$devid"
+    vifname=$(xenstore_read_default "$XENBUS_PATH/vifname" "")
+    if [ "$vifname" ]
+    then
+        vifname="${vifname}-emu"
+        if [ "$command" == "add" ] && ! ip link show "$vifname" >&/dev/null
+        then
+            do_or_die ip link set "$dev" name "$vifname"
+        fi
+        dev="$vifname"
+    fi
 fi
 
 ip=${ip:-}
diff -r a14b1dd0feee -r 44fc76485f79 tools/hotplug/Linux/xen-backend.rules
--- a/tools/hotplug/Linux/xen-backend.rules	Wed Apr 25 10:53:52 2012 +0100
+++ b/tools/hotplug/Linux/xen-backend.rules	Wed Apr 25 12:55:57 2012 +0100
@@ -13,4 +13,4 @@ KERNEL=="blktap-control", NAME="xen/blkt
 KERNEL=="gntdev", NAME="xen/%k", MODE="0600"
 KERNEL=="pci_iomul", NAME="xen/%k", MODE="0600"
 KERNEL=="tapdev[a-z]*", NAME="xen/blktap-2/tapdev%m", MODE="0600"
-SUBSYSTEM=="net", KERNEL=="tap*", ACTION=="add", RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
+SUBSYSTEM=="net", KERNEL=="vif*-emu", ACTION=="add", RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
diff -r a14b1dd0feee -r 44fc76485f79 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Apr 25 10:53:52 2012 +0100
+++ b/tools/libxl/libxl.c	Wed Apr 25 12:55:57 2012 +0100
@@ -2095,6 +2095,21 @@ int libxl_device_nic_getinfo(libxl_ctx *
     return 0;
 }
 
+const char *libxl__device_nic_devname(libxl__gc *gc,
+                                      uint32_t domid,
+                                      uint32_t devid,
+                                      libxl_nic_type type)
+{
+    switch (type) {
+    case LIBXL_NIC_TYPE_VIF:
+        return GCSPRINTF("vif%u.%d", domid, devid);
+    case LIBXL_NIC_TYPE_IOEMU:
+        return GCSPRINTF("vif%u.%d" TAP_DEVICE_SUFFIX, domid, devid);
+    default:
+        abort();
+    }
+}
+
 /******************************************************************************/
 int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                               libxl__device_console *console,
diff -r a14b1dd0feee -r 44fc76485f79 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Wed Apr 25 10:53:52 2012 +0100
+++ b/tools/libxl/libxl_dm.c	Wed Apr 25 12:55:57 2012 +0100
@@ -209,12 +209,9 @@ static char ** libxl__build_device_model
             if (vifs[i].nictype == LIBXL_NIC_TYPE_IOEMU) {
                 char *smac = libxl__sprintf(gc,
                                    LIBXL_MAC_FMT, LIBXL_MAC_BYTES(vifs[i].mac));
-                char *ifname;
-                if (!vifs[i].ifname)
-                    ifname = libxl__sprintf(gc,
-                                            "tap%d.%d", domid, vifs[i].devid);
-                else
-                    ifname = vifs[i].ifname;
+                const char *ifname = libxl__device_nic_devname(gc,
+                                                domid, vifs[i].devid,
+                                                LIBXL_NIC_TYPE_IOEMU);
                 flexarray_vappend(dm_args,
                                 "-net", libxl__sprintf(gc, "nic,vlan=%d,macaddr=%s,model=%s",
                                                        vifs[i].devid, smac, vifs[i].model),
@@ -449,13 +446,9 @@ static char ** libxl__build_device_model
             if (vifs[i].nictype == LIBXL_NIC_TYPE_IOEMU) {
                 char *smac = libxl__sprintf(gc,
                                 LIBXL_MAC_FMT, LIBXL_MAC_BYTES(vifs[i].mac));
-                char *ifname;
-                if (!vifs[i].ifname) {
-                    ifname = libxl__sprintf(gc, "tap%d.%d",
-                                            guest_domid, vifs[i].devid);
-                } else {
-                    ifname = vifs[i].ifname;
-                }
+                const char *ifname = libxl__device_nic_devname(gc,
+                                                guest_domid, vifs[i].devid,
+                                                LIBXL_NIC_TYPE_IOEMU);
                 flexarray_append(dm_args, "-device");
                 flexarray_append(dm_args,
                    libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s",
diff -r a14b1dd0feee -r 44fc76485f79 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Wed Apr 25 10:53:52 2012 +0100
+++ b/tools/libxl/libxl_internal.h	Wed Apr 25 12:55:57 2012 +0100
@@ -83,6 +83,7 @@
 #define STUBDOM_CONSOLE_RESTORE 2
 #define STUBDOM_CONSOLE_SERIAL 3
 #define STUBDOM_SPECIAL_CONSOLES 3
+#define TAP_DEVICE_SUFFIX "-emu"
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
@@ -196,17 +197,6 @@ _hidden libxl__ev_xswatch *libxl__watch_
  * version of the _evdisable_FOO function; the internal one is
  * used during cleanup.
  */
-_hidden int libxl__domain_create_info_setdefault(libxl__gc *gc,
-                                        libxl_domain_create_info *c_info);
-_hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
-                                        libxl_domain_build_info *b_info);
-_hidden int libxl__device_disk_setdefault(libxl__gc *gc,
-                                          libxl_device_disk *disk);
-_hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic);
-_hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb);
-_hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
-_hidden int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci *pci);
-
 struct libxl__evgen_domain_death {
     uint32_t domid;
     unsigned shutdown_reported:1, death_reported:1;
@@ -704,6 +694,21 @@ _hidden int libxl__wait_for_backend(libx
  *     All libxl API functions are expected to have arranged for this
  *     to be called before using any values within these structures.
  */
+_hidden int libxl__domain_create_info_setdefault(libxl__gc *gc,
+                                        libxl_domain_create_info *c_info);
+_hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
+                                        libxl_domain_build_info *b_info);
+_hidden int libxl__device_disk_setdefault(libxl__gc *gc,
+                                          libxl_device_disk *disk);
+_hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic);
+_hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb);
+_hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
+_hidden int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci *pci);
+
+_hidden const char *libxl__device_nic_devname(libxl__gc *gc,
+                                              uint32_t domid,
+                                              uint32_t devid,
+                                              libxl_nic_type type);
 
 /* Arranges that dev will be removed from its guest.  When
  * this is done, the ao will be completed.  An error
diff -r a14b1dd0feee -r 44fc76485f79 tools/python/xen/xend/image.py
--- a/tools/python/xen/xend/image.py	Wed Apr 25 10:53:52 2012 +0100
+++ b/tools/python/xen/xend/image.py	Wed Apr 25 12:55:57 2012 +0100
@@ -917,11 +917,7 @@ class HVMImageHandler(ImageHandler):
             ret.append("-net")
             ret.append("nic,vlan=%d,macaddr=%s,model=%s" %
                        (nics, mac, model))
-            vifname = devinfo.get('vifname')
-            if vifname:
-                vifname = "tap-" + vifname
-            else:
-                vifname = "tap%d.%d" % (self.vm.getDomid(), nics-1)
+            vifname = "vif%d.%d-emu" % (self.vm.getDomid(), nics-1)
             ret.append("-net")
             ret.append("tap,vlan=%d,ifname=%s,bridge=%s" %
                        (nics, vifname, bridge))