[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v6 05/13] libxl: convert libxl__device_disk_local_attach to an async op
Ian Jackson wrote:
Roger Pau Monne writes ("[PATCH v6 05/13] libxl: convert
libxl__device_disk_local_attach to an async op"):
This will be needed in future patches, when libxl__device_disk_add
becomes async also. Create a new status structure that defines the
local attach of a disk device and use it in
libxl__device_disk_local_attach.
This is looking good. I have a couple of comments:
Thanks for the review!
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 7ebc0df..182a975 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
...
+static void bootloader_fisnihed_cb(libxl__egc *egc,
^^^^^^^^
`finished'. You have apparently managed to misspell this everywhere
you mention it!
Probably copy-pasted it wrong everywhere, fixed.
+ libxl__disk_local_state *dls,
+ int rc);
+
static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
int rc)
{
bootloader_cleanup(egc, bl);
+
+ if (bl->diskpath) {
+ bl->dls.callback = bootloader_fisnihed_cb;
+ libxl__device_disk_local_detach(egc,&bl->dls);
+ return;
+ }
Can we make libxl__device_disk_local_detach idempotent (and perhaps
call it `libxl__device_disk_local_attached_initiate_cleanup' or
something, if you can think of a name that's less than a paragraph) ?
I'm not sure the "attached" in the name is correct, because the device
might have failed to attach, and still we are going to call this
function. How about "libxl__device_disk_local_initiate_detach"? (still
quite long). Then the attach function should be renamed to
libxl__device_disk_local_initiate_attach.
If so then this would be simpler and wouldn't need to test
bl->diskpath.
Ok, I've moved the test for bl->dls.diskpath to
libxl__device_disk_local_detach, so we have a more linear flow of callbacks.
+static void bootloader_disk_attached_cb(libxl__egc *egc,
+ libxl__disk_local_state *dls,
+ int rc)
...
+ bl->diskpath = bl->dls.diskpath;
Now that we have a bl->dls.diskpath, surely we can do away with
bl->diskpath ? I'm not a fan of having multiple variables with the
same information in them, unless it's essential. It just leads to
confusion and error.
Done.
+/*
+ * Make a disk available in this (the control) domain. Calls dls->callback
+ * when finished.
+ */
+_hidden void libxl__device_disk_local_attach(libxl__egc *egc,
+ libxl__disk_local_state *dls);
+_hidden void libxl__device_disk_local_detach(libxl__egc *egc,
+ libxl__disk_local_state *dls);
You really need to explain the rules for one of these dls's.
Is it something like this:
* A libxl__disk_local_state may be in the following states:
* Undefined, Idle, Attaching, Attached, Detaching
Ok, I've added the init function and the description of what this
functions do, together with the rename it should be a little bit more clear.
typedef void libxl__disk_local_state_callback(libxl__egc*,
libxl__disk_local_state*,
int rc);
_hidden void libxl__device_disk_local_attach(libxl__egc *egc,
libxl__disk_local_state *dls);
/* Undefined/Idle -> Attaching. Will call callback.
* On entry to callback, if rc!=0 dls is Idle;
* if rc==0 it is Attached and dls->diskpath is usable. */
_hidden void libxl__device_disk_local_detach(libxl__egc *egc,
libxl__disk_local_state *dls);
/* Attached -> Detaching. Will call callback.
* On entry to callback, dls is Idle, regardless of
* success or failure. */
And my suggestion about idempotency would change this latter comment
to:
/* Idle/Attached -> Detaching. Will call callback.
* On entry to callback, dls is Idle, regardless of
* success or failure. */
And the bootloader code might want this:
_hidden void libxl__device_disk_local_init(libxl__disk_local_state *dls);
/* Undefined/Idle -> Idle */
Or you can explain it in prose if you like.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel