Re: [Xen-devel] [PATCH v10 16/19] libxl: call hotplug scripts for nic devices from libxl

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-07-20 at 17:01 +0100, Ian Campbell wrote:
> > +        switch (nictype) {
> > +        case LIBXL_NIC_TYPE_VIF_IOEMU:
> > +            env[nr++] = "INTERFACE";
> > +            env[nr++] = libxl__strdup(gc, libxl__device_nic_devname(gc,
> > +                                                      dev->domid, 
> > dev->devid,
> > +                                                      
> > LIBXL_NIC_TYPE_VIF_IOEMU));
> 
> Is this fall-through of the case deliberate? If so then a comment would
> be good.
> 
> However I'm not sure about this. It seems like the hotplug scripts are
> only called once in the VIF_IOEMU case? I would expect one call for the
> PV VIF device and one for the TAP device? Perhaps I'm just missing the
> location of the other one? Is this "num_exec" variable involved in some
> way? But you don't propagate that to get_hotplug_env() and therefore
> include *both* sets of env vars?
> 
> In any case something related seems to be broken for guests which use a
> stub domain and use vifname=foo. What I see in that case is the script
> called three times...
>      1. For the PV interface of the stub domain (LIBXL_NIC_TYPE_VIF),
>         renames vifX+1.0 -> foo-emu (*)
>      2. For the PV interface of the real domain
>         (LIBXL_NIC_TYPE_VIF_IOEMU part 1), renames vifX.0 -> foo
>      3. For the emu interface of the real domain
>         (LIBXL_NIC_TYPE_VIF_IOEMU part 2) renames vifX.0 -> foo-emu
> 
> The rename in #3 fails, because foo-emu already exists. But in fact this
> call to the script should never happen for a domain with a stub domain
> since the emulated device in this case is within the stub domain, not
> the driver domain.

My slightly skanky fix, still with debugging still, is below. I'm
posting it now because it's late and I'm going home.

I think a better fix would be to determine up front which scripts needed
running and track that instead of num_execs.

diff -r 574bece33c88 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c    Fri Jul 20 15:58:18 2012 +0100
+++ b/tools/libxl/libxl_dm.c    Fri Jul 20 17:35:54 2012 +0100
@@ -616,6 +616,25 @@ static char ** libxl__build_device_model
     }
 }
 
+static void libxl__dm_nics_from_hvm_guest_config(libxl__gc *gc,
+                                    libxl_domain_config * const guest_config,
+                                    libxl_domain_config *dm_config)
+{
+    int i, nr = guest_config->num_nics;
+
+    GCNEW_ARRAY(dm_config->nics, nr);
+
+    for (i=0; i<nr; i++) {
+        dm_config->nics[i] = guest_config->nics[i];
+        if (dm_config->nics[i].ifname)
+            dm_config->nics[i].ifname = GCSPRINTF("%s" TAP_DEVICE_SUFFIX,
+                                                  dm_config->nics[i].ifname);
+        dm_config->nics[i].nictype = LIBXL_NIC_TYPE_VIF;
+    }
+
+    dm_config->num_nics = nr;
+}
+
 static int libxl__vfb_and_vkb_from_hvm_guest_config(libxl__gc *gc,
                                         const libxl_domain_config 
*guest_config,
                                         libxl_device_vfb *vfb,
@@ -764,8 +783,7 @@ void libxl__spawn_stub_dm(libxl__egc *eg
     dm_config->disks = guest_config->disks;
     dm_config->num_disks = guest_config->num_disks;
 
-    dm_config->nics = guest_config->nics;
-    dm_config->num_nics = guest_config->num_nics;
+    libxl__dm_nics_from_hvm_guest_config(gc, guest_config, dm_config);
 
     dm_config->c_info.run_hotplug_scripts =
         guest_config->c_info.run_hotplug_scripts;
diff -r 574bece33c88 tools/libxl/libxl_linux.c
--- a/tools/libxl/libxl_linux.c Fri Jul 20 15:58:18 2012 +0100
+++ b/tools/libxl/libxl_linux.c Fri Jul 20 17:35:54 2012 +0100
@@ -117,11 +117,13 @@ static char **get_hotplug_env(libxl__gc 
             env[nr++] = libxl__strdup(gc, libxl__device_nic_devname(gc,
                                                       dev->domid, dev->devid,
                                                       
LIBXL_NIC_TYPE_VIF_IOEMU));
+            fprintf(stderr, "dom%d: %d: VIF_IOEMU %s = %s\n", dev->domid, 
dev->devid, env[nr-2], env[nr-1]);
         case LIBXL_NIC_TYPE_VIF:
             env[nr++] = "vif";
             env[nr++] = libxl__strdup(gc, libxl__device_nic_devname(gc,
                                                       dev->domid, dev->devid,
                                                       LIBXL_NIC_TYPE_VIF));
+            fprintf(stderr, "dom%d: %d: VIF %s = %s\n", dev->domid, 
dev->devid, env[nr-2], env[nr-1]);
             break;
         default:
             return NULL;
@@ -159,11 +161,22 @@ static int libxl__hotplug_nic(libxl__gc 
         rc = ERROR_FAIL;
         goto out;
     }
+
+    fprintf(stderr, "%s: type=%s num_exec=%d action=%d hasstubdom=%s 
isstubdom=%s\n",
+            __func__, libxl_nic_type_to_string(nictype),
+            num_exec, action,
+            libxl_get_stubdom_id(CTX, dev->domid) ? "yes" : "no",
+            libxl_is_stubdom(CTX, dev->domid, NULL) ? "yes" : "no");
+
     if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
         rc = 0;
         goto out;
     }
-
+    if (nictype == LIBXL_NIC_TYPE_VIF_IOEMU && num_exec != 0 && 
libxl_get_stubdom_id(CTX, dev->domid)) {
+        fprintf(stderr, "skipping EMU for domain w/ stubdom\n");
+        rc = 0;
+        goto out;
+    }
     *env = get_hotplug_env(gc, dev);
     if (!env) {
         rc = ERROR_FAIL;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel