Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 0/12] libxl: new hotplug calling convention
This series implements a new hoplug calling convention for libxl.
The aim of this new convention is to reduce the blackout phase of
migration when using complex hotplug scripts, like iSCSI or other kind
of storage backends that might have a non trivial setup time.
There are some issues that I would like to discuss, the first one is
the fact that pdev_path field in libxl_device_disk is no longuer used
to store a physical path, since diskspec "target" can now contain
"random" information to connect a block device. This is not a blocker
for this series to be accepted, but I''m tempted to say that the easier
way to solve this is to use a union for target and pdev_path, this
way everything will work as expected, and semantics will be right.
The second issue is related to iSCSI and iscsiadm, specifically the
way to set authentication parameters, which is done with command line
parameters or editing files (which each distro seems to place in
different locations). I will look into this to see if we can find a
suitable solution.
Thanks for the comments, Roger.
Branch available in the git repository at:
git://xenbits.xen.org/people/royger/xen.git iscsi_v1
*Acked
Roger Pau Monne (12):
*libxl: libxl__prepare_ao_device should reset num_exec
libxl: remove double check in NetBSD hotplug
libxl: move libxl_device_action to idl
libxl: pack hotplug related variables
libxl: add new hotplug interface support to hotplug script callers
libxl: add support for different hotplug interfaces
libxl: add prepare/unprepare operations to the libxl public interface
libxl: add disk specific remove functions
xl: add support for new hotplug interface to block-attach/detach
libxl: add local attach support for new hotplug scripts
hotplug: document new hotplug interface
hotplug/Linux: add iscsi block hotplug script
docs/misc/libxl-hotplug-interface.txt | 167 +++++++++++++++++
tools/hotplug/Linux/Makefile | 1 +
tools/hotplug/Linux/block-iscsi | 258 +++++++++++++++++++++++++
tools/libxl/libxl.c | 332 +++++++++++++++++++++++++++++----
tools/libxl/libxl.h | 8 +
tools/libxl/libxl_bootloader.c | 1 +
tools/libxl/libxl_create.c | 62 ++++++-
tools/libxl/libxl_device.c | 153 +++++++++++++---
tools/libxl/libxl_internal.h | 87 ++++++++-
tools/libxl/libxl_linux.c | 78 ++++++---
tools/libxl/libxl_netbsd.c | 10 +-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/libxl_types_internal.idl | 10 +
tools/libxl/xl_cmdimpl.c | 5 +
14 files changed, 1071 insertions(+), 102 deletions(-)
create mode 100644 docs/misc/libxl-hotplug-interface.txt
create mode 100644 tools/hotplug/Linux/block-iscsi
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 01/12] libxl: libxl__prepare_ao_device should reset num_exec
num_exec was not cleared when calling libxl__prepare_ao_device.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_device.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 51dd06e..58d3f35 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -409,6 +409,7 @@ void libxl__prepare_ao_device(libxl__ao *ao,
libxl__ao_device *aodev)
aodev->ao = ao;
aodev->rc = 0;
aodev->dev = NULL;
+ aodev->num_exec = 0;
/* Initialize timer for QEMU Bodge and hotplug execution */
libxl__ev_time_init(&aodev->timeout);
aodev->active = 1;
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 02/12] libxl: remove double check in NetBSD hotplug
Remove a duplicated check performed in libxl__get_hotplug_script_info
for NetBSD
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_netbsd.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9587833..36662c0 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -76,10 +76,6 @@ int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
switch (dev->backend_kind) {
case LIBXL__DEVICE_KIND_VBD:
case LIBXL__DEVICE_KIND_VIF:
- if (num_exec != 0) {
- rc = 0;
- goto out;
- }
rc = libxl__hotplug(gc, dev, args, action);
if (!rc) rc = 1;
break;
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 03/12] libxl: move libxl_device_action to idl
Move to idl for ease of expansion and auto-generated functions.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 18 +++++++++---------
tools/libxl/libxl_device.c | 11 ++++++-----
tools/libxl/libxl_internal.h | 6 ------
tools/libxl/libxl_linux.c | 7 ++++---
tools/libxl/libxl_netbsd.c | 2 +-
tools/libxl/libxl_types_internal.idl | 5 +++++
6 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 73e0dc3..a67c4dc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1695,12 +1695,12 @@ static void device_addrm_aocomplete(libxl__egc *egc,
libxl__ao_device *aodev)
if (aodev->rc) {
if (aodev->dev) {
LOG(ERROR, "unable to %s %s with id %u",
- aodev->action == DEVICE_CONNECT ? "add" :
"remove",
+ libxl__device_action_to_string(aodev->action),
libxl__device_kind_to_string(aodev->dev->kind),
aodev->dev->devid);
} else {
LOG(ERROR, "unable to %s device",
- aodev->action == DEVICE_CONNECT ? "add" :
"remove");
+ libxl__device_action_to_string(aodev->action));
}
goto out;
}
@@ -1806,7 +1806,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t
domid,
libxl__xs_kvs_of_flexarray(gc, front,
front->count));
aodev->dev = device;
- aodev->action = DEVICE_CONNECT;
+ aodev->action = LIBXL__DEVICE_ACTION_ADD;
libxl__wait_device_connection(egc, aodev);
rc = 0;
@@ -2163,7 +2163,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
}
aodev->dev = device;
- aodev->action = DEVICE_CONNECT;
+ aodev->action = LIBXL__DEVICE_ACTION_ADD;
libxl__wait_device_connection(egc, aodev);
rc = 0;
@@ -2628,7 +2628,7 @@ static void local_device_attach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
rc = aodev->rc;
if (rc) {
LOGE(ERROR, "unable to %s %s with id %u",
- aodev->action == DEVICE_CONNECT ? "add" :
"remove",
+ libxl__device_action_to_string(aodev->action),
libxl__device_kind_to_string(aodev->dev->kind),
aodev->dev->devid);
goto out;
@@ -2682,7 +2682,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc
*egc,
disk, device);
if (rc != 0) goto out;
- aodev->action = DEVICE_DISCONNECT;
+ aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
aodev->dev = device;
aodev->callback = local_device_detach_cb;
aodev->force = 0;
@@ -2713,7 +2713,7 @@ static void local_device_detach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
if (aodev->rc) {
LOGE(ERROR, "unable to %s %s with id %u",
- aodev->action == DEVICE_CONNECT ? "add" :
"remove",
+ libxl__device_action_to_string(aodev->action),
libxl__device_kind_to_string(aodev->dev->kind),
aodev->dev->devid);
goto out;
@@ -2892,7 +2892,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t
domid,
libxl__xs_kvs_of_flexarray(gc, front,
front->count));
aodev->dev = device;
- aodev->action = DEVICE_CONNECT;
+ aodev->action = LIBXL__DEVICE_ACTION_ADD;
libxl__wait_device_connection(egc, aodev);
rc = 0;
@@ -3370,7 +3370,7 @@ out:
\
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
- aodev->action = DEVICE_DISCONNECT; \
+ aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
aodev->dev = device; \
aodev->callback = device_addrm_aocomplete; \
aodev->force = f; \
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 58d3f35..eeea9d9 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -614,7 +614,7 @@ void libxl__devices_destroy(libxl__egc *egc,
libxl__devices_remove_state *drs)
continue;
}
aodev = libxl__multidev_prepare(multidev);
- aodev->action = DEVICE_DISCONNECT;
+ aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
aodev->dev = dev;
aodev->force = drs->force;
libxl__initiate_device_remove(egc, aodev);
@@ -849,7 +849,8 @@ static void device_backend_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
device_backend_cleanup(gc, aodev);
- if (rc == ERROR_TIMEDOUT && aodev->action == DEVICE_DISCONNECT
&&
+ if (rc == ERROR_TIMEDOUT &&
+ aodev->action == LIBXL__DEVICE_ACTION_REMOVE &&
!aodev->force) {
aodev->force = 1;
libxl__initiate_device_remove(egc, aodev);
@@ -858,7 +859,7 @@ static void device_backend_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
if (rc) {
LOG(ERROR, "unable to %s device with path %s",
- aodev->action == DEVICE_CONNECT ? "connect" :
"disconnect",
+ libxl__device_action_to_string(aodev->action),
libxl__device_backend_path(gc, aodev->dev));
goto out;
}
@@ -981,7 +982,7 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
if (hotplug_error)
LOG(ERROR, "script: %s", hotplug_error);
aodev->rc = ERROR_FAIL;
- if (aodev->action == DEVICE_CONNECT)
+ if (aodev->action == LIBXL__DEVICE_ACTION_ADD)
/*
* Only fail on device connection, on disconnection
* ignore error, and continue with the remove process
@@ -1011,7 +1012,7 @@ static void device_hotplug_done(libxl__egc *egc,
libxl__ao_device *aodev)
device_hotplug_clean(gc, aodev);
/* Clean xenstore if it's a disconnection */
- if (aodev->action == DEVICE_DISCONNECT) {
+ if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE) {
rc = libxl__device_destroy(gc, aodev->dev);
if (!aodev->rc)
aodev->rc = rc;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0b38e3e..f127f8a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1829,12 +1829,6 @@ _hidden const char *libxl__run_dir_path(void);
/*----- device addition/removal -----*/
-/* Action to perform (either connect or disconnect) */
-typedef enum {
- DEVICE_CONNECT,
- DEVICE_DISCONNECT
-} libxl__device_action;
-
typedef struct libxl__ao_device libxl__ao_device;
typedef struct libxl__multidev libxl__multidev;
typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 1fed3cd..1ef13c0 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -173,11 +173,12 @@ static int libxl__hotplug_nic(libxl__gc *gc, libxl__device
*dev,
(*args)[nr++] = script;
if (nictype == LIBXL_NIC_TYPE_VIF_IOEMU && num_exec) {
- (*args)[nr++] = action == DEVICE_CONNECT ? "add" :
"remove";
+ (*args)[nr++] = (char *) libxl__device_action_to_string(action);
(*args)[nr++] = "type_if=tap";
(*args)[nr++] = NULL;
} else {
- (*args)[nr++] = action == DEVICE_CONNECT ? "online" :
"offline";
+ (*args)[nr++] = action == LIBXL__DEVICE_ACTION_ADD ? "online"
:
+
"offline";
(*args)[nr++] = "type_if=vif";
(*args)[nr++] = NULL;
}
@@ -213,7 +214,7 @@ static int libxl__hotplug_disk(libxl__gc *gc, libxl__device
*dev,
const int arraysize = 3;
GCNEW_ARRAY(*args, arraysize);
(*args)[nr++] = script;
- (*args)[nr++] = action == DEVICE_CONNECT ? "add" :
"remove";
+ (*args)[nr++] = (char *) libxl__device_action_to_string(action);
(*args)[nr++] = NULL;
assert(nr == arraysize);
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 36662c0..0ad69af 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -50,7 +50,7 @@ static int libxl__hotplug(libxl__gc *gc, libxl__device *dev,
char ***args,
GCNEW_ARRAY(*args, arraysize);
(*args)[nr++] = script;
(*args)[nr++] = be_path;
- (*args)[nr++] = GCSPRINTF("%d", action == DEVICE_CONNECT ?
+ (*args)[nr++] = GCSPRINTF("%d", action ==
LIBXL__DEVICE_ACTION_ADD ?
XenbusStateInitWait : XenbusStateClosed);
(*args)[nr++] = NULL;
assert(nr == arraysize);
diff --git a/tools/libxl/libxl_types_internal.idl
b/tools/libxl/libxl_types_internal.idl
index c40120e..cb9444f 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -33,3 +33,8 @@ libxl__device_console = Struct("device_console", [
("consback", libxl__console_backend),
("output", string),
])
+
+libxl__device_action = Enumeration("device_action", [
+ (1, "ADD"),
+ (2, "REMOVE"),
+ ])
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 04/12] libxl: pack hotplug related variables
Create a new struct to hold hotplug related variables that are
scattered in libxl__ao_device. We will later expand the number of
hotplug related variables, so it's best to have them packed
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_device.c | 6 +++---
tools/libxl/libxl_internal.h | 15 ++++++++++-----
tools/libxl/libxl_linux.c | 10 +++++-----
tools/libxl/libxl_netbsd.c | 4 ++--
4 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index eeea9d9..044cac5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -409,7 +409,7 @@ void libxl__prepare_ao_device(libxl__ao *ao,
libxl__ao_device *aodev)
aodev->ao = ao;
aodev->rc = 0;
aodev->dev = NULL;
- aodev->num_exec = 0;
+ aodev->hotplug.num_exec = 0;
/* Initialize timer for QEMU Bodge and hotplug execution */
libxl__ev_time_init(&aodev->timeout);
aodev->active = 1;
@@ -892,7 +892,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device
*aodev)
* and return the necessary args/env vars for execution */
hotplug = libxl__get_hotplug_script_info(gc, aodev->dev, &args,
&env,
aodev->action,
- aodev->num_exec);
+ &aodev->hotplug);
switch (hotplug) {
case 0:
/* no hotplug script to execute */
@@ -994,7 +994,7 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
* If no more executions are needed, device_hotplug will call
* device_hotplug_done breaking the loop.
*/
- aodev->num_exec++;
+ aodev->hotplug.num_exec++;
device_hotplug(egc, aodev);
return;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f127f8a..d69d819 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1831,6 +1831,7 @@ _hidden const char *libxl__run_dir_path(void);
typedef struct libxl__ao_device libxl__ao_device;
typedef struct libxl__multidev libxl__multidev;
+typedef struct libxl__hotplug libxl__hotplug;
typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
/* This functions sets the necessary libxl__ao_device struct values to use
@@ -1849,6 +1850,10 @@ typedef void libxl__device_callback(libxl__egc*,
libxl__ao_device*);
*/
_hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
+struct libxl__hotplug {
+ int num_exec;
+};
+
struct libxl__ao_device {
/* filled in by user */
libxl__ao *ao;
@@ -1867,7 +1872,7 @@ struct libxl__ao_device {
libxl__ev_time timeout;
/* device hotplug execution */
const char *what;
- int num_exec;
+ libxl__hotplug hotplug;
libxl__ev_child child;
};
@@ -2048,18 +2053,18 @@ _hidden void libxl__initiate_device_remove(libxl__egc
*egc,
* 0: No need to execute hotplug script
* 1: Execute hotplug script
*
- * The last parameter, "num_exec" refeers to the number of times
hotplug
- * scripts have been called for this device.
+ * The last parameter, libxl__hotplug contains information related to hotplug
+ * execution.
*
* The main body of libxl will, for each device, keep calling
* libxl__get_hotplug_script_info, with incrementing values of
- * num_exec, and executing the resulting script accordingly,
+ * hotplug->num_exec, and executing the resulting script accordingly,
* until libxl__get_hotplug_script_info returns<=0.
*/
_hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
libxl__device_action action,
- int num_exec);
+ libxl__hotplug *hotplug);
/*----- local disk attach: attach a disk locally to run the bootloader -----*/
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 1ef13c0..531bc7e 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -227,7 +227,7 @@ error:
int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
libxl__device_action action,
- int num_exec)
+ libxl__hotplug *hotplug)
{
char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
int rc;
@@ -240,7 +240,7 @@ int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
switch (dev->backend_kind) {
case LIBXL__DEVICE_KIND_VBD:
- if (num_exec != 0) {
+ if (hotplug->num_exec != 0) {
rc = 0;
goto out;
}
@@ -251,12 +251,12 @@ int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
* If domain has a stubdom we don't have to execute hotplug scripts
* for emulated interfaces
*/
- if ((num_exec > 1) ||
- (libxl_get_stubdom_id(CTX, dev->domid) && num_exec)) {
+ if ((hotplug->num_exec > 1) ||
+ (libxl_get_stubdom_id(CTX, dev->domid) &&
hotplug->num_exec)) {
rc = 0;
goto out;
}
- rc = libxl__hotplug_nic(gc, dev, args, env, action, num_exec);
+ rc = libxl__hotplug_nic(gc, dev, args, env, action,
hotplug->num_exec);
break;
default:
/* No need to execute any hotplug scripts */
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 0ad69af..fa5e418 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -62,13 +62,13 @@ out:
int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
libxl__device_action action,
- int num_exec)
+ libxl__hotplug *hotplug)
{
char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
int rc;
/* Check if we have to run hotplug scripts */
- if (!disable_udev || num_exec > 0) {
+ if (!disable_udev || hotplug->num_exec > 0) {
rc = 0;
goto out;
}
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers
Add support to call hotplug scripts that use the new hotplug interface
to the low-level functions in charge of calling hotplug scripts.
libxl__prepare_ao_device sets the hotplug version to 1, in later
patches hotplug version will be set to 2 by the caller to use this
new hotplug calling convention.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_device.c | 14 +++++++-
tools/libxl/libxl_internal.h | 22 ++++++++++++
tools/libxl/libxl_linux.c | 61 +++++++++++++++++++++++++--------
tools/libxl/libxl_types_internal.idl | 5 +++
4 files changed, 86 insertions(+), 16 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 044cac5..904853a 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -83,6 +83,12 @@ out:
return rc;
}
+char *libxl__device_xs_hotplug_path(libxl__gc *gc, libxl__device *dev)
+{
+ return GCSPRINTF("/local/domain/%u/libxl/hotplug/%u/%u",
+ dev->backend_domid, dev->domid, dev->devid);
+}
+
int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
libxl__device *device, char **bents, char **fents)
{
@@ -410,6 +416,7 @@ void libxl__prepare_ao_device(libxl__ao *ao,
libxl__ao_device *aodev)
aodev->rc = 0;
aodev->dev = NULL;
aodev->hotplug.num_exec = 0;
+ aodev->hotplug.version = 1;
/* Initialize timer for QEMU Bodge and hotplug execution */
libxl__ev_time_init(&aodev->timeout);
aodev->active = 1;
@@ -974,7 +981,12 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
device_hotplug_clean(gc, aodev);
- if (status) {
+ if (status && aodev->action != LIBXL__DEVICE_ACTION_VERSION) {
+ /* If a hotplug script returns an error when called
+ * with the "version" argument it means it is using the old
+ * hotplug calling convention, and we don't have to print
+ * an error message.
+ */
libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
aodev->what, pid, status);
hotplug_error = libxl__xs_read(gc, XBT_NULL,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d69d819..e819bbd 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1847,11 +1847,20 @@ typedef void libxl__device_callback(libxl__egc*,
libxl__ao_device*);
* Once _prepare has been called on a libxl__ao_device, it is safe to just
* discard this struct, there's no need to call any destroy function.
* _prepare can also be called multiple times with the same libxl__ao_device.
+ *
+ * If hotplug.version field is 2, the new hotplug script calling convention
+ * will be used to call the hotplug script. This new convention provides
+ * new actions to hotplug scripts, "prepare", "unprepare",
"localattach",
+ * "localdetach" and "version". This new actions have been
added to offload
+ * work done by hotplug scripts during the blackout phase of migration.
+ * "prepare" is called before the remote domain is paused, so as much
+ * operations as possible should be done in this phase.
*/
_hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
struct libxl__hotplug {
int num_exec;
+ int version;
};
struct libxl__ao_device {
@@ -1861,6 +1870,8 @@ struct libxl__ao_device {
libxl__device *dev;
int force;
libxl__device_callback *callback;
+ /* used by the VERISON and LOCALATTACH actions, set by caller */
+ libxl_device_disk *disk;
/* return value, zeroed by user on entry, is valid on callback */
int rc;
/* private for multidev */
@@ -2044,6 +2055,17 @@ _hidden void libxl__initiate_device_remove(libxl__egc
*egc,
libxl__ao_device *aodev);
/*
+ * libxl__device_xs_hotplug_path returns the xenstore hotplug
+ * path that is used to share information with the hotplug
+ * script.
+ *
+ * This path is only used by new hotplug scripts, that are
+ * specified using "method" instead of "script" in the disk
+ * parameters.
+ */
+_hidden char *libxl__device_xs_hotplug_path(libxl__gc *gc, libxl__device *dev);
+
+/*
* libxl__get_hotplug_script_info returns the args and env that should
* be passed to the hotplug script for the requested device.
*
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 531bc7e..00ffd6d 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -191,32 +191,63 @@ out:
static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
- libxl__device_action action)
+ libxl__device_action action,
+ int hotplug_version)
{
char *be_path = libxl__device_backend_path(gc, dev);
+ char *hotplug_path = libxl__device_xs_hotplug_path(gc, dev);
char *script;
int nr = 0, rc = 0;
+ const int env_arraysize = 5;
+ const int arg_arraysize = 3;
- script = libxl__xs_read(gc, XBT_NULL,
- GCSPRINTF("%s/%s", be_path,
"script"));
- if (!script) {
- LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
- rc = ERROR_FAIL;
- goto error;
- }
-
- *env = get_hotplug_env(gc, script, dev);
- if (!*env) {
+ switch (hotplug_version) {
+ case 1:
+ script = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/script",
be_path));
+ if (!script) {
+ LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
+ rc = ERROR_FAIL;
+ goto error;
+ }
+ *env = get_hotplug_env(gc, script, dev);
+ if (!*env) {
+ rc = ERROR_FAIL;
+ goto error;
+ }
+ break;
+ case 2:
+ /* The new hotplug calling convention only uses two ENV variables:
+ * BACKEND_PATH: path to xenstore backend of the related device.
+ * HOTPLUG_PATH: path to the xenstore directory that can be used to
+ * pass extra parameters to the script.
+ */
+ script = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/script",
hotplug_path));
+ if (!script) {
+ LOGEV(ERROR, errno, "unable to read script from %s",
hotplug_path);
+ rc = ERROR_FAIL;
+ goto error;
+ }
+ GCNEW_ARRAY(*env, env_arraysize);
+ (*env)[nr++] = "BACKEND_PATH";
+ (*env)[nr++] = be_path;
+ (*env)[nr++] = "HOTPLUG_PATH";
+ (*env)[nr++] = hotplug_path;
+ (*env)[nr++] = NULL;
+ assert(nr == env_arraysize);
+ nr = 0;
+ break;
+ default:
+ LOG(ERROR, "unknown hotplug script version %d",
hotplug_version);
rc = ERROR_FAIL;
goto error;
}
- const int arraysize = 3;
- GCNEW_ARRAY(*args, arraysize);
+ GCNEW_ARRAY(*args, arg_arraysize);
(*args)[nr++] = script;
(*args)[nr++] = (char *) libxl__device_action_to_string(action);
(*args)[nr++] = NULL;
- assert(nr == arraysize);
+ assert(nr == arg_arraysize);
rc = 1;
@@ -244,7 +275,7 @@ int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
rc = 0;
goto out;
}
- rc = libxl__hotplug_disk(gc, dev, args, env, action);
+ rc = libxl__hotplug_disk(gc, dev, args, env, action,
hotplug->version);
break;
case LIBXL__DEVICE_KIND_VIF:
/*
diff --git a/tools/libxl/libxl_types_internal.idl
b/tools/libxl/libxl_types_internal.idl
index cb9444f..253bb18 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -37,4 +37,9 @@ libxl__device_console = Struct("device_console", [
libxl__device_action = Enumeration("device_action", [
(1, "ADD"),
(2, "REMOVE"),
+ (3, "PREPARE"),
+ (4, "UNPREPARE"),
+ (5, "LOCALATTACH"),
+ (6, "LOCALDETACH"),
+ (7, "VERSION"),
])
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 06/12] libxl: add support for different hotplug interfaces
Add a new variable to libxl_device_disk "hotplug_version", that will
be automatically set to the detected hotplug script interface version.
This patch adds the basic support to handle this new scripts, by
adding the prepare/unprepare private libxl functions, and modifying
the current domain creation/destruction to call this functions at the
appropriate spots. The hotplug interface detection is done in the
_prepare call.
This patch also adds some helpers, that will be used here and in later
patches.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 171 +++++++++++++++++++++++++++++++++++++++++-
tools/libxl/libxl_create.c | 62 +++++++++++++++-
tools/libxl/libxl_device.c | 127 ++++++++++++++++++++++++++-----
tools/libxl/libxl_internal.h | 43 +++++++++++
tools/libxl/libxl_types.idl | 1 +
5 files changed, 381 insertions(+), 23 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a67c4dc..9061fdf 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2011,6 +2011,146 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t
domid,
return 0;
}
+/* Callbacks for libxl__device_disk_prepare */
+
+static void device_disk_prepare_cb(libxl__egc *, libxl__ao_device *);
+
+void libxl__device_disk_prepare(libxl__egc *egc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__ao_device *prepare_aodev)
+{
+ STATE_AO_GC(prepare_aodev->ao);
+ char *hotplug_path;
+ char *script_path;
+ int rc;
+ xs_transaction_t t = XBT_NULL;
+ libxl__disk_prepare *dp;
+ libxl__ao_device *aodev;
+
+ if (!disk->script) {
+ /* If script is not set assume v1 */
+ disk->hotplug_version = 1;
+ prepare_aodev->callback(egc, prepare_aodev);
+ return;
+ }
+
+ GCNEW(dp);
+ dp->disk = disk;
+ /* Save original aodev, since we will call it once hotplug
+ * script detection has finished
+ */
+ dp->aodev = prepare_aodev;
+ aodev = &dp->version_aodev;
+ libxl__prepare_ao_device(ao, aodev);
+
+ GCNEW(aodev->dev);
+ rc = libxl__device_from_disk(gc, domid, disk, aodev->dev);
+ if (rc != 0) {
+ LOG(ERROR, "Invalid or unsupported virtual disk identifier
%s",
+ disk->vdev);
+ goto error;
+ }
+
+ script_path = libxl__abs_path(gc, disk->script,
+ libxl__xen_script_dir_path());
+
+ hotplug_path = libxl__device_xs_hotplug_path(gc, aodev->dev);
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto error;
+
+ rc = libxl__xs_write_checked(gc, t,
+ GCSPRINTF("%s/params",
hotplug_path),
+ disk->pdev_path);
+ if (rc)
+ goto error;
+
+ rc = libxl__xs_write_checked(gc, t,
+ GCSPRINTF("%s/script",
hotplug_path),
+ script_path);
+ if (rc)
+ goto error;
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto error;
+ }
+
+ aodev->callback = device_disk_prepare_cb;
+ aodev->action = LIBXL__DEVICE_ACTION_VERSION;
+ aodev->disk = disk;
+ aodev->hotplug.version = 2;
+ libxl__device_hotplug(egc, aodev);
+ return;
+
+error:
+ assert(rc);
+ libxl__xs_transaction_abort(gc, &t);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
+static void device_disk_prepare_cb(libxl__egc *egc,
+ libxl__ao_device *version_aodev)
+{
+ STATE_AO_GC(version_aodev->ao);
+ libxl__disk_prepare *dp = CONTAINER_OF(version_aodev, *dp, version_aodev);
+ libxl__ao_device *aodev = dp->aodev;
+
+ if (dp->disk->hotplug_version == 1) {
+ aodev->callback(egc, aodev);
+ return;
+ }
+
+ aodev->action = LIBXL__DEVICE_ACTION_PREPARE;
+ aodev->hotplug.version = dp->disk->hotplug_version;
+ aodev->dev = version_aodev->dev;
+ libxl__device_hotplug(egc, aodev);
+ return;
+}
+
+void libxl__device_disk_unprepare(libxl__egc *egc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ char *hotplug_path;
+ int rc;
+
+ if (disk->hotplug_version == 1) {
+ aodev->callback(egc, aodev);
+ return;
+ }
+
+ GCNEW(aodev->dev);
+ rc = libxl__device_from_disk(gc, domid, disk, aodev->dev);
+ if (rc != 0) {
+ LOG(ERROR, "Invalid or unsupported virtual disk identifier
%s",
+ disk->vdev);
+ goto error;
+ }
+
+ hotplug_path = libxl__device_xs_hotplug_path(gc, aodev->dev);
+ if (!libxl__xs_read(gc, XBT_NULL, hotplug_path)) {
+ LOG(DEBUG, "unable to unprepare device because %s doesn't
exist",
+ hotplug_path);
+ aodev->callback(egc, aodev);
+ return;
+ }
+
+ aodev->action = LIBXL__DEVICE_ACTION_UNPREPARE;
+ aodev->hotplug.version = disk->hotplug_version;
+ libxl__device_hotplug(egc, aodev);
+ return;
+
+error:
+ assert(rc);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
/* Specific function called directly only by local disk attach,
* all other users should instead use the regular
* libxl__device_disk_add wrapper
@@ -2070,8 +2210,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
dev = disk->pdev_path;
do_backend_phy:
- flexarray_append(back, "params");
- flexarray_append(back, dev);
+ if (disk->hotplug_version == 1) {
+ /*
+ * If the new hotplug version is used params is
+ * stored under a private path, since it can contain
+ * data that the guest should not see.
+ */
+ flexarray_append(back, "params");
+ flexarray_append(back, dev);
+ }
script = libxl__abs_path(gc, disk->script?:
"block",
libxl__xen_script_dir_path());
@@ -2164,6 +2311,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
aodev->dev = device;
aodev->action = LIBXL__DEVICE_ACTION_ADD;
+ aodev->hotplug.version = disk->hotplug_version;
+ aodev->disk = disk;
libxl__wait_device_connection(egc, aodev);
rc = 0;
@@ -2257,6 +2406,7 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t
domid,
GC_INIT(ctx);
char *dompath, *path;
int devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
+ libxl__device dev;
int rc = ERROR_FAIL;
if (devid < 0)
@@ -2275,6 +2425,23 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t
domid,
goto out;
rc = libxl__device_disk_from_xs_be(gc, path, disk);
+ if (rc) {
+ LOG(ERROR, "unable to parse disk device from path %s", path);
+ goto out;
+ }
+
+ /* Check if the device is using the new hotplug interface */
+ rc = libxl__device_from_disk(gc, domid, disk, &dev);
+ if (rc) {
+ LOG(ERROR, "invalid or unsupported virtual disk identifier
%s",
+ disk->vdev);
+ goto out;
+ }
+ disk->hotplug_version = 1;
+ path = libxl__device_xs_hotplug_path(gc, &dev);
+ if (libxl__xs_read(gc, XBT_NULL, path))
+ disk->hotplug_version = 2;
+
out:
GC_FREE;
return rc;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a8dfe61..aadec00 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -554,6 +554,10 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
*/
/* Event callbacks, in this order: */
+
+static void domcreate_launch_bootloader(libxl__egc *egc,
+ libxl__multidev *multidev,
+ int ret);
static void domcreate_devmodel_started(libxl__egc *egc,
libxl__dm_spawn_state *dmss,
int rc);
@@ -590,6 +594,12 @@ static void domcreate_destruction_cb(libxl__egc *egc,
libxl__domain_destroy_state *dds,
int rc);
+/* If creation is not successful, this callback will be executed
+ * when devices have been unprepared */
+static void domcreate_unprepare_cb(libxl__egc *egc,
+ libxl__multidev *multidev,
+ int ret);
+
static void initiate_domain_create(libxl__egc *egc,
libxl__domain_create_state *dcs)
{
@@ -600,7 +610,6 @@ static void initiate_domain_create(libxl__egc *egc,
/* convenience aliases */
libxl_domain_config *const d_config = dcs->guest_config;
- const int restore_fd = dcs->restore_fd;
memset(&dcs->build_state, 0, sizeof(dcs->build_state));
domid = 0;
@@ -633,6 +642,33 @@ static void initiate_domain_create(libxl__egc *egc,
if (ret) goto error_out;
}
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_launch_bootloader;
+ libxl__prepare_disks(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_launch_bootloader(libxl__egc *egc,
+ libxl__multidev *multidev,
+ int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+ STATE_AO_GC(dcs->ao);
+
+ /* convenience aliases */
+ const int restore_fd = dcs->restore_fd;
+ libxl_domain_config *const d_config = dcs->guest_config;
+
+ if (ret) {
+ LOG(ERROR, "unable to prepare devices");
+ goto error_out;
+ }
+
dcs->bl.ao = ao;
libxl_device_disk *bootdisk d_config->num_disks > 0 ?
&d_config->disks[0] : NULL;
@@ -1171,11 +1207,35 @@ static void domcreate_destruction_cb(libxl__egc *egc,
{
STATE_AO_GC(dds->ao);
libxl__domain_create_state *dcs = CONTAINER_OF(dds, *dcs, dds);
+ uint32_t domid = dcs->guest_domid;
+ libxl_domain_config *const d_config = dcs->guest_config;
if (rc)
LOG(ERROR, "unable to destroy domain %u following failed
creation",
dds->domid);
+ /*
+ * We might have devices that have been prepared, but with no
+ * frontend xenstore entries, so domain destruction fails to
+ * find them, that is why we have to unprepare them manually.
+ */
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_unprepare_cb;
+ libxl__unprepare_disks(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
+ return;
+}
+
+static void domcreate_unprepare_cb(libxl__egc *egc,
+ libxl__multidev *multidev,
+ int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+ STATE_AO_GC(dcs->ao);
+
+ if (ret)
+ LOG(ERROR, "unable to unprepare devices");
+
dcs->callback(egc, dcs, ERROR_FAIL, dcs->guest_domid);
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 904853a..df4ea11 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -498,18 +498,21 @@ void libxl__multidev_prepared(libxl__egc *egc,
/******************************************************************************/
-/* Macro for defining the functions that will add a bunch of disks when
+/* Macro for defining the functions that will operate a bunch of devices when
* inside an async op with multidev.
* This macro is added to prevent repetition of code.
*
* The following functions are defined:
+ * libxl__prepare_disks
+ * libxl__unprepare_disks
* libxl__add_disks
* libxl__add_nics
* libxl__add_vtpms
*/
-#define DEFINE_DEVICES_ADD(type) \
- void libxl__add_##type##s(libxl__egc *egc, libxl__ao *ao, uint32_t domid, \
+#define DEFINE_DEVICES_FUNC(type, op) \
+ void libxl__##op##_##type##s(libxl__egc *egc, libxl__ao *ao, \
+ uint32_t domid, \
libxl_domain_config *d_config, \
libxl__multidev *multidev) \
{ \
@@ -517,16 +520,19 @@ void libxl__multidev_prepared(libxl__egc *egc,
int i; \
for (i = 0; i < d_config->num_##type##s; i++) { \
libxl__ao_device *aodev = libxl__multidev_prepare(multidev); \
- libxl__device_##type##_add(egc, domid,
&d_config->type##s[i], \
+ libxl__device_##type##_##op(egc, domid,
&d_config->type##s[i], \
aodev); \
} \
}
-DEFINE_DEVICES_ADD(disk)
-DEFINE_DEVICES_ADD(nic)
-DEFINE_DEVICES_ADD(vtpm)
+DEFINE_DEVICES_FUNC(disk, add)
+DEFINE_DEVICES_FUNC(nic, add)
+DEFINE_DEVICES_FUNC(vtpm, add)
-#undef DEFINE_DEVICES_ADD
+DEFINE_DEVICES_FUNC(disk, prepare)
+DEFINE_DEVICES_FUNC(disk, unprepare)
+
+#undef DEFINE_DEVICES_FUNC
/******************************************************************************/
@@ -534,6 +540,7 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
const char *be_path = libxl__device_backend_path(gc, dev);
const char *fe_path = libxl__device_frontend_path(gc, dev);
+ const char *hotplug_path = libxl__device_xs_hotplug_path(gc, dev);
const char *tapdisk_path = GCSPRINTF("%s/%s", be_path,
"tapdisk-params");
const char *tapdisk_params;
xs_transaction_t t = 0;
@@ -549,6 +556,7 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
libxl__xs_path_cleanup(gc, t, fe_path);
libxl__xs_path_cleanup(gc, t, be_path);
+ libxl__xs_path_cleanup(gc, t, hotplug_path);
rc = libxl__xs_transaction_commit(gc, &t);
if (!rc) break;
@@ -621,6 +629,14 @@ void libxl__devices_destroy(libxl__egc *egc,
libxl__devices_remove_state *drs)
continue;
}
aodev = libxl__multidev_prepare(multidev);
+ /*
+ * Check if the device has hotplug entries in xenstore,
+ * which would mean it's using the new hotplug calling
+ * convention.
+ */
+ path = libxl__device_xs_hotplug_path(gc, dev);
+ if (libxl__xs_read(gc, XBT_NULL, path))
+ aodev->hotplug.version = 2;
aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
aodev->dev = dev;
aodev->force = drs->force;
@@ -673,8 +689,6 @@ static void device_backend_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
static void device_backend_cleanup(libxl__gc *gc,
libxl__ao_device *aodev);
-static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
-
static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
const struct timeval *requested_abs);
@@ -709,7 +723,7 @@ void libxl__wait_device_connection(libxl__egc *egc,
libxl__ao_device *aodev)
* If Qemu is running, it will set the state of the device to
* 4 directly, without waiting in state 2 for any hotplug execution.
*/
- device_hotplug(egc, aodev);
+ libxl__device_hotplug(egc, aodev);
return;
}
@@ -841,7 +855,7 @@ static void device_qemu_timeout(libxl__egc *egc,
libxl__ev_time *ev,
rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
if (rc) goto out;
- device_hotplug(egc, aodev);
+ libxl__device_hotplug(egc, aodev);
return;
out:
@@ -871,7 +885,7 @@ static void device_backend_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
goto out;
}
- device_hotplug(egc, aodev);
+ libxl__device_hotplug(egc, aodev);
return;
out:
@@ -886,7 +900,7 @@ static void device_backend_cleanup(libxl__gc *gc,
libxl__ao_device *aodev)
libxl__ev_devstate_cancel(gc, &aodev->backend_ds);
}
-static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
+void libxl__device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
char *be_path = libxl__device_backend_path(gc, aodev->dev);
@@ -994,10 +1008,13 @@ static void device_hotplug_child_death_cb(libxl__egc
*egc,
if (hotplug_error)
LOG(ERROR, "script: %s", hotplug_error);
aodev->rc = ERROR_FAIL;
- if (aodev->action == LIBXL__DEVICE_ACTION_ADD)
+ if (aodev->action == LIBXL__DEVICE_ACTION_ADD ||
+ aodev->action == LIBXL__DEVICE_ACTION_PREPARE ||
+ aodev->action == LIBXL__DEVICE_ACTION_LOCALATTACH)
/*
- * Only fail on device connection, on disconnection
- * ignore error, and continue with the remove process
+ * Only fail on device connection, prepare or localattach,
+ * on other cases ignore error, and continue with the remove
+ * process.
*/
goto error;
}
@@ -1007,7 +1024,7 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
* device_hotplug_done breaking the loop.
*/
aodev->hotplug.num_exec++;
- device_hotplug(egc, aodev);
+ libxl__device_hotplug(egc, aodev);
return;
@@ -1019,17 +1036,87 @@ error:
static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
+ char *hotplug_path;
+ const char *pdev, *hotplug_version;
int rc;
device_hotplug_clean(gc, aodev);
- /* Clean xenstore if it's a disconnection */
- if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE) {
+ switch (aodev->action) {
+ case LIBXL__DEVICE_ACTION_REMOVE:
+ switch (aodev->hotplug.version) {
+ case 1:
+ /* Clean xenstore if it's a disconnection */
+ rc = libxl__device_destroy(gc, aodev->dev);
+ if (!aodev->rc)
+ aodev->rc = rc;
+ break;
+ case 2:
+ /*
+ * Chain unprepare hotplug execution
+ * after disconnection of device.
+ */
+ aodev->hotplug.num_exec = 0;
+ aodev->action = LIBXL__DEVICE_ACTION_UNPREPARE;
+ libxl__device_hotplug(egc, aodev);
+ return;
+ default:
+ LOG(ERROR, "unknown hotplug script version (%d)",
+ aodev->hotplug.version);
+ if (!aodev->rc)
+ aodev->rc = ERROR_FAIL;
+ break;
+ }
+ break;
+ case LIBXL__DEVICE_ACTION_UNPREPARE:
+ /* Clean hotplug xenstore path */
rc = libxl__device_destroy(gc, aodev->dev);
if (!aodev->rc)
aodev->rc = rc;
+ break;
+ case LIBXL__DEVICE_ACTION_ADD:
+ if (aodev->hotplug.version == 1)
+ goto out;
+ /*
+ * Update pdev_path in libxl_device_disk to point to the block
+ * device
+ */
+ hotplug_path = libxl__device_xs_hotplug_path(gc, aodev->dev);
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/pdev",
hotplug_path),
+ &pdev);
+ if (rc || !pdev) {
+ LOG(ERROR, "unable to get physical disk from %s",
+ GCSPRINTF("%s/pdev", hotplug_path));
+ aodev->rc = rc ? rc : ERROR_FAIL;
+ goto out;
+ }
+
+ free(aodev->disk->pdev_path);
+ aodev->disk->pdev_path = libxl__strdup(NOGC, pdev);
+ break;
+ case LIBXL__DEVICE_ACTION_VERSION:
+ /* Fetch hotplug version output */
+ hotplug_path = libxl__device_xs_hotplug_path(gc, aodev->dev);
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/version",
hotplug_path),
+ &hotplug_version);
+ if (rc) {
+ LOG(ERROR, "unable to get hotplug_version from %s",
+ GCSPRINTF("%s/version", hotplug_path));
+ aodev->rc = rc;
+ goto out;
+ }
+ if (!hotplug_version)
+ aodev->disk->hotplug_version = 1;
+ else
+ aodev->disk->hotplug_version = atoi(hotplug_version);
+ break;
+ default:
+ break;
}
+out:
aodev->callback(egc, aodev);
return;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e819bbd..67fb95e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2012,6 +2012,12 @@ struct libxl__multidev {
_hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
libxl_device_disk *disk,
libxl__ao_device *aodev);
+_hidden void libxl__device_disk_prepare(libxl__egc *egc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__ao_device *aodev);
+_hidden void libxl__device_disk_unprepare(libxl__egc *egc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__ao_device *aodev);
/* AO operation to connect a nic device */
_hidden void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
@@ -2088,6 +2094,28 @@ _hidden int libxl__get_hotplug_script_info(libxl__gc *gc,
libxl__device *dev,
libxl__device_action action,
libxl__hotplug *hotplug);
+/*
+ * libxl__device_hotplug runs the hotplug script associated
+ * with the device passed in aodev->dev.
+ *
+ * The libxl__ao_device passed to this function should be
+ * prepared using libxl__prepare_ao_device prior to calling
+ * this function.
+ *
+ * Once finished, aodev->callback will be executed.
+ */
+_hidden void libxl__device_hotplug(libxl__egc *egc,
+ libxl__ao_device *aodev);
+
+/* Internal structure to hold device_disk_prepare specific information */
+typedef struct libxl__disk_prepare libxl__disk_prepare;
+struct libxl__disk_prepare {
+ libxl__ao_device version_aodev;
+ libxl__ao_device *aodev;
+ libxl_device_disk *disk;
+ libxl__device_callback *callback;
+};
+
/*----- local disk attach: attach a disk locally to run the bootloader -----*/
typedef struct libxl__disk_local_state libxl__disk_local_state;
@@ -2461,6 +2489,21 @@ _hidden void libxl__add_disks(libxl__egc *egc, libxl__ao
*ao, uint32_t domid,
libxl_domain_config *d_config,
libxl__multidev *multidev);
+_hidden void libxl__prepare_disks(libxl__egc *egc, libxl__ao *ao,
+ uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__multidev *multidev);
+
+_hidden void libxl__prepare_undisks(libxl__egc *egc, libxl__ao *ao,
+ uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__multidev *multidev);
+
+_hidden void libxl__unprepare_disks(libxl__egc *egc, libxl__ao *ao,
+ uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__multidev *multidev);
+
_hidden void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
libxl_domain_config *d_config,
libxl__multidev *multidev);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index acc4bc9..8d8cd6a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -367,6 +367,7 @@ libxl_device_disk = Struct("device_disk", [
("removable", integer),
("readwrite", integer),
("is_cdrom", integer),
+ ("hotplug_version", integer),
])
libxl_device_nic = Struct("device_nic", [
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface
Add public functions for the prepare/unprepare disk operations.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 22 +++++++++++++---------
tools/libxl/libxl.h | 8 ++++++++
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9061fdf..7e67e61 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1688,7 +1688,7 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid,
int autopass)
/******************************************************************************/
/* generic callback for devices that only need to set ao_complete */
-static void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
+static void device_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
@@ -3539,7 +3539,7 @@ out:
libxl__prepare_ao_device(ao, aodev); \
aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
aodev->dev = device; \
- aodev->callback = device_addrm_aocomplete; \
+ aodev->callback = device_aocomplete; \
aodev->force = f; \
libxl__initiate_device_remove(egc, aodev); \
\
@@ -3580,10 +3580,12 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
* libxl_device_disk_add
* libxl_device_nic_add
* libxl_device_vtpm_add
+ * libxl_device_disk_prepare
+ * libxl_device_disk_unprepare
*/
-#define DEFINE_DEVICE_ADD(type) \
- int libxl_device_##type##_add(libxl_ctx *ctx, \
+#define DEFINE_DEVICE_FUNC(type, op) \
+ int libxl_device_##type##_##op(libxl_ctx *ctx, \
uint32_t domid, libxl_device_##type *type, \
const libxl_asyncop_how *ao_how) \
{ \
@@ -3592,8 +3594,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
\
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
- aodev->callback = device_addrm_aocomplete; \
- libxl__device_##type##_add(egc, domid, type, aodev); \
+ aodev->callback = device_aocomplete; \
+ libxl__device_##type##_##op(egc, domid, type, aodev); \
\
return AO_INPROGRESS; \
}
@@ -3601,13 +3603,15 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
/* Define alladd functions and undef the macro */
/* disk */
-DEFINE_DEVICE_ADD(disk)
+DEFINE_DEVICE_FUNC(disk, add)
+DEFINE_DEVICE_FUNC(disk, prepare)
+DEFINE_DEVICE_FUNC(disk, unprepare)
/* nic */
-DEFINE_DEVICE_ADD(nic)
+DEFINE_DEVICE_FUNC(nic, add)
/* vtpm */
-DEFINE_DEVICE_ADD(vtpm)
+DEFINE_DEVICE_FUNC(vtpm, add)
#undef DEFINE_DEVICE_ADD
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 72e5efb..eda27e2 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -687,6 +687,10 @@ void libxl_vtpminfo_list_free(libxl_vtpminfo *, int
nr_vtpms);
*/
/* Disks */
+int libxl_device_disk_prepare(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
const libxl_asyncop_how *ao_how)
@@ -699,6 +703,10 @@ int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t
domid,
libxl_device_disk *disk,
const libxl_asyncop_how *ao_how)
LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_disk_unprepare(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int
*num);
int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 08/12] libxl: add disk specific remove functions
Add a specific macro to generate libxl_device_disk_{remove/destroy}
functions that passes the hotplug_version field down to the aodev
struct.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 35 +++++++++++++++++++++++++++++++++--
1 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7e67e61..80782cb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3548,11 +3548,41 @@ out:
return AO_INPROGRESS; \
}
+/* Specific for disk devices, that have to set aodev->hotplug_version */
+#define DEFINE_DISK_REMOVE(type, removedestroy, f) \
+ int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
+ uint32_t domid, libxl_device_##type *type, \
+ const libxl_asyncop_how *ao_how) \
+ { \
+ AO_CREATE(ctx, domid, ao_how); \
+ libxl__device *device; \
+ libxl__ao_device *aodev; \
+ int rc; \
+ \
+ GCNEW(device); \
+ rc = libxl__device_from_##type(gc, domid, type, device); \
+ if (rc != 0) goto out; \
+ \
+ GCNEW(aodev); \
+ libxl__prepare_ao_device(ao, aodev); \
+ aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
+ aodev->dev = device; \
+ aodev->callback = device_aocomplete; \
+ aodev->force = f; \
+ aodev->hotplug.version = type->hotplug_version; \
+ LOG(DEBUG, "hotplug version: %d", aodev->hotplug.version);
\
+ libxl__initiate_device_remove(egc, aodev); \
+ \
+ out: \
+ if (rc) return AO_ABORT(rc); \
+ return AO_INPROGRESS; \
+ }
+
/* Define all remove/destroy functions and undef the macro */
/* disk */
-DEFINE_DEVICE_REMOVE(disk, remove, 0)
-DEFINE_DEVICE_REMOVE(disk, destroy, 1)
+DEFINE_DISK_REMOVE(disk, remove, 0)
+DEFINE_DISK_REMOVE(disk, destroy, 1)
/* nic */
DEFINE_DEVICE_REMOVE(nic, remove, 0)
@@ -3571,6 +3601,7 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
+#undef DEFINE_DISK_REMOVE
#undef DEFINE_DEVICE_REMOVE
/******************************************************************************/
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 09/12] xl: add support for new hotplug interface to block-attach/detach
Call prepare before attaching a block device.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e964bf1..c4af90f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5649,6 +5649,11 @@ int main_blockattach(int argc, char **argv)
return 0;
}
+ if (libxl_device_disk_prepare(ctx, fe_domid, &disk, 0)) {
+ fprintf(stderr, "libxl_device_disk_prepare failed.\n");
+ return 1;
+ }
+
if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
fprintf(stderr, "libxl_device_disk_add failed.\n");
}
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 10/12] libxl: add local attach support for new hotplug scripts
Adds support for locally attaching disks that use the new hotplug
script interface, by calling the localattach/localdetach operations
and returning a block device that can be used to execute pygrub.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 86 ++++++++++++++++++++++++++++++++++------
tools/libxl/libxl_bootloader.c | 1 +
tools/libxl/libxl_internal.h | 1 +
3 files changed, 76 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 80782cb..5a8c2cc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2709,6 +2709,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc
*egc,
const libxl_device_disk *in_disk = dls->in_disk;
libxl_device_disk *disk = &dls->disk;
const char *blkdev_start = dls->blkdev_start;
+ uint32_t domid = dls->domid;
assert(in_disk->pdev_path);
@@ -2725,6 +2726,23 @@ void libxl__device_disk_local_initiate_attach(libxl__egc
*egc,
case LIBXL_DISK_BACKEND_PHY:
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk
%s",
disk->pdev_path);
+ if (disk->hotplug_version == 2) {
+ disk->vdev = libxl__strdup(gc, in_disk->vdev);
+ libxl__prepare_ao_device(ao, &dls->aodev);
+ GCNEW(dls->aodev.dev);
+ rc = libxl__device_from_disk(gc, domid, disk,
+ dls->aodev.dev);
+ if (rc != 0) {
+ LOG(ERROR, "Invalid or unsupported virtual disk "
+ "identifier %s", disk->vdev);
+ goto out;
+ }
+ dls->aodev.action = LIBXL__DEVICE_ACTION_LOCALATTACH;
+ dls->aodev.hotplug.version = disk->hotplug_version;
+ dls->aodev.callback = local_device_attach_cb;
+ libxl__device_hotplug(egc, &dls->aodev);
+ return;
+ }
dev = disk->pdev_path;
break;
case LIBXL_DISK_BACKEND_TAP:
@@ -2787,7 +2805,8 @@ static void local_device_attach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
libxl__disk_local_state *dls = CONTAINER_OF(aodev, *dls, aodev);
- char *dev = NULL, *be_path = NULL;
+ char *dev = NULL, *be_path = NULL, *hotplug_path;
+ const char *pdev = NULL;
int rc;
libxl__device device;
libxl_device_disk *disk = &dls->disk;
@@ -2800,20 +2819,45 @@ static void local_device_attach_cb(libxl__egc *egc,
libxl__ao_device *aodev)
aodev->dev->devid);
goto out;
}
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ /* Fetch hotplug output to obtain the block device */
+ rc = libxl__device_from_disk(gc, dls->domid, disk, &device);
+ if (rc)
+ goto out;
+ hotplug_path = libxl__device_xs_hotplug_path(gc, &device);
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/pdev",
hotplug_path),
+ &pdev);
+ if (rc)
+ goto out;
+ if (!pdev) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ LOG(DEBUG, "attached local block device %s", pdev);
+ dls->diskpath = libxl__strdup(gc, pdev);
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ dev = GCSPRINTF("/dev/%s", disk->vdev);
+ LOG(DEBUG, "locally attaching qdisk %s", dev);
- dev = GCSPRINTF("/dev/%s", disk->vdev);
- LOG(DEBUG, "locally attaching qdisk %s", dev);
+ rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk,
&device);
+ if (rc < 0)
+ goto out;
+ be_path = libxl__device_backend_path(gc, &device);
+ rc = libxl__wait_for_backend(gc, be_path, "4");
+ if (rc < 0)
+ goto out;
- rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
- if (rc < 0)
- goto out;
- be_path = libxl__device_backend_path(gc, &device);
- rc = libxl__wait_for_backend(gc, be_path, "4");
- if (rc < 0)
+ if (dev != NULL)
+ dls->diskpath = libxl__strdup(gc, dev);
+ break;
+ default:
+ LOG(ERROR, "invalid disk backend for local attach callback");
+ rc = ERROR_FAIL;
goto out;
-
- if (dev != NULL)
- dls->diskpath = libxl__strdup(gc, dev);
+ }
dls->callback(egc, dls, 0);
return;
@@ -2837,11 +2881,29 @@ void libxl__device_disk_local_initiate_detach(libxl__egc
*egc,
libxl_device_disk *disk = &dls->disk;
libxl__device *device;
libxl__ao_device *aodev = &dls->aodev;
+ uint32_t domid = dls->domid;
libxl__prepare_ao_device(ao, aodev);
if (!dls->diskpath) goto out;
switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ if (disk->hotplug_version == 2) {
+ GCNEW(aodev->dev);
+ rc = libxl__device_from_disk(gc, domid, disk,
+ aodev->dev);
+ if (rc != 0) {
+ LOG(ERROR, "Invalid or unsupported virtual disk "
+ "identifier %s", disk->vdev);
+ goto out;
+ }
+ aodev->action = LIBXL__DEVICE_ACTION_LOCALDETACH;
+ aodev->hotplug.version = disk->hotplug_version;
+ aodev->callback = local_device_detach_cb;
+ libxl__device_hotplug(egc, aodev);
+ return;
+ }
+ break;
case LIBXL_DISK_BACKEND_QDISK:
if (disk->vdev != NULL) {
GCNEW(device);
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index ed12b2c..5d889fe 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -381,6 +381,7 @@ void libxl__bootloader_run(libxl__egc *egc,
libxl__bootloader_state *bl)
bl->dls.ao = ao;
bl->dls.in_disk = bl->disk;
bl->dls.blkdev_start = info->blkdev_start;
+ bl->dls.domid = domid;
bl->dls.callback = bootloader_disk_attached_cb;
libxl__device_disk_local_initiate_attach(egc, &bl->dls);
return;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 67fb95e..293d0eb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2133,6 +2133,7 @@ struct libxl__disk_local_state {
libxl_device_disk disk;
const char *blkdev_start;
libxl__disk_local_state_callback *callback;
+ uint32_t domid;
/* filled by libxl__device_disk_local_initiate_attach */
char *diskpath;
/* private for implementation of local detach */
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 11/12] hotplug: document new hotplug interface
Mention the new diskspec parameter and add a document explaining the new hotplug interface. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> --- docs/misc/libxl-hotplug-interface.txt | 167 +++++++++++++++++++++++++++++++++ 1 files changed, 167 insertions(+), 0 deletions(-) create mode 100644 docs/misc/libxl-hotplug-interface.txt diff --git a/docs/misc/libxl-hotplug-interface.txt b/docs/misc/libxl-hotplug-interface.txt new file mode 100644 index 0000000..2ee0ebd --- /dev/null +++ b/docs/misc/libxl-hotplug-interface.txt @@ -0,0 +1,167 @@ + ----------------------- + LIBXL HOTPLUG INTERFACE + ----------------------- + +This document specifies the new libxl hotplug interface. This new +interface has been designed to operate better with complex hotplug +scripts, that need to perform several operations and can take a +considerable time to execute. + +Hotplug scripts are expected to take a parameter, passed by the caller +and provide a block device as output, that will be attached to the guest. + + + +====================+ENVIRONMENT VARIABLES +====================+ + +The following environment variables will be set before calling +the hotplug script. + + +HOTPLUG_PATH +------------ + +Points to the xenstore directory that holds information relative +to this hotplug script. At present only one parameter is passed by +the toolstack, the "params" xenstore entry which contains the "target" +line specified in the diskspec xl disk configuration (pdev_path in +libxl_device_disk struct). + +This xenstore directory will be used to communicate between the +hotplug script and the toolstack, and it can also be used by the +hotplug script to store temporary information. This directory is created +before calling the "prepare" operation, and the toolstack guarantees +that it will not be removed before the "unprepare" operation has been +finished. After that, the toolstack will take the appropriate actions +to remove it. The toolstack guarantees that HOTPLUG_PATH will always +point to a valid xenstore path for all operations. + +The path of this directory follows the syntax: + +/local/domain/<local_domid>/libxl/hotplug/<guest_domid>/<device_id> + +(Note that there is no end slash appended to HOTPLUG_PATH) + + +BACKEND_PATH +------------ + +Points to the xenstore backend path of the corresponding block device. +Since hotplug scripts are always executed in the Domain that acts as +backend for a device, it will always have the following syntax: + +/local/domain/<local_domain>/backend/vbd/<guest_domid>/<device_id> + +(Note that there is no end slash appended to BACKEND_PATH) + +This environment variable is not set for all operations, since some +hotplug operations are executed before the backend xenstore is set up. + + + +======================+COMMAND LINE PARAMETERS +======================+ + +Script will be called with only one parameter, that is either prepare, +add, remove, unprepare, localattach or localdetach. + +VERSION +------- + +Operation used to request the version this hotplug script supports. Failure +to perform this operation will result in hotplug script detected as version 1. + +BACKEND_PATH: not valid + +Expected output: +HOTPLGU_PATH/version = version supported by the hotplug script + + +PREPARE +------- + +This is the first operation that the hotplug script will be requested to +execute. This operation is executed before the disk is connected, to +give the hotplug script the chance to offload some work from the "add" +operation, that is performed later. + +BACKEND_PATH: not valid + +Expected output: +None + + +ADD +--- + +This operation should connect the device to the domain. Will only be called +after the "prepare" operation has finished successfully. + +BACKEND_PATH: valid + +Expected output: +BACKEND_PATH/physical-device = block device major:minor +BACKEND_PATH/params = block device path +HOTPLUG_PATH/pdev = block device path + + +REMOVE +------ + + +Disconnects a block device from a domain. Will only be called +after the "prepare" operation has finished successfully. Implementations +should take into account that the "remove" operation will also be called if +the "add" operation has failed. + +BACKEND_PATH: valid + +Expected output: +None + + +LOCALATTACH +----------- + + +Creates a block device in the current domain that points to the guest +disk device. Will only be called after the "prepare" operation has +finished successfully. + +BACKEND_PATH: not valid + +Expected output: +HOTPLUG_PATH/pdev = block device path + + +LOCALDETACH +----------- + + +Disconnects a device (previously connected with the localattach +operation) from the current domain. Will only be called after +the "prepare" operation has finished successfully. Implementations +should take into account that the "localdetach" operation will +also be called if the "localattach" operation has failed. + +BACKEND_PATH: not valid + +Expected output: +None + + +UNPREPARE +--------- + + +Performs the necessary actions to unprepare the device. + +BACKEND_PATH: not valid + +Expected output: +None -- 1.7.7.5 (Apple Git-26) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Jan-23 17:48 UTC
[PATCH v1 12/12] hotplug/Linux: add iscsi block hotplug script
This hotplug script has been tested with IET and NetBSD iSCSI targets,
without authentication.
Authentication parameters, including the password are passed as
parameters to iscsiadm, which is not recommended because other users
of the system can see them. This parameters could also be set by
editing a corresponding file directly, but the location of this file
seems to be different depending on the distribution used.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/hotplug/Linux/Makefile | 1 +
tools/hotplug/Linux/block-iscsi | 258 +++++++++++++++++++++++++++++++++++++++
2 files changed, 259 insertions(+), 0 deletions(-)
create mode 100644 tools/hotplug/Linux/block-iscsi
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 0605559..98c7738 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -21,6 +21,7 @@ XEN_SCRIPTS += blktap
XEN_SCRIPTS += xen-hotplug-cleanup
XEN_SCRIPTS += external-device-migrate
XEN_SCRIPTS += vscsi
+XEN_SCRIPTS += block-iscsi
XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
XEN_SCRIPT_DATA += block-common.sh
diff --git a/tools/hotplug/Linux/block-iscsi b/tools/hotplug/Linux/block-iscsi
new file mode 100644
index 0000000..0fcc585
--- /dev/null
+++ b/tools/hotplug/Linux/block-iscsi
@@ -0,0 +1,258 @@
+#!/bin/sh
+#
+# Open-iSCSI Xen block device hotplug script
+#
+# Author Roger Pau Monné <roger.pau@citrix.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published
+# by the Free Software Foundation; version 2.1 only. with the special
+# exception on linking described in file LICENSE.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU Lesser General Public License for more details.
+
+remove_label()
+{
+ echo $1 | sed "s/^\("$2"\)//"
+}
+
+check_tools()
+{
+ if ! type iscsiadm > /dev/null 2>&1; then
+ echo "Unable to find iscsiadm tool"
+ return 1
+ fi
+ if [ "$multipath" = "y" ] && ! type multipath
> /dev/null 2>&1; then
+ echo "Unable to find multipath"
+ return 1
+ fi
+}
+
+# Sets the following global variables based on the params field passed in as
+# a parameter: iqn, portal, auth_method, user, multipath, password
+parse_target()
+{
+ # set multipath default value
+ multipath="n"
+ for param in $(echo "$1" | tr "," "\n")
+ do
+ if [ -n "$password" ]; then
+ password="$password,$param"
+ continue
+ fi
+ case $param in
+ iqn=*)
+ iqn=$(remove_label $param "iqn=")
+ ;;
+ portal=*)
+ portal=$(remove_label $param "portal=")
+ ;;
+ auth_method=*)
+ auth_method=$(remove_label $param "auth_method=")
+ ;;
+ user=*)
+ user=$(remove_label $param "user=")
+ ;;
+ multipath=*)
+ multipath=$(remove_label $param "multipath=")
+ ;;
+ password=*)
+ password=$(remove_label $param "password=")
+ ;;
+ esac
+ done
+ if [ -z "$iqn" ] || [ -z "$portal" ]; then
+ echo "iqn= and portal= are required parameters"
+ return 1
+ fi
+ if [ "$multipath" != "y" ] && [
"$multipath" != "n" ]; then
+ echo "multipath valid values are y and n, $multipath not a valid
value"
+ return 1
+ fi
+ return 0
+}
+
+# Outputs the block device major:minor
+device_major_minor()
+{
+ stat -L -c %t:%T "$1"
+}
+
+# Sets $dev to point to the device associated with the value in iqn
+find_device()
+{
+ while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
+ sleep 0.1
+ done
+ sddev=$(readlink -f /dev/disk/by-path/*"$iqn"-lun-0)
+ if [ ! -b "$sddev" ]; then
+ echo "Unable to find attached device path"
+ return 1
+ fi
+ if [ "$multipath" = "y" ]; then
+ mdev=$(multipath -ll "$sddev" | head -1 | awk '{ print
$1}')
+ if [ ! -b /dev/mapper/"$mdev" ]; then
+ echo "Unable to find attached device multipath"
+ return 1
+ fi
+ dev="/dev/mapper/$mdev"
+ else
+ dev="$sddev"
+ fi
+ return 0
+}
+
+# Attaches the target $iqn in $portal and sets $dev to point to the
+# multipath device
+attach()
+{
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--login > /dev/null 2>&1
+ if [ $? -ne 0 ]; then
+ echo "Unable to connect to target"
+ return 1
+ fi
+ find_device
+ if [ $? -ne 0 ]; then
+ echo "Unable to find iSCSI device"
+ return 1
+ fi
+ return 0
+}
+
+# Discovers targets in $portal and checks that $iqn is one of those targets
+# Also sets the auth parameters to attach the device
+prepare()
+{
+ # Check if target is already opened
+ iscsiadm -m session 2>&1 | grep -q "$iqn"
+ if [ $? -eq 0 ]; then
+ echo "Device already opened"
+ return 1
+ fi
+ # Discover portal targets
+ iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q
"$iqn"
+ if [ $? -ne 0 ]; then
+ echo "No matching target iqn found"
+ return 1
+ fi
+ # Set auth method if necessary
+ if [ -n "$auth_method" ] || [ -n "$user" ] || [ -n
"$password" ]; then
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=update --name \
+ node.session.auth.authmethod --value="$auth_method" \
+ > /dev/null 2>&1 && \
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=update --name \
+ node.session.auth.username --value="$user" \
+ > /dev/null 2>&1 && \
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=update --name \
+ node.session.auth.password --value="$password" \
+ > /dev/null 2>&1
+ if [ $? -ne 0 ]; then
+ echo "Unable to set authentication parameters"
+ return 1
+ fi
+ fi
+ return 0
+}
+
+# Attaches the device and writes xenstore backend entries to connect
+# the device
+add()
+{
+ localattach
+ if [ $? -ne 0 ]; then
+ echo "Failed to attach device"
+ return 1
+ fi
+ mm=$(device_major_minor "$dev")
+ xenstore-write "$BACKEND_PATH/physical-device" "$mm"
+ xenstore-write "$BACKEND_PATH/params" "$dev"
+ return 0
+}
+
+# Disconnects the device
+remove()
+{
+ find_device
+ if [ $? -ne 0 ]; then
+ echo "Unable to find device"
+ return 1
+ fi
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--logout > /dev/null 2>&1
+ if [ $? -ne 0 ]; then
+ echo "Unable to disconnect target"
+ return 1
+ fi
+ return 0
+}
+
+# Removes the auth params set by prepare
+unprepare()
+{
+ if [ -n "$auth_method" ] || [ -n "$user" ] || [ -n
"$password" ]; then
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=delete --name \
+ node.session.auth.authmethod > /dev/null 2>&1
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=delete --name \
+ node.session.auth.username > /dev/null 2>&1
+ iscsiadm -m node --targetname "$iqn" -p "$portal"
--op=delete --name \
+ node.session.auth.password > /dev/null 2>&1
+ fi
+}
+
+# Attaches the device to the current domain, and writes the resulting
+# block device to xenstore
+localattach()
+{
+ attach
+ if [ $? -ne 0 ]; then
+ echo "Failed to attach device"
+ return 1
+ fi
+ xenstore-write "$HOTPLUG_PATH/pdev" "$dev"
+ return 0
+}
+
+command=$1
+target=$(xenstore-read $HOTPLUG_PATH/params)
+
+if [ -z "$target" ]; then
+ echo "No information about the target"
+ exit 1
+fi
+
+check_tools || exit 1
+
+parse_target "$target"
+
+case $command in
+prepare)
+ prepare
+ exit $?
+ ;;
+add)
+ add
+ exit $?
+ ;;
+remove)
+ remove
+ exit $?
+ ;;
+unprepare)
+ unprepare
+ exit $?
+ ;;
+localattach)
+ localattach
+ exit $?
+ ;;
+localdetach)
+ remove
+ exit $?
+ ;;
+version)
+ xenstore-write "$HOTPLUG_PATH/version" "2"
+ exit $?
+ ;;
+esac
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Ian Campbell
2013-Jan-25 08:57 UTC
Re: [PATCH v1 01/12] libxl: libxl__prepare_ao_device should reset num_exec
On Wed, 2013-01-23 at 17:48 +0000, Roger Pau Monne wrote:> num_exec was not cleared when calling libxl__prepare_ao_device. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Acked-by: Ian Campbell <ian.campbell@citrix.com>Applied, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Roger Pau Monné
2013-Mar-04 11:13 UTC
Re: [PATCH v1 0/12] libxl: new hotplug calling convention
On 23/01/13 18:48, Roger Pau Monne wrote:> This series implements a new hoplug calling convention for libxl. > > The aim of this new convention is to reduce the blackout phase of > migration when using complex hotplug scripts, like iSCSI or other kind > of storage backends that might have a non trivial setup time. > > There are some issues that I would like to discuss, the first one is > the fact that pdev_path field in libxl_device_disk is no longuer used > to store a physical path, since diskspec "target" can now contain > "random" information to connect a block device. This is not a blocker > for this series to be accepted, but I''m tempted to say that the easier > way to solve this is to use a union for target and pdev_path, this > way everything will work as expected, and semantics will be right. > > The second issue is related to iSCSI and iscsiadm, specifically the > way to set authentication parameters, which is done with command line > parameters or editing files (which each distro seems to place in > different locations). I will look into this to see if we can find a > suitable solution. > > Thanks for the comments, Roger. > > Branch available in the git repository at: > git://xenbits.xen.org/people/royger/xen.git iscsi_v1 > > *Acked > > Roger Pau Monne (12): > *libxl: libxl__prepare_ao_device should reset num_exec > libxl: remove double check in NetBSD hotplug > libxl: move libxl_device_action to idl > libxl: pack hotplug related variables > libxl: add new hotplug interface support to hotplug script callers > libxl: add support for different hotplug interfaces > libxl: add prepare/unprepare operations to the libxl public interface > libxl: add disk specific remove functions > xl: add support for new hotplug interface to block-attach/detach > libxl: add local attach support for new hotplug scripts > hotplug: document new hotplug interface > hotplug/Linux: add iscsi block hotplug script > > docs/misc/libxl-hotplug-interface.txt | 167 +++++++++++++++++ > tools/hotplug/Linux/Makefile | 1 + > tools/hotplug/Linux/block-iscsi | 258 +++++++++++++++++++++++++ > tools/libxl/libxl.c | 332 +++++++++++++++++++++++++++++---- > tools/libxl/libxl.h | 8 + > tools/libxl/libxl_bootloader.c | 1 + > tools/libxl/libxl_create.c | 62 ++++++- > tools/libxl/libxl_device.c | 153 +++++++++++++--- > tools/libxl/libxl_internal.h | 87 ++++++++- > tools/libxl/libxl_linux.c | 78 ++++++--- > tools/libxl/libxl_netbsd.c | 10 +- > tools/libxl/libxl_types.idl | 1 + > tools/libxl/libxl_types_internal.idl | 10 + > tools/libxl/xl_cmdimpl.c | 5 + > 14 files changed, 1071 insertions(+), 102 deletions(-) > create mode 100644 docs/misc/libxl-hotplug-interface.txt > create mode 100644 tools/hotplug/Linux/block-iscsiIt has been some time since I''ve posted the series, should I rebase and resend them?
Ian Jackson
2013-Mar-13 14:45 UTC
Re: [PATCH v1 02/12] libxl: remove double check in NetBSD hotplug
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 02/12] libxl: remove double
check in NetBSD hotplug"):> Remove a duplicated check performed in libxl__get_hotplug_script_info
> for NetBSD
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Jackson
2013-Mar-13 14:48 UTC
Re: [PATCH v1 03/12] libxl: move libxl_device_action to idl
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 03/12] libxl: move
libxl_device_action to idl"):> Move to idl for ease of expansion and auto-generated functions.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Jackson
2013-Mar-13 14:49 UTC
Re: [PATCH v1 04/12] libxl: pack hotplug related variables
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 04/12] libxl: pack hotplug
related variables"):> Create a new struct to hold hotplug related variables that are
> scattered in libxl__ao_device. We will later expand the number of
> hotplug related variables, so it''s best to have them packed
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
I think you should avoid the word "packed" which normally makes people
think of removing structure memory layout padding. "grouped"
perhaps ?
Apart from that, which I think is just a comment about the commit
message:
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
Ian Jackson
2013-Mar-13 14:53 UTC
Re: [PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 05/12] libxl: add new
hotplug interface support to hotplug script
callers"):> Add support to call hotplug scripts that use the new hotplug interface
> to the low-level functions in charge of calling hotplug scripts.
>
> libxl__prepare_ao_device sets the hotplug version to 1, in later
> patches hotplug version will be set to 2 by the caller to use this
> new hotplug calling convention.
I think this needs a comprehensive document describing the new calling
convention. Since there isn''t currently a document describing the old
calling convention I think you may need to do some reverse
engineering.
And then when you do that, there are things about the old calling
convention which are distinctly dodgy and should perhaps be changed.
Is this autoprobing with the "version" argument really the best way to
deal with compatibility ? I guess we do know that it''s reliable.
Ian.
Roger Pau Monné
2013-Mar-13 16:04 UTC
Re: [PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers
On 13/03/13 15:53, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers"): >> Add support to call hotplug scripts that use the new hotplug interface >> to the low-level functions in charge of calling hotplug scripts. >> >> libxl__prepare_ao_device sets the hotplug version to 1, in later >> patches hotplug version will be set to 2 by the caller to use this >> new hotplug calling convention. > > I think this needs a comprehensive document describing the new calling > convention. Since there isn''t currently a document describing the old > calling convention I think you may need to do some reverse > engineering.Patch 11/12 adds a document describing the new calling convention.> > And then when you do that, there are things about the old calling > convention which are distinctly dodgy and should perhaps be changed.I would prefer to avoid touching anything related to the old calling convention, mainly because I would like to avoid touching the old hotplug scripts in this series, and because there might be out-of-tree hotplug scripts relaying on some of this obscure features.> Is this autoprobing with the "version" argument really the best way to > deal with compatibility ? I guess we do know that it''s reliable.I think it''s quite reliable, unless someone has an out-of-tree v1 hotplug script that implements this parameter, but I don''t think this is going to be the case.
Ian Jackson
2013-Mar-13 16:10 UTC
Re: [PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 05/12] libxl: add new
hotplug interface support to hotplug script
callers"):> On 13/03/13 15:53, Ian Jackson wrote:
> > I think this needs a comprehensive document describing the new calling
> > convention. Since there isn''t currently a document
describing the old
> > calling convention I think you may need to do some reverse
> > engineering.
>
> Patch 11/12 adds a document describing the new calling convention.
Surely this should be in the same patch ? In general the document for
something should be in the same patch as the code that introduces it.
TBH I''m not sure I understand the division between the various
patches.
> > And then when you do that, there are things about the old calling
> > convention which are distinctly dodgy and should perhaps be changed.
>
> I would prefer to avoid touching anything related to the old calling
> convention, mainly because I would like to avoid touching the old
> hotplug scripts in this series, and because there might be out-of-tree
> hotplug scripts relaying on some of this obscure features.
Yes, of course. Sorry, I was unclear. I meant that there were
aspects about the old calling convention which you are inheriting into
the new one which may want to be revisited.
> I think it''s quite reliable, unless someone has an out-of-tree v1
> hotplug script that implements this parameter, but I don''t think
this is
> going to be the case.
Right.
Ian.
Ian Jackson
2013-Mar-13 16:17 UTC
Re: [PATCH v1 06/12] libxl: add support for different hotplug interfaces
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 06/12] libxl: add support
for different hotplug interfaces"):> Add a new variable to libxl_device_disk "hotplug_version", that
will
> be automatically set to the detected hotplug script interface version.
...> + /*
> + * We might have devices that have been prepared, but with no
> + * frontend xenstore entries, so domain destruction fails to
> + * find them, that is why we have to unprepare them manually.
> + */
> + libxl__multidev_begin(ao, &dcs->multidev);
> + dcs->multidev.callback = domcreate_unprepare_cb;
> + libxl__unprepare_disks(egc, ao, domid, d_config,
&dcs->multidev);
> + libxl__multidev_prepared(egc, &dcs->multidev, 0);
> + return;
I don''t think this is correct. What if the calling application
crashes and this code is never executed ? Then libxl_destroy would
leak this state.
So I think you need to record the preparedness somewhere where
libxl_destroy can find it.
Ian.
Ian Jackson
2013-Mar-13 16:19 UTC
Re: [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 07/12] libxl: add
prepare/unprepare operations to the libxl public
interface"):> Add public functions for the prepare/unprepare disk operations.
I''m not sure I understand how the caller is supposed to use this.
How does the eventual script manage to correlate the preparation with
the unpreparation ?
How can the caller see what has been prepared ?
Ian.
Ian Jackson
2013-Mar-13 16:22 UTC
Re: [PATCH v1 08/12] libxl: add disk specific remove functions
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 08/12] libxl: add disk
specific remove functions"):> Add a specific macro to generate libxl_device_disk_{remove/destroy}
> functions that passes the hotplug_version field down to the aodev
> struct.
> +/* Specific for disk devices, that have to set aodev->hotplug_version
*/
> +#define DEFINE_DISK_REMOVE(type, removedestroy, f) \
> + int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
This macro is very similar to DEFINE_DEVICE_REMOVE. The only
difference seems to be these extra lines:
> + aodev->hotplug.version = type->hotplug_version;
\
> + LOG(DEBUG, "hotplug version: %d",
aodev->hotplug.version); \
> + libxl__initiate_device_remove(egc, aodev); \
Perhaps the right answer would be to add a new formal parameter to
DEFINE_DEVICE_REMOVE which allows DEFINE_DEVICE_REMOVE''s user to
specify some extra code here ?
Ian.
Ian Jackson
2013-Mar-13 16:22 UTC
Re: [PATCH v1 09/12] xl: add support for new hotplug interface to block-attach/detach
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 09/12] xl: add support for
new hotplug interface to block-attach/detach"):> Call prepare before attaching a block device.
Why is this helpful in xl ?
Ian.
Ian Jackson
2013-Mar-13 16:25 UTC
Re: [PATCH v1 10/12] libxl: add local attach support for new hotplug scripts
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 10/12] libxl: add local
attach support for new hotplug scripts"):> Adds support for locally attaching disks that use the new hotplug
> script interface, by calling the localattach/localdetach operations
> and returning a block device that can be used to execute pygrub.
This is an optimisation, isn''t it ? In the sense that nothing should
go wrong if we don''t have this code ?
Ian.
Ian Jackson
2013-Mar-13 16:29 UTC
Re: [PATCH v1 11/12] hotplug: document new hotplug interface
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 11/12] hotplug: document new
hotplug interface"):> Mention the new diskspec parameter and add a document explaining the
> new hotplug interface.
...> +HOTPLUG_PATH
> +------------
> +
> +Points to the xenstore directory that holds information relative
> +to this hotplug script. At present only one parameter is passed by
> +the toolstack, the "params" xenstore entry which contains the
"target"
> +line specified in the diskspec xl disk configuration (pdev_path in
> +libxl_device_disk struct).
> +
> +This xenstore directory will be used to communicate between the
> +hotplug script and the toolstack, and it can also be used by the
> +hotplug script to store temporary information.
Doesn''t this leave the possibility that a script might decide to write
"sponge"="42" in the xenstore directory, for its own
purposes, and
that libxl might later decide to try to pass "sponge"="bath"
for an
enhanced interface ?
I think you need to specify which names each user is entitled to use.
Also, you need to say something about access control.
> +======================> +COMMAND LINE PARAMETERS
> +======================...
> +Script will be called with only one parameter, that is either prepare,
> +add, remove, unprepare, localattach or localdetach.
You need to say what scripts should do with unknown parameters.
> +BACKEND_PATH: not valid
> +
> +Expected output:
> +HOTPLGU_PATH/version = version supported by the hotplug script
The output is written into xenstore ? (Also, typo.)
thanks,
Ian.
Ian Jackson
2013-Mar-13 16:31 UTC
Re: [PATCH v1 12/12] hotplug/Linux: add iscsi block hotplug script
Roger Pau Monne writes ("[Xen-devel] [PATCH v1 12/12] hotplug/Linux: add
iscsi block hotplug script"):> This hotplug script has been tested with IET and NetBSD iSCSI targets,
> without authentication.
>
> Authentication parameters, including the password are passed as
> parameters to iscsiadm, which is not recommended because other users
> of the system can see them. This parameters could also be set by
> editing a corresponding file directly, but the location of this file
> seems to be different depending on the distribution used.
Would it be possible for the user to specify the file that the
password should be read from ?
Did you write this script from scratch ? (TBH I don''t intend to
review it in detail...)
Why no set -e ?
Ian.
Roger Pau Monné
2013-Mar-14 16:44 UTC
Re: [PATCH v1 04/12] libxl: pack hotplug related variables
On 13/03/13 15:49, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 04/12] libxl: pack hotplug related variables"): >> Create a new struct to hold hotplug related variables that are >> scattered in libxl__ao_device. We will later expand the number of >> hotplug related variables, so it''s best to have them packed >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> Cc: Ian Campbell <ian.campbell@citrix.com> > > I think you should avoid the word "packed" which normally makes people > think of removing structure memory layout padding. "grouped" > perhaps ? > > Apart from that, which I think is just a comment about the commit > message: > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>Thanks, I''ve fixed the commit message and added your Ack.
Roger Pau Monné
2013-Mar-14 17:03 UTC
Re: [PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers
On 13/03/13 17:10, Ian Jackson wrote:> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 05/12] libxl: add new hotplug interface support to hotplug script callers"): >> On 13/03/13 15:53, Ian Jackson wrote: >>> I think this needs a comprehensive document describing the new calling >>> convention. Since there isn''t currently a document describing the old >>> calling convention I think you may need to do some reverse >>> engineering. >> >> Patch 11/12 adds a document describing the new calling convention. > > Surely this should be in the same patch ? In general the document for > something should be in the same patch as the code that introduces it. > > TBH I''m not sure I understand the division between the various > patches.I''ve just added it as a separate patch because I wanted to add it once the implementation in libxl was complete, but I don''t have any problem squashing the doc into this patch. Regarding the order of the series, I''ve basically added the core functionality first in this patch, and then I went adding the missing bits all over the other patches, trying to keep them small and concrete.> >>> And then when you do that, there are things about the old calling >>> convention which are distinctly dodgy and should perhaps be changed. >> >> I would prefer to avoid touching anything related to the old calling >> convention, mainly because I would like to avoid touching the old >> hotplug scripts in this series, and because there might be out-of-tree >> hotplug scripts relaying on some of this obscure features. > > Yes, of course. Sorry, I was unclear. I meant that there were > aspects about the old calling convention which you are inheriting into > the new one which may want to be revisited.I''m not sure I follow you here, there''s a switch in libxl__hotplug_disk that passes completely different parameters and env vars depending on the hotplug script version, so there''s no inheritance between the old and the new calling convention.
Roger Pau Monné
2013-Mar-15 11:03 UTC
Re: [PATCH v1 06/12] libxl: add support for different hotplug interfaces
On 13/03/13 17:17, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 06/12] libxl: add support for different hotplug interfaces"): >> Add a new variable to libxl_device_disk "hotplug_version", that will >> be automatically set to the detected hotplug script interface version. > ... >> + /* >> + * We might have devices that have been prepared, but with no >> + * frontend xenstore entries, so domain destruction fails to >> + * find them, that is why we have to unprepare them manually. >> + */ >> + libxl__multidev_begin(ao, &dcs->multidev); >> + dcs->multidev.callback = domcreate_unprepare_cb; >> + libxl__unprepare_disks(egc, ao, domid, d_config, &dcs->multidev); >> + libxl__multidev_prepared(egc, &dcs->multidev, 0); >> + return; > > I don''t think this is correct. What if the calling application > crashes and this code is never executed ? Then libxl_destroy would > leak this state. > > So I think you need to record the preparedness somewhere where > libxl_destroy can find it.Right, I''ve added a new xenstore path that has the following nomenclature: /libxl/<domid>/hotplug/status/<devid> Note that it doesn''t contain the backend domid, so it can be parsed inside libxl__devices_destroy in order to find devices using the new hotplug interface (even devices that have been prepared but not attached to the domain). This path is created after the device has been prepared, and is cleaned after the device has been unprepared. It only contains the necessary information to be able to fill a libxl__device struct.
Ian Jackson
2013-Mar-15 11:24 UTC
Re: [PATCH v1 06/12] libxl: add support for different hotplug interfaces
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 06/12] libxl: add
support for different hotplug interfaces"):> Right, I''ve added a new xenstore path that has the following
nomenclature:
>
> /libxl/<domid>/hotplug/status/<devid>
That sounds plausible. What''s the value ?
> This path is created after the device has been prepared, and is cleaned
> after the device has been unprepared. It only contains the necessary
> information to be able to fill a libxl__device struct.
I think it needs to be created before you start preparation, so that
preparation attempts which crash halfway through are recorded.
Ian.
Roger Pau Monné
2013-Mar-15 11:33 UTC
Re: [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface
On 13/03/13 17:19, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface"): >> Add public functions for the prepare/unprepare disk operations. > > I''m not sure I understand how the caller is supposed to use this.The caller should execute prepare before adding the disk (hopefully in a not performance sensitive path).> How does the eventual script manage to correlate the preparation with > the unpreparation ?You call the script with the prepare or unprepare parameters, so the script knows which action it has to execute.> How can the caller see what has been prepared ?The caller should take care of this states by himself (just as we do with add or remove), so it knows if the device has been prepared or not. The only downfall of not calling prepare before add is that libxl will thread this device as using the v1 hotplug interface, so old users of libxl will still have the same functionality, but they will not be able to use new hotplug scripts using the v2 interface. This functions are only used for disk attach/detach (xl block-attach/block-detach), for domain creation the domain builder logic already takes care of preparing the devices before adding them.
Roger Pau Monné
2013-Mar-15 11:36 UTC
Re: [PATCH v1 06/12] libxl: add support for different hotplug interfaces
On 15/03/13 12:24, Ian Jackson wrote:> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 06/12] libxl: add support for different hotplug interfaces"): >> Right, I''ve added a new xenstore path that has the following nomenclature: >> >> /libxl/<domid>/hotplug/status/<devid> > > That sounds plausible. What''s the value ?There''s only one value: /libxl/<domid>/hotplug/status/<devid>/backend_domid = <backend_dom> That''s all we need to be able to fill a libxl__device structure. I''m also thinking that it might be prudent to use /libxl/<domid>/hotplug/status/vbd/<devid>/backend_domid = <backend_dom> Right now vbds are the only users of the new hotplug interface, but who knows if we might want to use it for nics in the future too.> >> This path is created after the device has been prepared, and is cleaned >> after the device has been unprepared. It only contains the necessary >> information to be able to fill a libxl__device struct. > > I think it needs to be created before you start preparation, so that > preparation attempts which crash halfway through are recorded.Yes, I was also thinking about it after sending the reply...
Roger Pau Monné
2013-Mar-15 11:42 UTC
Re: [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface
On 15/03/13 12:33, Roger Pau Monné wrote:> The caller should take care of this states by himself (just as we do > with add or remove), so it knows if the device has been prepared or not.Just as a clarification, the way to know if a device has been prepared but not added is to look for the xenstore entries. If the device only has /libxl/<domid>/hotplug/status/<devid>/ then it has been prepared but not added, on the other hand, if the device has the above xenstore path, and also /local/domain/<domid>/device/vbd/<devid>/backend then it has been prepared and added. But I can''t come up with any reason a libxl user should look at this paths by himself.
Roger Pau Monné
2013-Mar-15 11:52 UTC
Re: [PATCH v1 08/12] libxl: add disk specific remove functions
On 13/03/13 17:22, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 08/12] libxl: add disk specific remove functions"): >> Add a specific macro to generate libxl_device_disk_{remove/destroy} >> functions that passes the hotplug_version field down to the aodev >> struct. > >> +/* Specific for disk devices, that have to set aodev->hotplug_version */ >> +#define DEFINE_DISK_REMOVE(type, removedestroy, f) \ >> + int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \ > > This macro is very similar to DEFINE_DEVICE_REMOVE. The only > difference seems to be these extra lines: > >> + aodev->hotplug.version = type->hotplug_version; \ >> + LOG(DEBUG, "hotplug version: %d", aodev->hotplug.version); \ >> + libxl__initiate_device_remove(egc, aodev); \ > > Perhaps the right answer would be to add a new formal parameter to > DEFINE_DEVICE_REMOVE which allows DEFINE_DEVICE_REMOVE''s user to > specify some extra code here ?type->hotplug_version is only available in libxl_device_disk, so even if using something like if (param) aodev->hotplug.version = type->hotplug_version; Would break compilation because other device types don''t have the hotplug_version field (and adding a hotplug_version field to all device structures seems overkill).
Roger Pau Monné
2013-Mar-15 11:54 UTC
Re: [PATCH v1 09/12] xl: add support for new hotplug interface to block-attach/detach
On 13/03/13 17:22, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 09/12] xl: add support for new hotplug interface to block-attach/detach"): >> Call prepare before attaching a block device. > > Why is this helpful in xl ?prepare function gets the hotplug version of the script and also executes the prepare hotplug action if necessary. Failing to call prepare would assume hotplug version is v1, so we won''t be able to block-attach devices using the v2 hotplug interface.
Roger Pau Monné
2013-Mar-15 11:59 UTC
Re: [PATCH v1 10/12] libxl: add local attach support for new hotplug scripts
On 13/03/13 17:25, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 10/12] libxl: add local attach support for new hotplug scripts"): >> Adds support for locally attaching disks that use the new hotplug >> script interface, by calling the localattach/localdetach operations >> and returning a block device that can be used to execute pygrub. > > This is an optimisation, isn''t it ? In the sense that nothing should > go wrong if we don''t have this code ?The status-quo right now is that custom hotplug scripts cannot be used in conjunction with pygrub, because we need to pass pygrub either a block device or a raw file. This patch adds the necessary functionality in order to attach the block device before executing pygrub, so we can use this new hotplug scripts with pygrub, providing extra functionality.
Roger Pau Monné
2013-Mar-15 12:08 UTC
Re: [PATCH v1 12/12] hotplug/Linux: add iscsi block hotplug script
On 13/03/13 17:31, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 12/12] hotplug/Linux: add iscsi block hotplug script"): >> This hotplug script has been tested with IET and NetBSD iSCSI targets, >> without authentication. >> >> Authentication parameters, including the password are passed as >> parameters to iscsiadm, which is not recommended because other users >> of the system can see them. This parameters could also be set by >> editing a corresponding file directly, but the location of this file >> seems to be different depending on the distribution used. > > Would it be possible for the user to specify the file that the > password should be read from ?AFAIK no, this is read from a system defined file, that can be different depending on the distro used. Maybe be could expand this script latter if needed, in order to be able to detect where this file resides and use that instead of passing the user/password in the command line.> > Did you write this script from scratch ? (TBH I don''t intend to > review it in detail...)I''ve found a previous version of iSCSI and Xen here: http://backdrift.org/xen-block-iscsi-script-with-multipath-support but I didn''t use it, so yes.> Why no set -e ?Because some of the executed commands might fail, but that''s expected (and shouldn''t abort the execution) and we manually check for the return value of executed commands.> Ian. >
Ian Jackson
2013-Mar-15 12:20 UTC
Re: [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 07/12] libxl: add
prepare/unprepare operations to the libxl public
interface"):> On 13/03/13 17:19, Ian Jackson wrote:
> > I''m not sure I understand how the caller is supposed to use
this.
>
> The caller should execute prepare before adding the disk (hopefully in a
> not performance sensitive path).
That''s a non-backward-compatible API change, then ? We''re
trying to
avoid those.
> > How does the eventual script manage to correlate the preparation with
> > the unpreparation ?
>
> You call the script with the prepare or unprepare parameters, so the
> script knows which action it has to execute.
No, I mean, is it just the target string that is relevant for
identifying the preparation ? Can the same disk be prepared multiple
times (eg for multiple guests) ? How is this synchronised ?
> The only downfall of not calling prepare before add is that libxl will
> thread this device as using the v1 hotplug interface, so old users of
> libxl will still have the same functionality, but they will not be able
> to use new hotplug scripts using the v2 interface.
Perhaps it would be better to hide this from the caller and implement
the old API in terms of the new one ?
ian.
Ian Jackson
2013-Mar-15 12:24 UTC
Re: [PATCH v1 12/12] hotplug/Linux: add iscsi block hotplug script
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 12/12] hotplug/Linux:
add iscsi block hotplug script"):> On 13/03/13 17:31, Ian Jackson wrote:
> > Would it be possible for the user to specify the file that the
> > password should be read from ?
>
> AFAIK no, this is read from a system defined file, that can be different
> depending on the distro used. Maybe be could expand this script latter
> if needed, in order to be able to detect where this file resides and use
> that instead of passing the user/password in the command line.
Right, fair enough.
> > Did you write this script from scratch ? (TBH I don''t intend
to
> > review it in detail...)
>
> I''ve found a previous version of iSCSI and Xen here:
>
> http://backdrift.org/xen-block-iscsi-script-with-multipath-support
>
> but I didn''t use it, so yes.
Right.
> > Why no set -e ?
>
> Because some of the executed commands might fail, but that''s
expected
> (and shouldn''t abort the execution) and we manually check for the
return
> value of executed commands.
I would normally turn on set -e, and then deal explicitly with the
commands that are expected to fail, with patterns like
set +e
command that might fail
rc=$?
set -e
or
command we don''t care about ||:
or
if command
then ...
That avoids accidentally missing out error checking.
Ian.
Roger Pau Monné
2013-Mar-15 12:29 UTC
Re: [PATCH v1 11/12] hotplug: document new hotplug interface
On 13/03/13 17:29, Ian Jackson wrote:> Roger Pau Monne writes ("[Xen-devel] [PATCH v1 11/12] hotplug: document new hotplug interface"): >> Mention the new diskspec parameter and add a document explaining the >> new hotplug interface. > ... >> +HOTPLUG_PATH >> +------------ >> + >> +Points to the xenstore directory that holds information relative >> +to this hotplug script. At present only one parameter is passed by >> +the toolstack, the "params" xenstore entry which contains the "target" >> +line specified in the diskspec xl disk configuration (pdev_path in >> +libxl_device_disk struct). >> + >> +This xenstore directory will be used to communicate between the >> +hotplug script and the toolstack, and it can also be used by the >> +hotplug script to store temporary information. > > Doesn''t this leave the possibility that a script might decide to write > "sponge"="42" in the xenstore directory, for its own purposes, and > that libxl might later decide to try to pass "sponge"="bath" for an > enhanced interface ?This hotplug scripts (v2) should only be called with the parameters and env vars specified in this document, so we should never call this scripts with anything different. If we ever decide to pass some random env var, we should bump hotplug version to 3 and redo this document, but guessing now which env vars we might need in the future is kind of a shot in the dark.> I think you need to specify which names each user is entitled to use.Reserved xenstore entries are also listed in the document, the user should not use the following entries randomly: HOTPLUG_PATH/version HOTPLUG_PATH/pdev> Also, you need to say something about access control.The hotplug script will have permissions to read/write on HOTPLUG_PATH and BACKEND_PATH at least, but of course if this runs on the Dom0 it will have access to everything (just as hotplug scripts have right now)> >> +======================>> +COMMAND LINE PARAMETERS >> +======================> ... >> +Script will be called with only one parameter, that is either prepare, >> +add, remove, unprepare, localattach or localdetach. > > You need to say what scripts should do with unknown parameters. > >> +BACKEND_PATH: not valid >> + >> +Expected output: >> +HOTPLGU_PATH/version = version supported by the hotplug script > > The output is written into xenstore ? (Also, typo.) > > thanks, > Ian. >
Roger Pau Monné
2013-Mar-15 12:39 UTC
Re: [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface
On 15/03/13 13:20, Ian Jackson wrote:> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 07/12] libxl: add prepare/unprepare operations to the libxl public interface"): >> On 13/03/13 17:19, Ian Jackson wrote: >>> I''m not sure I understand how the caller is supposed to use this. >> >> The caller should execute prepare before adding the disk (hopefully in a >> not performance sensitive path). > > That''s a non-backward-compatible API change, then ? We''re trying to > avoid those.It is backward compatible if the caller only uses v1 hotplug scripts. If the caller wishes to use v2, then it has to call prepare.> >>> How does the eventual script manage to correlate the preparation with >>> the unpreparation ? >> >> You call the script with the prepare or unprepare parameters, so the >> script knows which action it has to execute. > > No, I mean, is it just the target string that is relevant for > identifying the preparation ? Can the same disk be prepared multiple > times (eg for multiple guests) ? How is this synchronised ?Yes, since everything is stored in terms of <domid>/<devid>, the same "params" line can be used in multiple guests.> >> The only downfall of not calling prepare before add is that libxl will >> thread this device as using the v1 hotplug interface, so old users of >> libxl will still have the same functionality, but they will not be able >> to use new hotplug scripts using the v2 interface. > > Perhaps it would be better to hide this from the caller and implement > the old API in terms of the new one ?The advantage of this new hotplug calling convention is that you are allowed to call prepare and add at different points, so you can offload work done in the add phase to the prepare call. That being said, this public prepare function is only used for block-attach, so I''m not sure if we are interested in doing any kind of offloading there (since it''s not a critical path), so I might as well remove the public prepare/unprepare functions and do it all inside add.
Ian Jackson
2013-Apr-11 16:17 UTC
Re: [PATCH v1 08/12] libxl: add disk specific remove functions [and 1 more messages]
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 08/12] libxl: add disk
specific remove functions"):> On 13/03/13 17:22, Ian Jackson wrote:
> > This macro is very similar to DEFINE_DEVICE_REMOVE. The only
> > difference seems to be these extra lines:
> >
> >> + aodev->hotplug.version = type->hotplug_version;
\
> >> + LOG(DEBUG, "hotplug version: %d",
aodev->hotplug.version); \
> >> + libxl__initiate_device_remove(egc, aodev);
\
> >
> > Perhaps the right answer would be to add a new formal parameter to
> > DEFINE_DEVICE_REMOVE which allows DEFINE_DEVICE_REMOVE''s user
to
> > specify some extra code here ?
>
> type->hotplug_version is only available in libxl_device_disk, so even if
> using something like
>
> if (param)
> aodev->hotplug.version = type->hotplug_version;
I see I didn''t reply to this and your v2 has this patch unchanged. I
still think we need to find a way not to duplicate this code. What I
meant above was something like this:
#define DEFINE_DEVICE_REMOVE(type, removedestroy, f, extra) \
int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
uint32_t domid, libxl_device_##type *type, \
const libxl_asyncop_how *ao_how) \
{ \
AO_CREATE(ctx, domid, ao_how); \
libxl__device *device; \
libxl__ao_device *aodev; \
int rc; \
\
GCNEW(device); \
rc = libxl__device_from_##type(gc, domid, type, device); \
if (rc != 0) goto out; \
\
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
aodev->dev = device; \
aodev->callback = device_addrm_aocomplete; \
aodev->force = f; \
extra; \
libxl__initiate_device_remove(egc, aodev); \
\
out: \
if (rc) return AO_ABORT(rc); \
return AO_INPROGRESS; \
}
#define DEFINE_DISK_REMOVE(type, removedestroy, f) \
DEFINE_DEVICE_REMOVE(type, removedestroy, f, {
aodev->hotplug.version = type->hotplug_version;
LOG(DEBUG, "hotplug version: %d", aodev->hotplug.version);
})
Ian.
Roger Pau Monné
2013-Apr-15 07:33 UTC
Re: [PATCH v1 08/12] libxl: add disk specific remove functions [and 1 more messages]
On 11/04/13 18:17, Ian Jackson wrote:> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v1 08/12] libxl: add disk specific remove functions"): >> On 13/03/13 17:22, Ian Jackson wrote: >>> This macro is very similar to DEFINE_DEVICE_REMOVE. The only >>> difference seems to be these extra lines: >>> >>>> + aodev->hotplug.version = type->hotplug_version; \ >>>> + LOG(DEBUG, "hotplug version: %d", aodev->hotplug.version); \ >>>> + libxl__initiate_device_remove(egc, aodev); \ >>> >>> Perhaps the right answer would be to add a new formal parameter to >>> DEFINE_DEVICE_REMOVE which allows DEFINE_DEVICE_REMOVE''s user to >>> specify some extra code here ? >> >> type->hotplug_version is only available in libxl_device_disk, so even if >> using something like >> >> if (param) >> aodev->hotplug.version = type->hotplug_version; > > I see I didn''t reply to this and your v2 has this patch unchanged. I > still think we need to find a way not to duplicate this code. What I > meant above was something like this: > > #define DEFINE_DEVICE_REMOVE(type, removedestroy, f, extra) \ > int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \ > uint32_t domid, libxl_device_##type *type, \ > const libxl_asyncop_how *ao_how) \ > { \ > AO_CREATE(ctx, domid, ao_how); \ > libxl__device *device; \ > libxl__ao_device *aodev; \ > int rc; \ > \ > GCNEW(device); \ > rc = libxl__device_from_##type(gc, domid, type, device); \ > if (rc != 0) goto out; \ > \ > GCNEW(aodev); \ > libxl__prepare_ao_device(ao, aodev); \ > aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \ > aodev->dev = device; \ > aodev->callback = device_addrm_aocomplete; \ > aodev->force = f; \ > extra; \ > libxl__initiate_device_remove(egc, aodev); \ > \ > out: \ > if (rc) return AO_ABORT(rc); \ > return AO_INPROGRESS; \ > } > > #define DEFINE_DISK_REMOVE(type, removedestroy, f) \ > DEFINE_DEVICE_REMOVE(type, removedestroy, f, { > aodev->hotplug.version = type->hotplug_version; > LOG(DEBUG, "hotplug version: %d", aodev->hotplug.version); > })Done, I will wait for comments for v2 before reposting.