[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device

Ian Jackson wrote:
Roger Pau Monne writes ("[PATCH v4 01/10] libxl: change libxl__ao_device_remove to 
Introduce a new structure to track state of device backends, that will
be used in following patches on this series.

This structure if used for both device creation and device
destruction and removes libxl__ao_device_remove.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e3d05c2..7fdecf1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
+/* Macro for defining device remove/destroy functions in a compact way */
+#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)                    \

Looks reasonable to me.

+/* Define all remove/destroy functions and undef the macro */
+/* disk */
+DEFINE_DEVICE_REMOVE(disk, remove, 0)
+DEFINE_DEVICE_REMOVE(disk, destroy, 1)

I'm not sure what purpose the comment serves but I don't really object
to it :-).

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 52f5435..670a17b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -830,13 +830,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *
+/* This functions sets the necessary libxl__ao_device struct values to use
+ * safely inside functions. It marks the operation as "active"
+ * since we need to be sure that all device status structs are set
+ * to active before start queueing events, or we might call
+ * ao_complete before all devices had finished */
+_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
+                                      libxl__ao_device **base);

You need to clearly explain what the constraints are on the order of
calling _prepare and _initiate_..._remove.

Questions whose answers should be clear from your text include:
  - is it legal to call _initiate_ without having previously called
    _prepare ?
  - is it legal to call _prepare and then simply throw away the
    aodev ?
  - is it legal to call _prepare multiple times ?  (If the answer
    to the previous question is `yes' then it must be, I think.)

I've added a more detailed comment about _prepare.

@@ -436,34 +441,35 @@ out:
-int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
-                                  libxl__device *dev)
+void libxl__initiate_device_remove(libxl__egc *egc,
+                                   libxl__ao_device *aodev)
+    t = xs_transaction_start(ctx->xsh);
+    if (aodev->force)
+        libxl__xs_path_cleanup(gc, t,
+                               libxl__device_frontend_path(gc, aodev->dev));
+    state = libxl__xs_read(gc, t, state_path);

Didn't I previously comment adversely on having this check here ?

Ah yes, I commented on the corresponding code in add (v2 08/15):

   [...], I think all of this is done for you by
   libxl__ev_devstate_wait.  You don't need to pre-check the state path
   at all.


   Do you really need to do the xenstore state read here ?  Surely
   libxl__ev_devstate_wait will do it for you.


+    /* Some devices (vkbd) fail to disconnect properly,
+     * but we shouldn't alarm the user if it's during
+     * domain destruction.
+     */
+    if (rc&&  aodev->action == DEVICE_CONNECT) {
+        LOG(ERROR, "unable to connect device with path %s",
+                   libxl__device_backend_path(gc, aodev->dev));
+        goto out;
+    } else if(rc) {

Missing space after `if'.


+        LOG(DEBUG, "unable to disconnect device with path %s",
+                   libxl__device_backend_path(gc, aodev->dev));
+        goto out;
+    }

Last time I wrote:

   Perhaps we should have a separate flag which controls error reporting
   so that a user-requested graceful device disconnect gets full error
   logging ?

Did you miss the fact that email suggesting the macro for generating
the device remove functions also had these other comments in ?

I've replied to this email, the response is here:


I would like to leave this for future work, since the error checking we have now is not better that what I'm proposing here.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.