Re: [patch] xen udev rule interfering with openvpn

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

On Sat, 2012-05-12 at 23:00 +0100, Teck Choon Giam wrote:
> On Sat, May 12, 2012 at 3:29 PM, Teck Choon Giam
> <giamteckchoon@gmail.com> wrote:
> > On Sat, May 12, 2012 at 7:53 AM, Teck Choon Giam
> > <giamteckchoon@gmail.com> wrote:
> >> On Fri, May 11, 2012 at 10:53 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> >>> Ian Campbell writes ("Re: [Xen-devel] [patch] xen udev rule interfering with openvpn"):
> >>>> libxl/xend: name tap devices vifX.Y-emu
> >>>
> >>> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >>
> >> This is my backport version which excludes the following:
> >>
> >> 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)
> >>
> >> I have tested with xm and xl with and without vifname set in domU
> >> config for hvmdomain and pvdomain.
> >
> > Sorry, when re-test one of the test case failed... xm create hvmdomain
> > with vifname set.  I must have missed certain test case :(
> 
> The same test case failed for xen unstable latest changeset 25326:cd4dd23a831d.

Oh dear.

> My tests as below:

Which ones fail?

> 1. xm create pvdomain without vifname set
> 2. xl create pvdomain without vifname set
> 3. xm create hvmdomain without vifname set
> 4. xl create hvmdomain without vifname set
> 5. xm create pvdomain with vifname set
> 6. xl create pvdomain with vifname set
> 7. xm create hvmdomain with vifname set
> 8. xl create hvmdomain with vifname set
> 
> Thanks.
> 
> Kindest regards,
> 
> 
> >
> > Kindest regards,
> > Giam Teck Choon
> >
> >
> >
> >>
> >> For your review and comments please.
> >>
> >> Thanks.
> >>
> >> Kindest regards,
> >> Giam Teck Choon
> >>
> >>
> >>
> >>
> >> 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.
> >>
> >> xen-unstable changeset: 25290:7a6dcecb1781
> >> Signed-off-by: Giam Teck Choon <giamteckchoon@gmail.com>
> >> ---
> >>  tools/hotplug/Linux/vif-common.sh     |   15 +++++++++++++--
> >>  tools/hotplug/Linux/xen-backend.rules |    2 +-
> >>  tools/libxl/libxl_dm.c                |   17 ++++++-----------
> >>  tools/python/xen/xend/image.py        |    6 +-----
> >>  4 files changed, 21 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/tools/hotplug/Linux/vif-common.sh
> >> b/tools/hotplug/Linux/vif-common.sh
> >> index c9c5d41..fff22bb 100644
> >> --- a/tools/hotplug/Linux/vif-common.sh
> >> +++ b/tools/hotplug/Linux/vif-common.sh
> >> @@ -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 --git a/tools/hotplug/Linux/xen-backend.rules
> >> b/tools/hotplug/Linux/xen-backend.rules
> >> index 40f2658..405387f 100644
> >> --- a/tools/hotplug/Linux/xen-backend.rules
> >> +++ b/tools/hotplug/Linux/xen-backend.rules
> >> @@ -13,4 +13,4 @@ KERNEL=="blktap-control",
> >> NAME="xen/blktap-2/control", MODE="0600"
> >>  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 --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >> index 1ffcc90..2c030ca 100644
> >> --- a/tools/libxl/libxl_dm.c
> >> +++ b/tools/libxl/libxl_dm.c
> >> @@ -134,11 +134,9 @@ static char **
> >> libxl_build_device_model_args_old(libxl__gc *gc,
> >>                 char *smac = libxl__sprintf(gc,
> >> "%02x:%02x:%02x:%02x:%02x:%02x",
> >>                                            vifs[i].mac[0],
> >> vifs[i].mac[1], vifs[i].mac[2],
> >>                                            vifs[i].mac[3],
> >> vifs[i].mac[4], vifs[i].mac[5]);
> >> -                char *ifname;
> >> -                if (!vifs[i].ifname)
> >> -                    ifname = libxl__sprintf(gc, "tap%d.%d",
> >> info->domid, vifs[i].devid);
> >> -                else
> >> -                    ifname = vifs[i].ifname;
> >> +                const char *ifname = libxl__sprintf(gc, "vif%d.%d-emu",
> >> +                                                    info->domid,
> >> +                                                    vifs[i].devid);
> >>                 flexarray_vappend(dm_args,
> >>                                 "-net", libxl__sprintf(gc,
> >> "nic,vlan=%d,macaddr=%s,model=%s",
> >>                                                        vifs[i].devid,
> >> smac, vifs[i].model),
> >> @@ -271,12 +269,9 @@ static char **
> >> libxl_build_device_model_args_new(libxl__gc *gc,
> >>                 char *smac = libxl__sprintf(gc,
> >> "%02x:%02x:%02x:%02x:%02x:%02x",
> >>                                            vifs[i].mac[0],
> >> vifs[i].mac[1], vifs[i].mac[2],
> >>                                            vifs[i].mac[3],
> >> vifs[i].mac[4], vifs[i].mac[5]);
> >> -                char *ifname;
> >> -                if (!vifs[i].ifname) {
> >> -                    ifname = libxl__sprintf(gc, "tap%d.%d",
> >> info->domid, vifs[i].devid);
> >> -                } else {
> >> -                    ifname = vifs[i].ifname;
> >> -                }
> >> +                const char *ifname = libxl__sprintf(gc, "vif%d.%d-emu",
> >> +                                                    info->domid,
> >> +                                                    vifs[i].devid);
> >>                 flexarray_append(dm_args, "-net");
> >>                 flexarray_append(dm_args, libxl__sprintf(gc,
> >> "nic,vlan=%d,macaddr=%s,model=%s",
> >>                             vifs[i].devid, smac, vifs[i].model));
> >> diff --git a/tools/python/xen/xend/image.py b/tools/python/xen/xend/image.py
> >> index f1464ac..3c8d80c 100644
> >> --- a/tools/python/xen/xend/image.py
> >> +++ b/tools/python/xen/xend/image.py
> >> @@ -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))