* [PATCH v3 00/15] libxl: synchronise domain configuration
@ 2014-09-04 22:43 Wei Liu
2014-09-04 22:43 ` [PATCH v3 01/15] libxl: make userdata_path libxl internal function Wei Liu
` (14 more replies)
0 siblings, 15 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Version 3 of this series based on master branch. This series can be pulled
from:
git://xenbits.xen.org/people/liuw/xen.git wip.config-sync3-b
Delta from v2 is listed in individual commit log.
This version has passed following tests:
test-amd64-amd64-xl
test-amd64-amd64-xl-qemuu-debianhvm-amd64
test-amd64-amd64-xl-qemut-debianhvm-amd64
I've also done some manual tests for things cannot be tested in OSSTest. If
you are interested in any specific test please let me know.
Tests I've done:
### Rename-restart test (test xl domain death handling path)
root@dt14:~# xl list
Name ID Mem VCPUs State Time(s)
Domain-0 0 511 8 r----- 4325.1
debian.guest.osstest-20140904T1759Z 49 512 1 ---s-- 6.4
debian.guest.osstest 50 512 0 --p--- 0.0
### xl config-update (test command compatability and domain death handling)
on_restart = "restart"
root@dt14:~# xl config-update debian.guest.osstest debian.guest.osstest.cfg
WARN: xl now has better capability to manage domain configuration, avoid using this command when possible
setting dom55 configuration
[ "xl" file appears in userdata store ]
[ reboot guest ]
[ "xl" file gone, transformed into "libxl-json" file ]
### xl list --long on Dom0 only system (test stub file generation)
[
{
"domid": 0,
"config": {
"c_info": {
"type": "pv",
"name": "Domain-0"
},
"b_info": {
"max_memkb": 524288,
"target_memkb": 523903,
"sched_params": {
},
"type.pv": {
}
}
}
}
]
### xl block-attach and block-detach for Dom0
root@dt14:~# xl block-attach Domain-0 'file:/root/standalone.test-amd64-amd64-xl-qemut-debianhvm-amd64.debianhvm-empty.iso,hdc:cdrom,r'
root@dt14:~# xl block-list Domain-0
Vdev BE handle state evt-ch ring-ref BE-path
5632 0 0 4 76 8 /local/domain/0/backend/qdisk/0/5632
root@dt14:~# xl block-detach Domain-0 hdcroot@dt14:~# xl block-list Domain-0
Vdev BE handle state evt-ch ring-ref BE-path
root@dt14:~#
### hotplug a device then migrate guest (test configuration preservation)
root@dt14:~# xl create debian.guest.osstest.cfg
root@dt14:~# xl network-list debian.guest.osstest
Idx BE Mac Addr. handle state evt-ch tx-/rx-ring-ref BE-path
0 0 5e:36:06:59:00:01 0 4 11 10/11 /local/domain/0/backend/vif/65/0
root@dt14:~# xl network-attach debian.guest.osstest 'bridge=xenbr0'
root@dt14:~# xl network-list debian.guest.osstest
Idx BE Mac Addr. handle state evt-ch tx-/rx-ring-ref BE-path
0 0 5e:36:06:59:00:01 0 4 11 10/11 /local/domain/0/backend/vif/65/0
1 0 00:16:3e:4b:9c:27 1 4 12 51/82 /local/domain/0/backend/vif/65/1
root@dt14:~# xl migrate debian.guest.osstest localhost
migration target: Ready to receive domain.
Saving to migration stream new xl format (info 0x1/0x0/1275)
Loading new save file <incoming migration stream> (new xl fmt info 0x1/0x0/1275)
Savefile contains xl domain config in JSON format
xc: Reloading memory pages: 131131/262144 50%migration target: Transfer complete, requesting permission to start domain.
migration sender: Target has acknowledged transfer.
migration sender: Giving target permission to start.
migration target: Got permission, starting domain.
migration target: Domain started successsfully.
migration sender: Target reports successful startup.
Migration successful.
root@dt14:~# xl network-list debian.guest.osstest
Idx BE Mac Addr. handle state evt-ch tx-/rx-ring-ref BE-path
0 0 5e:36:06:59:00:01 0 4 11 82/51 /local/domain/0/backend/vif/66/0
1 0 00:16:3e:4b:9c:27 1 4 12 11/10 /local/domain/0/backend/vif/66/1
Thanks
Wei.
Legend: A - acked
Wei Liu (15):
A libxl: make userdata_path libxl internal function
A libxl: functions to lock / unlock libxl userdata store
libxl: properly lock userdata store
A libxl: libxl-json format and internal functions to get / set it
A libxl: store a copy of configuration when creating domain
A libxl: introduce libxl__device_from_pcidev
libxl: disallow attaching the same device more than once
libxl: introduce helper to initialise Dom0
libxl: synchronise configuration when we hotplug a device
libxl: make libxl_cd_insert "eject" + "insert"
libxl: refactor libxl_get_memory_target
libxl: introduce libxl_retrieve_domain_configuration
libxl: introduce libxl_userdata_unlink
xl: use libxl_retrieve_domain_configuration and JSON format
A xl: long output of "list" command now contains Dom0 information
.gitignore | 1 +
docs/man/xl.pod.1 | 5 +
tools/hotplug/Linux/init.d/xencommons.in.in | 5 +-
tools/libxl/Makefile | 10 +-
tools/libxl/libxl.c | 526 +++++++++++++++++++++++++--
tools/libxl/libxl.h | 34 ++
tools/libxl/libxl_create.c | 22 ++
tools/libxl/libxl_device.c | 19 +
tools/libxl/libxl_dom.c | 112 +++++-
tools/libxl/libxl_internal.c | 146 ++++++++
tools/libxl/libxl_internal.h | 184 ++++++++++
tools/libxl/libxl_pci.c | 80 +++-
tools/libxl/libxl_types.idl | 3 +
tools/libxl/xen-init-dom0.c | 120 ++++++
tools/libxl/xl_cmdimpl.c | 127 ++++---
tools/libxl/xl_cmdtable.c | 4 +-
16 files changed, 1283 insertions(+), 115 deletions(-)
create mode 100644 tools/libxl/xen-init-dom0.c
--
1.7.10.4
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 01/15] libxl: make userdata_path libxl internal function
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 02/15] libxl: functions to lock / unlock libxl userdata store Wei Liu
` (13 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Later patch will make use of it to generate file path and name.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 14 +++++++-------
tools/libxl/libxl_internal.h | 3 +++
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c944804..5e0cb80 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1807,9 +1807,9 @@ char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid)
return GCSPRINTF(LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
}
-static const char *userdata_path(libxl__gc *gc, uint32_t domid,
- const char *userdata_userid,
- const char *wh)
+const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
+ const char *userdata_userid,
+ const char *wh)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
char *uuid_string;
@@ -1844,7 +1844,7 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid)
glob_t gl;
int r, i;
- pattern = userdata_path(gc, domid, "*", "?");
+ pattern = libxl__userdata_path(gc, domid, "*", "?");
if (!pattern)
goto out;
@@ -1875,7 +1875,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
int e, rc;
int fd = -1;
- filename = userdata_path(gc, domid, userdata_userid, "d");
+ filename = libxl__userdata_path(gc, domid, userdata_userid, "d");
if (!filename) {
rc = ERROR_NOMEM;
goto out;
@@ -1886,7 +1886,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
goto out;
}
- newfilename = userdata_path(gc, domid, userdata_userid, "n");
+ newfilename = libxl__userdata_path(gc, domid, userdata_userid, "n");
if (!newfilename) {
rc = ERROR_NOMEM;
goto out;
@@ -1937,7 +1937,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
int datalen = 0;
void *data = 0;
- filename = userdata_path(gc, domid, userdata_userid, "d");
+ filename = libxl__userdata_path(gc, domid, userdata_userid, "d");
if (!filename) {
rc = ERROR_NOMEM;
goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 04c9378..7244473 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -992,6 +992,9 @@ _hidden int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
uint32_t size, void *data);
_hidden int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid);
+_hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
+ const char *userdata_userid,
+ const char *wh);
_hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);
_hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 02/15] libxl: functions to lock / unlock libxl userdata store
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
2014-09-04 22:43 ` [PATCH v3 01/15] libxl: make userdata_path libxl internal function Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 03/15] libxl: properly lock " Wei Liu
` (12 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
This lock is used to protect all userdata files related to a particular
domain, which include but are not limited to domain configuration. A
new "domain-userdata-lock" entry is introduced in libxl registry.
This lock works among different processes and different threads within
the same process.
Locking protocol inspired by Ian Jackson's chiark-utils with-lock-ex. A
file lock is taken with flock(2). If that succeeds that thread fstat the
fd and stat the lock file path. If the device and inode match then the
lock has been successfully acquired. This lock remains acquired until
the lock file gets deleted or released by flock(2). If device and inode
don't match then another thread acquired the lock and deleted the file
in the meantime; lock procedure should restart.
Portability note: this lock utilises flock(2) so a proper implementation
of flock(2) is required -- that is, it should not be implemented with
fcntl(2).
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
change in v3:
rename functions to libxl__{un,}lock_domain_userdata
rename registry entry to "domain-userdata-lock"
---
tools/libxl/libxl.h | 3 ++
tools/libxl/libxl_internal.c | 69 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 5 +++
3 files changed, 77 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 460207b..1ca25ae 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1220,6 +1220,9 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
* "xl" domain config file in xl format, Unix line endings
* "libvirt-xml" domain config file in libvirt XML format. See
* http://libvirt.org/formatdomain.html
+ * "domain-userdata-lock" lock file to protect domain userdata in libxl.
+ * It's a per-domain lock. Applications should
+ * not touch this file.
*
* libxl does not enforce the registration of userdata userids or the
* semantics of the data. For specifications of the data formats
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b880c89..edf864b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -381,6 +381,75 @@ out:
return rc;
}
+/* Portability note: this lock utilises flock(2) so a proper implementation of
+ * flock(2) is required.
+ */
+libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
+{
+ libxl__carefd *carefd = NULL;
+ const char *lockfile;
+ int fd;
+ struct stat stab, fstab;
+
+ lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
+ if (!lockfile) goto out;
+
+ while (true) {
+ libxl__carefd_begin();
+ fd = open(lockfile, O_RDWR|O_CREAT, 0666);
+ if (fd < 0)
+ LOGE(ERROR, "cannot open lockfile %s, errno=%d", lockfile, errno);
+ carefd = libxl__carefd_opened(CTX, fd);
+ if (fd < 0) goto out;
+
+ /* Lock the file in exclusive mode, wait indefinitely to
+ * acquire the lock
+ */
+ while (flock(fd, LOCK_EX)) {
+ switch (errno) {
+ case EINTR:
+ /* Signal received, retry */
+ continue;
+ default:
+ /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
+ LOGE(ERROR,
+ "unexpected error while trying to lock %s, fd=%d, errno=%d",
+ lockfile, fd, errno);
+ goto out;
+ }
+ }
+
+ if (fstat(fd, &fstab)) {
+ LOGE(ERROR, "cannot fstat %s, fd=%d, errno=%d",
+ lockfile, fd, errno);
+ goto out;
+ }
+ if (stat(lockfile, &stab)) {
+ if (errno != ENOENT) {
+ LOGE(ERROR, "cannot stat %s, errno=%d", lockfile, errno);
+ goto out;
+ }
+ } else {
+ if (stab.st_dev == fstab.st_dev && stab.st_ino == fstab.st_ino)
+ break;
+ }
+
+ libxl__carefd_close(carefd);
+ }
+
+ return carefd;
+
+out:
+ if (carefd) libxl__carefd_close(carefd);
+ return NULL;
+}
+
+void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd)
+{
+ /* Simply closing the file descriptor releases the lock */
+ libxl__carefd_close(lock_carefd);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7244473..0cedd30 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -43,6 +43,7 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/socket.h>
+#include <sys/file.h>
#include <xenstore.h>
#include <xenctrl.h>
@@ -3223,6 +3224,10 @@ static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
+/* Portability note: a proper flock(2) implementation is required */
+libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid);
+void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd);
+
#endif
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 03/15] libxl: properly lock userdata store
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
2014-09-04 22:43 ` [PATCH v3 01/15] libxl: make userdata_path libxl internal function Wei Liu
2014-09-04 22:43 ` [PATCH v3 02/15] libxl: functions to lock / unlock libxl userdata store Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 10:52 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 04/15] libxl: libxl-json format and internal functions to get / set it Wei Liu
` (11 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Originally libxl user data store didn't have lock at all. There could be
such race condition as mentioned by Ian Jackson:
Task 1 Task 2
Creating the domain Trying to shut down
actually create domain
observe domid
start domain destruction
delete all userdata
destroy domain
store the userdata
*** forbidden state created: userdata exists but domain doesn't
*** userdata has been leaked
[ would now bomb out ]
This patch adds in proper locking to libxl user data store. The lock is
associated with a specific domain (i.e. a per-domain lock).
As for locking hierachy, we first take CTX lock (which is implemented
with pthread recursive mutex so even if the application has taken it
we're fine), then take the file lock. These locks are released in
reversed order.
A new libxl error code ERROR_LOCK_FAIL is introduced to describe failure
to acquire locks.
Also factor out libxl__userdata_{retrieve,store}, so that other
functions that already hold the lock can call them to manipulate
user data.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
changed in v3:
move the lock out of libxl__userdata_destroyall. The critical section
should be
lock
delete all userdata
destroy domain
unlcok
---
tools/libxl/libxl.c | 8 +++++
tools/libxl/libxl_dom.c | 73 +++++++++++++++++++++++++++++++++++-------
tools/libxl/libxl_internal.h | 10 ++++++
tools/libxl/libxl_types.idl | 1 +
4 files changed, 80 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2ae5fca..9bb1a90 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1530,6 +1530,7 @@ static void devices_destroy_cb(libxl__egc *egc,
uint32_t domid = dis->domid;
char *dom_path;
char *vm_path;
+ libxl__carefd *lock = NULL;
dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path) {
@@ -1555,6 +1556,12 @@ static void devices_destroy_cb(libxl__egc *egc,
xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc,
"/local/domain/%d/hvmloader", domid));
+ /* This is async operation, we already hold CTX lock */
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
libxl__userdata_destroyall(gc, domid);
rc = xc_domain_destroy(ctx->xch, domid);
@@ -1566,6 +1573,7 @@ static void devices_destroy_cb(libxl__egc *egc,
rc = 0;
out:
+ if (lock) libxl__unlock_domain_userdata(lock);
dis->callback(egc, dis, rc);
return;
}
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 5e0cb80..0dfdb08 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1865,11 +1865,10 @@ out:
return;
}
-int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
- const char *userdata_userid,
- const uint8_t *data, int datalen)
+int libxl__userdata_store(libxl__gc *gc, uint32_t domid,
+ const char *userdata_userid,
+ const uint8_t *data, int datalen)
{
- GC_INIT(ctx);
const char *filename;
const char *newfilename;
int e, rc;
@@ -1898,7 +1897,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
if (fd < 0)
goto err;
- if (libxl_write_exactly(ctx, fd, data, datalen, "userdata", newfilename))
+ if (libxl_write_exactly(CTX, fd, data, datalen, "userdata", newfilename))
goto err;
if (close(fd) < 0) {
@@ -1920,18 +1919,42 @@ err:
}
if (rc)
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write/rename %s for %s",
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR, "cannot write/rename %s for %s",
newfilename, filename);
out:
- GC_FREE;
return rc;
}
-int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
- const char *userdata_userid,
- uint8_t **data_r, int *datalen_r)
+int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
+ const char *userdata_userid,
+ const uint8_t *data, int datalen)
{
GC_INIT(ctx);
+ int rc;
+ libxl__carefd *lock;
+
+ CTX_LOCK;
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__userdata_store(gc, domid, userdata_userid,
+ data, datalen);
+
+ libxl__unlock_domain_userdata(lock);
+
+out:
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
+int libxl__userdata_retrieve(libxl__gc *gc, uint32_t domid,
+ const char *userdata_userid,
+ uint8_t **data_r, int *datalen_r)
+{
const char *filename;
int e, rc;
int datalen = 0;
@@ -1943,13 +1966,13 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
goto out;
}
- e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen);
+ e = libxl_read_file_contents(CTX, filename, data_r ? &data : 0, &datalen);
if (e && errno != ENOENT) {
rc = ERROR_FAIL;
goto out;
}
if (!e && !datalen) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "userdata file %s is empty", filename);
+ LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "userdata file %s is empty", filename);
if (data_r) assert(!*data_r);
rc = ERROR_FAIL;
goto out;
@@ -1958,7 +1981,33 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
if (data_r) *data_r = data;
if (datalen_r) *datalen_r = datalen;
rc = 0;
+
+out:
+ return rc;
+}
+
+int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
+ const char *userdata_userid,
+ uint8_t **data_r, int *datalen_r)
+{
+ GC_INIT(ctx);
+ int rc;
+ libxl__carefd *lock;
+
+ CTX_LOCK;
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__userdata_retrieve(gc, domid, userdata_userid,
+ data_r, datalen_r);
+
+
+ libxl__unlock_domain_userdata(lock);
out:
+ CTX_UNLOCK;
GC_FREE;
return rc;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0cedd30..bc89e79 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -997,6 +997,16 @@ _hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
const char *userdata_userid,
const char *wh);
_hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);
+/* Caller must hold userdata store lock before calling
+ * libxl__userdata_{retrieve,store}
+ * See libxl__{un,}lock_domain_userdata.
+ */
+_hidden int libxl__userdata_retrieve(libxl__gc *gc, uint32_t domid,
+ const char *userdata_userid,
+ uint8_t **data_r, int *datalen_r);
+_hidden int libxl__userdata_store(libxl__gc *gc, uint32_t domid,
+ const char *userdata_userid,
+ const uint8_t *data, int datalen);
_hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid,
int suspend_cancel);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 931c9e9..95681d5 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -58,6 +58,7 @@ libxl_error = Enumeration("error", [
(-12, "OSEVENT_REG_FAIL"),
(-13, "BUFFERFULL"),
(-14, "UNKNOWN_CHILD"),
+ (-15, "LOCK_FAIL"),
], value_namespace = "")
libxl_domain_type = Enumeration("domain_type", [
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 04/15] libxl: libxl-json format and internal functions to get / set it
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (2 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 03/15] libxl: properly lock " Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 05/15] libxl: store a copy of configuration when creating domain Wei Liu
` (10 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Introduce a new format in libxl userdata store called "libxl-json". This
file format contains JSON version of libxl_domain_config, generated by
libxl. Applications are not supposed to access this file directly.
Two internal functions to get and set libxl_domain_configuration
are also introduced. Also introduce a new error code to indicate
abnormal state that libxl-json config file is empty.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
change in v3:
Update registry entry description to cover what lock is used
---
tools/libxl/libxl.h | 5 ++++
tools/libxl/libxl_internal.c | 56 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 11 +++++++++
tools/libxl/libxl_types.idl | 1 +
4 files changed, 73 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1ca25ae..dab3a67 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1223,6 +1223,11 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
* "domain-userdata-lock" lock file to protect domain userdata in libxl.
* It's a per-domain lock. Applications should
* not touch this file.
+ * "libxl-json" libxl_domain_config object in JSON format, generated
+ * by libxl. Applications should not access this file
+ * directly. This file is protected by domain-userdata-lock
+ * for against Read-Modify-Write operation and domain
+ * destruction.
*
* libxl does not enforce the registration of userdata userids or the
* semantics of the data. For specifications of the data formats
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index edf864b..8ac1c14 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -450,6 +450,62 @@ void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd)
libxl__carefd_close(lock_carefd);
}
+int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
+ libxl_domain_config *d_config)
+{
+ uint8_t *data = NULL;
+ int rc, len;
+
+ rc = libxl__userdata_retrieve(gc, domid, "libxl-json", &data, &len);
+ if (rc) {
+ LOGEV(ERROR, rc,
+ "failed to retrieve domain configuration for domain %d", domid);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (len == 0) {
+ /* No logging, not necessary an error from caller's PoV. */
+ rc = ERROR_JSON_CONFIG_EMPTY;
+ goto out;
+ }
+ rc = libxl_domain_config_from_json(CTX, d_config, (const char *)data);
+
+out:
+ free(data);
+ return rc;
+}
+
+int libxl__set_domain_configuration(libxl__gc *gc, uint32_t domid,
+ libxl_domain_config *d_config)
+{
+ char *d_config_json;
+ int rc;
+
+ d_config_json = libxl_domain_config_to_json(CTX, d_config);
+ if (!d_config_json) {
+ LOGE(ERROR,
+ "failed to convert domain configuration to JSON for domain %d",
+ domid);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__userdata_store(gc, domid, "libxl-json",
+ (const uint8_t *)d_config_json,
+ strlen(d_config_json) + 1 /* include '\0' */);
+ if (rc) {
+ LOGEV(ERROR, rc, "failed to store domain configuration for domain %d",
+ domid);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+out:
+ free(d_config_json);
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index bc89e79..22c564f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3238,6 +3238,17 @@ int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid);
void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd);
+/*
+ * Retrieve / store domain configuration from / to libxl private
+ * data store. The registry entry in libxl private data store
+ * is "libxl-json".
+ * Caller must hold user data lock.
+ */
+int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
+ libxl_domain_config *d_config);
+int libxl__set_domain_configuration(libxl__gc *gc, uint32_t domid,
+ libxl_domain_config *d_config);
+
#endif
/*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 95681d5..1cd635e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -59,6 +59,7 @@ libxl_error = Enumeration("error", [
(-13, "BUFFERFULL"),
(-14, "UNKNOWN_CHILD"),
(-15, "LOCK_FAIL"),
+ (-16, "JSON_CONFIG_EMPTY"),
], value_namespace = "")
libxl_domain_type = Enumeration("domain_type", [
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 05/15] libxl: store a copy of configuration when creating domain
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (3 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 04/15] libxl: libxl-json format and internal functions to get / set it Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 06/15] libxl: introduce libxl__device_from_pcidev Wei Liu
` (9 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
The configuration is stored in libxl-json format. It will be used as
template to reconstruct domain configuration during runtime.
There's only one write to disk when domain creation finishes. We
therefore have a window that the domain exists but has no JSON config in
disk. We define this state as domain being created or destroyed. Any
other operations that need to access JSON config should bail.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_create.c | 22 ++++++++++++++++++++++
tools/libxl/libxl_internal.c | 21 +++++++++++++++++++++
tools/libxl/libxl_internal.h | 21 +++++++++++++++++++++
3 files changed, 64 insertions(+)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ee328e9..57a0ece 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1389,10 +1389,30 @@ static void domcreate_complete(libxl__egc *egc,
{
STATE_AO_GC(dcs->ao);
libxl_domain_config *const d_config = dcs->guest_config;
+ libxl_domain_config *d_config_saved = &dcs->guest_config_saved;
if (!rc && d_config->b_info.exec_ssidref)
rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, d_config->b_info.exec_ssidref);
+ if (!rc) {
+ libxl__carefd *lock;
+
+ /* Note that we hold CTX lock at this point so only need to
+ * take data store lock
+ */
+ lock = libxl__lock_domain_userdata(gc, dcs->guest_domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ } else {
+ libxl__update_domain_configuration(gc, d_config_saved, d_config);
+ rc = libxl__set_domain_configuration(gc, dcs->guest_domid,
+ d_config_saved);
+ libxl__unlock_domain_userdata(lock);
+ }
+ }
+
+ libxl_domain_config_dispose(d_config_saved);
+
if (rc) {
if (dcs->guest_domid) {
dcs->dds.ao = ao;
@@ -1443,6 +1463,8 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
GCNEW(cdcs);
cdcs->dcs.ao = ao;
cdcs->dcs.guest_config = d_config;
+ libxl_domain_config_init(&cdcs->dcs.guest_config_saved);
+ libxl_domain_config_copy(ctx, &cdcs->dcs.guest_config_saved, d_config);
cdcs->dcs.restore_fd = restore_fd;
cdcs->dcs.callback = domain_create_cb;
cdcs->dcs.checkpointed_stream = checkpointed_stream;
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 8ac1c14..e9747f1 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -506,6 +506,27 @@ out:
return rc;
}
+void libxl__update_domain_configuration(libxl__gc *gc,
+ libxl_domain_config *dst,
+ const libxl_domain_config *src)
+{
+ int i;
+
+ /* update network interface information */
+ for (i = 0; i < src->num_nics; i++)
+ libxl__update_config_nic(gc, &dst->nics[i], &src->nics[i]);
+
+ /* update vtpm information */
+ for (i = 0; i < src->num_vtpms; i++)
+ libxl__update_config_vtpm(gc, &dst->vtpms[i], &src->vtpms[i]);
+
+ /* update guest UUID */
+ libxl_uuid_copy(CTX, &dst->c_info.uuid, &src->c_info.uuid);
+
+ /* video ram */
+ dst->b_info.video_memkb = src->b_info.video_memkb;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 22c564f..3b8f74e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2781,6 +2781,7 @@ struct libxl__domain_create_state {
/* filled in by user */
libxl__ao *ao;
libxl_domain_config *guest_config;
+ libxl_domain_config guest_config_saved; /* vanilla config */
int restore_fd;
libxl__domain_create_cb *callback;
libxl_asyncprogress_how aop_console_how;
@@ -3249,6 +3250,26 @@ int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
int libxl__set_domain_configuration(libxl__gc *gc, uint32_t domid,
libxl_domain_config *d_config);
+/* ------ Things related to updating domain configurations ----- */
+void libxl__update_domain_configuration(libxl__gc *gc,
+ libxl_domain_config *dst,
+ const libxl_domain_config *src);
+static inline void libxl__update_config_nic(libxl__gc *gc,
+ libxl_device_nic *dst,
+ libxl_device_nic *src)
+{
+ dst->devid = src->devid;
+ libxl_mac_copy(CTX, &dst->mac, &src->mac);
+}
+
+static inline void libxl__update_config_vtpm(libxl__gc *gc,
+ libxl_device_vtpm *dst,
+ libxl_device_vtpm *src)
+{
+ dst->devid = src->devid;
+ libxl_uuid_copy(CTX, &dst->uuid, &src->uuid);
+}
+
#endif
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 06/15] libxl: introduce libxl__device_from_pcidev
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (4 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 05/15] libxl: store a copy of configuration when creating domain Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 07/15] libxl: disallow attaching the same device more than once Wei Liu
` (8 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_pci.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 2782d0e..0500cf3 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -64,6 +64,20 @@ static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, in
flexarray_append_pair(back, libxl__sprintf(gc, "state-%d", num), libxl__sprintf(gc, "%d", 1));
}
+static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
+ libxl_device_pci *pcidev,
+ libxl__device *device)
+{
+ device->backend_devid = 0;
+ device->backend_domid = 0;
+ device->backend_kind = LIBXL__DEVICE_KIND_PCI;
+ device->devid = 0;
+ device->domid = domid;
+ device->kind = LIBXL__DEVICE_KIND_PCI;
+
+ return 0;
+}
+
int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int num)
{
@@ -81,12 +95,7 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Creating pci backend");
/* add pci device */
- device.backend_devid = 0;
- device.backend_domid = 0;
- device.backend_kind = LIBXL__DEVICE_KIND_PCI;
- device.devid = 0;
- device.domid = domid;
- device.kind = LIBXL__DEVICE_KIND_PCI;
+ libxl__device_from_pcidev(gc, domid, pcidev, &device);
flexarray_append_pair(back, "frontend-id", libxl__sprintf(gc, "%d", domid));
flexarray_append_pair(back, "online", "1");
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 07/15] libxl: disallow attaching the same device more than once
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (5 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 06/15] libxl: introduce libxl__device_from_pcidev Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 10:56 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 08/15] libxl: introduce helper to initialise Dom0 Wei Liu
` (7 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Originally the code allowed users to attach the same device more than
once. It just stupidly overwrites xenstore entries. This is bogus as
frontend will be very confused.
Introduce a helper function to check if the device to be written to
xenstore already exists. A new error code is also introduced.
The check and add are within one xs transaction.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
place check and add within one xs transaction
---
tools/libxl/libxl.c | 70 +++++++++++++++++++++++++++++++++++++-----
tools/libxl/libxl_device.c | 19 ++++++++++++
tools/libxl/libxl_internal.h | 3 ++
tools/libxl/libxl_pci.c | 35 ++++++++++++++++-----
tools/libxl/libxl_types.idl | 1 +
5 files changed, 112 insertions(+), 16 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9bb1a90..ad3495a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1893,6 +1893,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
flexarray_t *back;
libxl__device *device;
int rc;
+ xs_transaction_t t = XBT_NULL;
rc = libxl__device_vtpm_setdefault(gc, vtpm);
if (rc) goto out;
@@ -1932,10 +1933,30 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
flexarray_append(front, "handle");
flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
- libxl__device_generic_add(gc, XBT_NULL, device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count),
- NULL);
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ rc = libxl__device_exists(gc, t, device);
+ if (rc < 0) goto out;
+ if (rc == 1) { /* already exists in xenstore */
+ LOG(ERROR, "device already exists in xenstore");
+ aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
+ }
+
+ libxl__device_generic_add(gc, t, device,
+ libxl__xs_kvs_of_flexarray(gc, back,
+ back->count),
+ libxl__xs_kvs_of_flexarray(gc, front,
+ front->count),
+ NULL);
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto out;
+ }
aodev->dev = device;
aodev->action = LIBXL__DEVICE_ACTION_ADD;
@@ -1943,6 +1964,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
+ libxl__xs_transaction_abort(gc, &t);
aodev->rc = rc;
if(rc) aodev->callback(egc, aodev);
return;
@@ -2209,6 +2231,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
goto out;
}
+ rc = libxl__device_exists(gc, t, device);
+ if (rc < 0) goto out;
+ if (rc == 1) { /* already exists in xenstore */
+ LOG(ERROR, "device already exists in xenstore");
+ aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
+ }
+
switch (disk->backend) {
case LIBXL_DISK_BACKEND_PHY:
dev = disk->pdev_path;
@@ -2998,6 +3029,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
flexarray_t *back;
libxl__device *device;
int rc;
+ xs_transaction_t t = XBT_NULL;
rc = libxl__device_nic_setdefault(gc, nic, domid);
if (rc) goto out;
@@ -3068,10 +3100,31 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
flexarray_append(front, "mac");
flexarray_append(front, libxl__sprintf(gc,
LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
- libxl__device_generic_add(gc, XBT_NULL, device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count),
- NULL);
+
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ rc = libxl__device_exists(gc, t, device);
+ if (rc < 0) goto out;
+ if (rc == 1) { /* already exists in xenstore */
+ LOG(ERROR, "device already exists in xenstore");
+ aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
+ }
+
+ libxl__device_generic_add(gc, t, device,
+ libxl__xs_kvs_of_flexarray(gc, back,
+ back->count),
+ libxl__xs_kvs_of_flexarray(gc, front,
+ front->count),
+ NULL);
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto out;
+ }
aodev->dev = device;
aodev->action = LIBXL__DEVICE_ACTION_ADD;
@@ -3079,6 +3132,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
+ libxl__xs_transaction_abort(gc, &t);
aodev->rc = rc;
if (rc) aodev->callback(egc, aodev);
return;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index f8a2e1b..045212f 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -40,6 +40,25 @@ char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device)
device->domid, device->devid);
}
+/* Returns 1 if device exists, 0 if not, ERROR_* (<0) on error. */
+int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
+ libxl__device *device)
+{
+ int rc;
+ char *be_path = libxl__device_backend_path(gc, device);
+ const char *dir;
+
+ be_path = libxl__device_backend_path(gc, device);
+ rc = libxl__xs_read_checked(gc, t, be_path, &dir);
+
+ if (rc)
+ return rc;
+
+ if (dir)
+ return 1;
+ return 0;
+}
+
int libxl__parse_backend_path(libxl__gc *gc,
const char *path,
libxl__device *dev)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3b8f74e..fafef5a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1033,6 +1033,9 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
libxl__device_console *console,
libxl__domain_build_state *state);
+/* Returns 1 if device exists, 0 if not, ERROR_* (<0) on error. */
+_hidden int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
+ libxl__device *device);
_hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
libxl__device *device, char **bents, char **fents, char **ro_fents);
_hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 0500cf3..38a9642 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -124,6 +124,8 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
char *num_devs, *be_path;
int num = 0;
xs_transaction_t t;
+ libxl__device *device;
+ int rc;
be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0", libxl__xs_get_dompath(gc, 0), domid);
num_devs = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/num_devs", be_path));
@@ -148,15 +150,32 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
if (!starting)
flexarray_append_pair(back, "state", libxl__sprintf(gc, "%d", 7));
-retry_transaction:
- t = xs_transaction_start(ctx->xsh);
- libxl__xs_writev(gc, t, be_path,
- libxl__xs_kvs_of_flexarray(gc, back, back->count));
- if (!xs_transaction_end(ctx->xsh, t, 0))
- if (errno == EAGAIN)
- goto retry_transaction;
+ GCNEW(device);
+ libxl__device_from_pcidev(gc, domid, pcidev, device);
- return 0;
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ rc = libxl__device_exists(gc, t, device);
+ if (rc < 0) goto out;
+ if (rc == 1) {
+ LOG(ERROR, "device already exists in xenstore");
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
+ }
+
+ libxl__xs_writev(gc, t, be_path,
+ libxl__xs_kvs_of_flexarray(gc, back, back->count));
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto out;
+ }
+
+out:
+ libxl__xs_transaction_abort(gc, &t);
+ return rc;
}
static int libxl__device_pci_remove_xenstore(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 1cd635e..f1fcbc3 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -60,6 +60,7 @@ libxl_error = Enumeration("error", [
(-14, "UNKNOWN_CHILD"),
(-15, "LOCK_FAIL"),
(-16, "JSON_CONFIG_EMPTY"),
+ (-17, "DEVICE_EXISTS"),
], value_namespace = "")
libxl_domain_type = Enumeration("domain_type", [
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 08/15] libxl: introduce helper to initialise Dom0
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (6 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 07/15] libxl: disallow attaching the same device more than once Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-05 13:22 ` Wei Liu
2014-09-09 11:16 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device Wei Liu
` (6 subsequent siblings)
14 siblings, 2 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
This small helper is responsible for generating Dom0 JSON config
stub and writing Dom0 xenstore entries. This helpers subsumes two calls
to xenstore-write in xencommons script.
Dom0 UUID is intentionally left untouched, so it is always all
zeros. This makes sure that we don't leak Dom0 stubs across rebooting.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
move to libxl directory
---
.gitignore | 1 +
tools/hotplug/Linux/init.d/xencommons.in.in | 5 +-
tools/libxl/Makefile | 10 ++-
tools/libxl/xen-init-dom0.c | 120 +++++++++++++++++++++++++++
4 files changed, 132 insertions(+), 4 deletions(-)
create mode 100644 tools/libxl/xen-init-dom0.c
diff --git a/.gitignore b/.gitignore
index 6d725aa..8e34b85 100644
--- a/.gitignore
+++ b/.gitignore
@@ -313,6 +313,7 @@ tools/libxl/testidl.c
tools/libxl/*.pyc
tools/libxl/libxl-save-helper
tools/libxl/test_timedereg
+tools/libxl/xen-init-dom0
tools/blktap2/control/tap-ctl
tools/firmware/etherboot/eb-roms.h
tools/firmware/etherboot/gpxe-git-snapshot.tar.gz
diff --git a/tools/hotplug/Linux/init.d/xencommons.in.in b/tools/hotplug/Linux/init.d/xencommons.in.in
index b311bb8..1d860d9 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in.in
@@ -90,9 +90,8 @@ do_start () {
exit 1
fi
- echo Setting domain 0 name and domid...
- ${BINDIR}/xenstore-write "/local/domain/0/name" "Domain-0"
- ${BINDIR}/xenstore-write "/local/domain/0/domid" 0
+ echo Setting domain 0 name, domid and JSON config...
+ ${PRIVATE_BINDIR}/xen-init-dom0
fi
echo Starting xenconsoled...
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index bd0db3b..4bee4af 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -110,7 +110,7 @@ LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
$(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
-CLIENTS = xl testidl libxl-save-helper
+CLIENTS = xl testidl libxl-save-helper xen-init-dom0
CFLAGS_XL += $(CFLAGS_libxenlight)
CFLAGS_XL += -Wshadow
@@ -121,6 +121,10 @@ $(XL_OBJS) $(TEST_PROG_OBJS) _libxl.api-for-check: \
$(XL_OBJS): CFLAGS += $(CFLAGS_XL)
$(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h needs it.
+XEN_INIT_DOM0_OBJS = xen-init-dom0.o
+$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
+$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+
SAVE_HELPER_OBJS = libxl_save_helper.o _libxl_save_msgs_helper.o
$(SAVE_HELPER_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
@@ -222,6 +226,9 @@ libxlutil.a: $(LIBXLU_OBJS)
xl: $(XL_OBJS) libxlutil.so libxenlight.so
$(CC) $(LDFLAGS) -o $@ $(XL_OBJS) libxlutil.so $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
+xen-init-dom0: $(XEN_INIT_DOM0_OBJS) libxenlight.so
+ $(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
test_%: test_%.o test_common.o libxlutil.so libxenlight_test.so
$(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so, $(LDLIBS_libxenlight)) $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
@@ -239,6 +246,7 @@ install: all
$(INSTALL_DIR) $(DESTDIR)$(BASH_COMPLETION_DIR)
$(INSTALL_DIR) $(DESTDIR)$(PRIVATE_BINDIR)
$(INSTALL_PROG) xl $(DESTDIR)$(SBINDIR)
+ $(INSTALL_PROG) xen-init-dom0 $(DESTDIR)$(PRIVATE_BINDIR)
$(INSTALL_PROG) libxl-save-helper $(DESTDIR)$(PRIVATE_BINDIR)
$(INSTALL_SHLIB) libxenlight.so.$(MAJOR).$(MINOR) $(DESTDIR)$(LIBDIR)
$(SYMLINK_SHLIB) libxenlight.so.$(MAJOR).$(MINOR) $(DESTDIR)$(LIBDIR)/libxenlight.so.$(MAJOR)
diff --git a/tools/libxl/xen-init-dom0.c b/tools/libxl/xen-init-dom0.c
new file mode 100644
index 0000000..7925d64
--- /dev/null
+++ b/tools/libxl/xen-init-dom0.c
@@ -0,0 +1,120 @@
+#include <err.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <stdio.h>
+
+#include <xenctrl.h>
+#include <xenstore.h>
+#include <libxl.h>
+
+#define DOMNAME_PATH "/local/domain/0/name"
+#define DOMID_PATH "/local/domain/0/domid"
+
+static libxl_ctx *ctx;
+static xentoollog_logger_stdiostream *logger;
+static struct xs_handle *xsh;
+
+static void ctx_alloc(void)
+{
+ if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
+ (xentoollog_logger *)logger)) {
+ fprintf(stderr, "cannot init libxl context\n");
+ exit(1);
+ }
+ xsh = xs_open(0);
+ if (!xsh) {
+ fprintf(stderr, "cannot open xenstore connection\n");
+ exit(1);
+ }
+}
+
+static void ctx_free(void)
+{
+ if (ctx) {
+ libxl_ctx_free(ctx);
+ ctx = NULL;
+ }
+ if (logger) {
+ xtl_logger_destroy((xentoollog_logger *)logger);
+ logger = NULL;
+ }
+ if (xsh) {
+ xs_close(xsh);
+ xsh = NULL;
+ }
+}
+
+int main(int argc, char **argv)
+{
+ int rc;
+ libxl_domain_config dom0_config;
+ char *domname_string = NULL, *domid_string = NULL;
+ char *json = NULL;;
+
+ logger = xtl_createlogger_stdiostream(stderr, XTL_ERROR, 0);
+ if (!logger) exit(1);
+
+ atexit(ctx_free);
+
+ ctx_alloc();
+
+ libxl_domain_config_init(&dom0_config);
+
+ /* Sanity check: this program can only be run once. */
+ domid_string = xs_read(xsh, XBT_NULL, DOMID_PATH, NULL);
+ domname_string = xs_read(xsh, XBT_NULL, DOMNAME_PATH, NULL);
+ if (domid_string && domname_string) {
+ fprintf(stderr, "Dom0 is already set up\n");
+ rc = 0;
+ goto out;
+ }
+
+ /* Generate stub JSON config. */
+ dom0_config.c_info.type = LIBXL_DOMAIN_TYPE_PV;
+ libxl_domain_build_info_init_type(&dom0_config.b_info,
+ LIBXL_DOMAIN_TYPE_PV);
+
+ json = libxl_domain_config_to_json(ctx, &dom0_config);
+ /* libxl-json format requires the string ends with '\0'. Code
+ * snippet taken from libxl.
+ */
+ rc = libxl_userdata_store(ctx, 0, "libxl-json",
+ (const uint8_t *)json,
+ strlen(json) + 1 /* include '\0' */);
+ if (rc) {
+ fprintf(stderr, "cannot store stub json config for Dom0\n");
+ goto out;
+ }
+
+ /* Write xenstore entries. */
+ if (!xs_write(xsh, XBT_NULL, DOMID_PATH, "0", strlen("0"))) {
+ fprintf(stderr, "cannot set domid for Dom0\n");
+ rc = 1;
+ goto out;
+ }
+
+ if (!xs_write(xsh, XBT_NULL, DOMNAME_PATH, "Domain-0",
+ strlen("Domain-0"))) {
+ fprintf(stderr, "cannot set domain name for Dom0\n");
+ rc = 1;
+ goto out;
+ }
+
+ printf("Done setting up Dom0\n");
+
+out:
+ libxl_domain_config_dispose(&dom0_config);
+ free(domid_string);
+ free(domname_string);
+ free(json);
+ return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (7 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 08/15] libxl: introduce helper to initialise Dom0 Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 11:11 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
` (5 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
We update JSON version first, then write to xenstore, so that we
maintain the following invariant: any device which is present in
xenstore has a corresponding entry in JSON.
The workflow is as followed:
lock json config
read json config
update in-memory json config with new entry, rewrite
if there's stale entry
for loop
open xs transaction
check device existence, abort if it exists
write in-memory json config to disk
commit xs transaction
end for loop
unlock json config
Please see comment in libxl_internal.h for correctness proof.
As those routines are called both during domain creation and device
hotplug, we add a flag to indicate whether we need to update JSON
config. This flag is only set to true when we hotplug a device. We
cannot update JSON config during domain creation as JSON config is
committed to disk only when domain creation finishes.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 99 +++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 131 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_pci.c | 24 ++++++++
3 files changed, 254 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ad3495a..fa7d5d5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1894,6 +1894,13 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
libxl__device *device;
int rc;
xs_transaction_t t = XBT_NULL;
+ libxl_domain_config d_config;
+ libxl_device_vtpm vtpm_saved;
+ libxl__carefd *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_vtpm_init(&vtpm_saved);
+ libxl_device_vtpm_copy(CTX, &vtpm_saved, vtpm);
rc = libxl__device_vtpm_setdefault(gc, vtpm);
if (rc) goto out;
@@ -1908,6 +1915,8 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
}
}
+ libxl__update_config_vtpm(gc, &vtpm_saved, vtpm);
+
GCNEW(device);
rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
if ( rc != 0 ) goto out;
@@ -1933,6 +1942,19 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
flexarray_append(front, "handle");
flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
+ if (aodev->update_json) {
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
+ DEVICE_ADD(vtpm, vtpms, domid, &vtpm_saved, COMPARE_DEVID, &d_config);
+ }
+
for (;;) {
rc = libxl__xs_transaction_start(gc, &t);
if (rc) goto out;
@@ -1946,6 +1968,11 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
goto out;
}
+ if (aodev->update_json) {
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ }
+
libxl__device_generic_add(gc, t, device,
libxl__xs_kvs_of_flexarray(gc, back,
back->count),
@@ -1965,6 +1992,9 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
libxl__xs_transaction_abort(gc, &t);
+ if (lock) libxl__unlock_domain_userdata(lock);
+ libxl_device_vtpm_dispose(&vtpm_saved);
+ libxl_domain_config_dispose(&d_config);
aodev->rc = rc;
if(rc) aodev->callback(egc, aodev);
return;
@@ -2197,6 +2227,13 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
int rc;
libxl_ctx *ctx = gc->owner;
xs_transaction_t t = XBT_NULL;
+ libxl_domain_config d_config;
+ libxl_device_disk disk_saved;
+ libxl__carefd *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_disk_init(&disk_saved);
+ libxl_device_disk_copy(ctx, &disk_saved, disk);
libxl_domain_type type = libxl__domain_type(gc, domid);
if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -2204,6 +2241,29 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
goto out;
}
+ /*
+ * get_vdev != NULL -> local attach
+ * get_vdev == NULL -> block attach
+ *
+ * We don't care about local attach state because it's only
+ * intermediate state. What's nice is that vdev won't change in
+ * "block-attach" case so the macro works as expected -- it can
+ * safely skip an existing entry. See similar check in the for
+ * loop below.
+ */
+ if (!get_vdev && aodev->update_json) {
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
+ DEVICE_ADD(disk, disks, domid, &disk_saved, COMPARE_DISK, &d_config);
+ }
+
for (;;) {
rc = libxl__xs_transaction_start(gc, &t);
if (rc) goto out;
@@ -2356,6 +2416,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
}
}
+ if (!get_vdev && aodev->update_json) {
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ }
+
libxl__device_generic_add(gc, t, device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count),
@@ -2374,6 +2439,9 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
out:
libxl__xs_transaction_abort(gc, &t);
+ if (lock) libxl__unlock_domain_userdata(lock);
+ libxl_device_disk_dispose(&disk_saved);
+ libxl_domain_config_dispose(&d_config);
aodev->rc = rc;
if (rc) aodev->callback(egc, aodev);
return;
@@ -3030,6 +3098,13 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
libxl__device *device;
int rc;
xs_transaction_t t = XBT_NULL;
+ libxl_domain_config d_config;
+ libxl_device_nic nic_saved;
+ libxl__carefd *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_nic_init(&nic_saved);
+ libxl_device_nic_copy(CTX, &nic_saved, nic);
rc = libxl__device_nic_setdefault(gc, nic, domid);
if (rc) goto out;
@@ -3044,6 +3119,8 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
}
}
+ libxl__update_config_nic(gc, &nic_saved, nic);
+
GCNEW(device);
rc = libxl__device_from_nic(gc, domid, nic, device);
if ( rc != 0 ) goto out;
@@ -3101,6 +3178,19 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
flexarray_append(front, libxl__sprintf(gc,
LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
+ if (aodev->update_json) {
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
+ DEVICE_ADD(nic, nics, domid, &nic_saved, COMPARE_DEVID, &d_config);
+ }
+
for (;;) {
rc = libxl__xs_transaction_start(gc, &t);
if (rc) goto out;
@@ -3114,6 +3204,11 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
goto out;
}
+ if (aodev->update_json) {
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ }
+
libxl__device_generic_add(gc, t, device,
libxl__xs_kvs_of_flexarray(gc, back,
back->count),
@@ -3133,6 +3228,9 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
libxl__xs_transaction_abort(gc, &t);
+ if (lock) libxl__unlock_domain_userdata(lock);
+ libxl_device_nic_dispose(&nic_saved);
+ libxl_domain_config_dispose(&d_config);
aodev->rc = rc;
if (rc) aodev->callback(egc, aodev);
return;
@@ -3686,6 +3784,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
aodev->callback = device_addrm_aocomplete; \
+ aodev->update_json = true; \
libxl__device_##type##_add(egc, domid, type, aodev); \
\
return AO_INPROGRESS; \
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fafef5a..2546f88 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2146,6 +2146,8 @@ struct libxl__ao_device {
int num_exec;
/* for calling hotplug scripts */
libxl__async_exec_state aes;
+ /* If we need to udpate JSON config */
+ bool update_json;
};
/*
@@ -2271,6 +2273,78 @@ struct libxl__multidev {
*
* Once finished, aodev->callback will be executed.
*/
+/*
+ * As of Xen 4.5 we maintain various infomation, including hotplug
+ * device information, in JSON files, so that we can use this JSON
+ * file as a template to reconstruct domain configuration.
+ *
+ * In essnse there are now two views of device state, one is xenstore,
+ * the other is JSON file. We use xenstore as primary reference.
+ *
+ * Here we maintain one invariant: every device in xenstore must have
+ * an entry in JSON file.
+ *
+ * All device hotplug routines should comply to following pattern:
+ * lock json config (json_lock)
+ * read json config
+ * update in-memory json config with new entry, rewrite
+ * if there's stale entry
+ * for loop -- xs transaction
+ * open xs transaction
+ * check device existence, abort if it exists
+ * write in-memory json config to disk
+ * commit xs transaction
+ * end for loop
+ * unlock json config
+ *
+ * Device removal routines are not touched.
+ *
+ * Here is the proof that we always maintain that invariant and we
+ * don't leak files during interaction of hotplug thread and other
+ * threads / processes.
+ *
+ * # Safe against parallel add
+ *
+ * When another thread / process tries to add same device, it's
+ * blocked by json_lock. The loser of two threads will bail at
+ * existence check, so that we don't overwrite anything.
+ *
+ * # Safe against domain destruction
+ *
+ * When another thread / process tries to destroy domain, it's blocked
+ * by json_lock. If domain destruction thread is loser, it deletes
+ * every userdata file after it requires the lock. If hotplug thread
+ * is loser, it bails at acquiring lock, no device is added. Either
+ * way, no file is leaked.
+ *
+ * # Safe against parallel removal
+ *
+ * When another thread / process tries to remove a device, it's _NOT_
+ * blocked by json_lock, but xenstore transaction can help maintain
+ * invariant. The removal threads either a) sees that device in
+ * xenstore, b) doesn't see that device in xenstore.
+ *
+ * In a), it sees that device in xenstore. At that point hotplug is
+ * already finished (both JSON and xenstore change committed). So that
+ * device can be safely removed. JSON entry is left untouched and
+ * becomes stale, but this is a valid state -- next time when a
+ * device with same identifier gets added, the stale entry gets
+ * overwritten.
+ *
+ * In b), it doesn't see that device in xenstore, but it will commence
+ * anyway. Eventually a force removal is initiated, which will forcely
+ * remove xenstore entry.
+ *
+ * If hotplug threads creates xenstore entry (therefore JSON entry as
+ * well) before force removal, that xenstore entry is removed. We're
+ * left with JSON stale entry but not xenstore entry, which is a valid
+ * state.
+ *
+ * If hotplug thread has not created xenstore entry when the removal
+ * is committed, we're obviously safe. Hotplug thread will add in
+ * xenstore entry afterwards. We have both JSON and xenstore entry,
+ * it's a valid state.
+ */
_hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
libxl_device_disk *disk,
libxl__ao_device *aodev);
@@ -3273,6 +3347,63 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
libxl_uuid_copy(CTX, &dst->uuid, &src->uuid);
}
+/* Macros used to compare device identifier. Returns true if the two
+ * devices have same identifier. */
+#define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid)
+#define COMPARE_DISK(a, b) (!strcmp((a)->vdev, (b)->vdev))
+#define COMPARE_PCI(a, b) ((a)->func == (b)->func && \
+ (a)->bus == (b)->bus && \
+ (a)->dev == (b)->dev)
+
+/* DEVICE_ADD
+ *
+ * Add a device in libxl_domain_config structure
+ *
+ * It takes 6 parameters:
+ * type: the type of the device, say nic, vtpm, disk, pci etc
+ * ptr: pointer to the start of the array, the array must be
+ * of type libxl_device_#type
+ * domid: domain id of target domain
+ * dev: the device that is to be added / removed / updated
+ * compare: the COMPARE_* macro used to compare @dev's identifier to
+ * those in the array pointed to by @ptr
+ * d_config: pointer to template domain config
+ *
+ * For most device types (nic, vtpm), the array pointer @ptr can be
+ * derived from @type, pci device being the exception, hence we need
+ * to have @ptr.
+ *
+ * If there is already a device with the same identifier in d_config,
+ * that entry is updated.
+ */
+#define DEVICE_ADD(type, ptr, domid, dev, compare, d_config) \
+ ({ \
+ int DA_x; \
+ libxl_device_##type *DA_p = NULL; \
+ \
+ /* Check for existing device */ \
+ for (DA_x = 0; DA_x < (d_config)->num_##ptr; DA_x++) { \
+ if (compare(&(d_config)->ptr[DA_x], (dev))) { \
+ DA_p = &(d_config)->ptr[DA_x]; \
+ break; \
+ } \
+ } \
+ \
+ if (!DA_p) { \
+ (d_config)->ptr = \
+ libxl__realloc(NOGC, (d_config)->ptr, \
+ ((d_config)->num_##ptr + 1) * \
+ sizeof(libxl_device_##type)); \
+ DA_p = &(d_config)->ptr[(d_config)->num_##ptr]; \
+ (d_config)->num_##ptr++; \
+ } else { \
+ libxl_device_##type##_dispose(DA_p); \
+ } \
+ \
+ libxl_device_##type##_init(DA_p); \
+ libxl_device_##type##_copy(CTX, DA_p, (dev)); \
+ })
+
#endif
/*
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 38a9642..b87819f 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -126,6 +126,13 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
xs_transaction_t t;
libxl__device *device;
int rc;
+ libxl_domain_config d_config;
+ libxl_device_pci pcidev_saved;
+ libxl__carefd *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_pci_init(&pcidev_saved);
+ libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0", libxl__xs_get_dompath(gc, 0), domid);
num_devs = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/num_devs", be_path));
@@ -153,6 +160,17 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
GCNEW(device);
libxl__device_from_pcidev(gc, domid, pcidev, device);
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
+ DEVICE_ADD(pci, pcidevs, domid, &pcidev_saved, COMPARE_PCI, &d_config);
+
for (;;) {
rc = libxl__xs_transaction_start(gc, &t);
if (rc) goto out;
@@ -165,6 +183,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
goto out;
}
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
libxl__xs_writev(gc, t, be_path,
libxl__xs_kvs_of_flexarray(gc, back, back->count));
@@ -175,6 +196,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
out:
libxl__xs_transaction_abort(gc, &t);
+ if (lock) libxl__unlock_domain_userdata(lock);
+ libxl_device_pci_dispose(&pcidev_saved);
+ libxl_domain_config_dispose(&d_config);
return rc;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert"
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (8 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 11:30 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 11/15] libxl: refactor libxl_get_memory_target Wei Liu
` (4 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
A "cdrom insert" is always processed as "eject" + "insert", with JSON
config updated in between. So that we can know the correct state of
CDROM later when we try to retrieve domain configuration: if xenstore is
"empty", then CDROM is "empty"; otherwise use the information presented
in JSON.
Note that libxl_cd_insert doesn't care about the incarnation of "empty"
state so I don't state explictly what is in xenstore.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
clarify "empty" in commit log
---
tools/libxl/libxl.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 65 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index fa7d5d5..1fdb2d8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2662,8 +2662,9 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
return 0;
}
-int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how)
+static int libxl__cdrom_insert(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
{
AO_CREATE(ctx, domid, ao_how);
int num = 0, i;
@@ -2774,6 +2775,68 @@ out:
return AO_INPROGRESS;
}
+/*
+ * A "cdrom insert" is always processed as "eject" + "insert", with
+ * updating JSON in between. So that we can know the current state of
+ * CDROM later when we try to retrieve domain configuration: if
+ * xenstore is "empty", then CDROM is "empty"; otherwise use the image
+ * in JSON.
+ *
+ * Note that we don't actually care about the incarnation of "empty"
+ * state so we don't state explicitly what the content in xenstore
+ * should be.
+ *
+ * In order to maintain lock hierarchy, CTX lock is taken when
+ * entering this function.
+ */
+int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
+{
+ GC_INIT(ctx);
+ libxl_domain_config d_config;
+ libxl_device_disk empty;
+ int rc;
+ libxl__carefd *lock = NULL;
+
+ CTX_LOCK;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_disk_init(&empty);
+ empty.format = LIBXL_DISK_FORMAT_EMPTY;
+ empty.vdev = libxl__strdup(NOGC, disk->vdev);
+ empty.is_cdrom = 1;
+
+ rc = libxl__cdrom_insert(ctx, domid, &empty, NULL);
+ if (rc)
+ goto out;
+
+ /* Optimisation: don't insert empty disk twice, and skip
+ * manipulating JSON. */
+ if (disk->format == LIBXL_DISK_FORMAT_EMPTY)
+ goto out;
+
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ DEVICE_ADD(disk, disks, domid, disk, COMPARE_DISK, &d_config);
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ libxl__unlock_domain_userdata(lock);
+
+ rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
+
+out:
+ libxl_device_disk_dispose(&empty);
+ libxl_domain_config_dispose(&d_config);
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
/* libxl__alloc_vdev only works on the local domain, that is the domain
* where the toolstack is running */
static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 11/15] libxl: refactor libxl_get_memory_target
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (9 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 11:36 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 12/15] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
` (3 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Introduce a helper function which can return either "target" node or
"static-max" node of a domain. Reimplement libxl_get_memory_target using
this helper. libxl__fill_dom0_memory_info is adjusted as well.
This helper will be used in later patch.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
don't introduce new public function
---
tools/libxl/libxl.c | 88 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 71 insertions(+), 17 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1fdb2d8..e2a956c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4245,7 +4245,8 @@ out:
return rc;
}
-static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb)
+static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
+ uint32_t *max_memkb)
{
int rc;
libxl_dominfo info;
@@ -4279,6 +4280,17 @@ retry_transaction:
}
}
+ if (staticmax) {
+ *max_memkb = strtoul(staticmax, &endptr, 10);
+ if (*endptr != '\0') {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "invalid memory static-max %s from %s\n",
+ staticmax, max_path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
rc = libxl_domain_info(ctx, &info, 0);
if (rc < 0)
goto out;
@@ -4292,9 +4304,11 @@ retry_transaction:
(uint32_t) info.current_memkb);
*target_memkb = (uint32_t) info.current_memkb;
}
- if (staticmax == NULL)
+ if (staticmax == NULL) {
libxl__xs_write(gc, t, max_path, "%"PRIu32,
- (uint32_t) info.max_memkb);
+ (uint32_t) info.max_memkb);
+ *max_memkb = (uint32_t) info.max_memkb;
+ }
if (freememslack == NULL) {
free_mem_slack_kb = (uint32_t) (PAGE_TO_MEMKB(physinfo.total_pages) -
@@ -4325,12 +4339,12 @@ static int libxl__get_free_memory_slack(libxl__gc *gc, uint32_t *free_mem_slack)
int rc;
char *free_mem_slack_path = "/local/domain/0/memory/freemem-slack";
char *free_mem_slack_s, *endptr;
- uint32_t target_memkb;
+ uint32_t target_memkb, max_memkb;
retry:
free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
if (!free_mem_slack_s) {
- rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
+ rc = libxl__fill_dom0_memory_info(gc, &target_memkb, &max_memkb);
if (rc < 0)
return rc;
goto retry;
@@ -4353,6 +4367,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
int rc = 1, abort_transaction = 0;
uint32_t memorykb = 0, videoram = 0;
uint32_t current_target_memkb = 0, new_target_memkb = 0;
+ uint32_t current_max_memkb = 0;
char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
char *dompath = libxl__xs_get_dompath(gc, domid);
xc_domaininfo_t info;
@@ -4368,7 +4383,8 @@ retry_transaction:
if (!target && !domid) {
if (!xs_transaction_end(ctx->xsh, t, 1))
goto out_no_transaction;
- rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb);
+ rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb,
+ ¤t_max_memkb);
if (rc < 0)
goto out_no_transaction;
goto retry_transaction;
@@ -4483,38 +4499,76 @@ out_no_transaction:
return rc;
}
-int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target)
+/* bool get_target:
+ * true -> retrieve "target" node
+ * false -> retrieve "static-max" node
+ */
+static int libxl__get_memory_target(libxl__gc *gc, uint32_t domid,
+ uint32_t *out_mem, bool get_target)
{
- GC_INIT(ctx);
- int rc = 1;
- char *target = NULL, *endptr = NULL;
+ int rc;
+ char *target = NULL, *static_max = NULL, *endptr = NULL;
char *dompath = libxl__xs_get_dompath(gc, domid);
- uint32_t target_memkb;
+ uint32_t target_memkb, max_memkb;
target = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
- "%s/memory/target", dompath));
- if (!target && !domid) {
- rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
+ "%s/memory/target", dompath));
+ static_max = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
+ "%s/memory/static-max", dompath));
+
+ rc = ERROR_FAIL;
+ if ((!target || !static_max) && !domid) {
+ rc = libxl__fill_dom0_memory_info(gc, &target_memkb,
+ &max_memkb);
if (rc < 0)
goto out;
} else if (!target) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
"cannot get target memory info from %s/memory/target\n",
dompath);
goto out;
+ } else if (!static_max) {
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
+ "cannot get target memory info from %s/memory/static-max\n",
+ dompath);
+ goto out;
} else {
target_memkb = strtoul(target, &endptr, 10);
if (*endptr != '\0') {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
"invalid memory target %s from %s/memory/target\n",
target, dompath);
goto out;
}
+ max_memkb = strtoul(static_max, &endptr, 10);
+ if (*endptr != '\0') {
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
+ "invalid memory target %s from %s/memory/static-max\n",
+ static_max, dompath);
+ goto out;
+ }
+
}
- *out_target = target_memkb;
+
+ if (!get_target)
+ *out_mem = max_memkb;
+ else
+ *out_mem = target_memkb;
+
rc = 0;
out:
+ return rc;
+}
+
+int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid,
+ uint32_t *out_target)
+{
+ GC_INIT(ctx);
+ int rc;
+
+ rc = libxl__get_memory_target(gc, domid, out_target, true);
+
GC_FREE;
return rc;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 12/15] libxl: introduce libxl_retrieve_domain_configuration
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (10 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 11/15] libxl: refactor libxl_get_memory_target Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 11:41 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 13/15] libxl: introduce libxl_userdata_unlink Wei Liu
` (2 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Introduce a new public API to return domain configuration. This returned
configuration can be used to rebuild a domain.
Note that this configuration only describes the configuration necessary
to reproduce the guest visible state and does not necessarily include
specific decisions made by the toolstack regarding its current
incarnation (e.g. disk backend) unless they were specified by the
application when the domain was created.
With this approach we can preserve what user has provided in the
original configuration as well as valuable information from xenstore.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
correct target memory calculation
---
tools/libxl/libxl.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl.h | 16 +++++
2 files changed, 210 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e2a956c..2e8ea72 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6119,6 +6119,200 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src)
for (i = 0; i < 6; i++)
(*dst)[i] = (*src)[i];
}
+
+int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
+ libxl_domain_config *d_config)
+{
+ GC_INIT(ctx);
+ int rc;
+ libxl__carefd *lock = NULL;
+
+ CTX_LOCK;
+
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, d_config);
+ if (rc) {
+ LOG(ERROR, "fail to get domain configuration for domain %d", domid);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Domain name */
+ {
+ char *domname;
+ domname = libxl_domid_to_name(ctx, domid);
+ if (!domname) {
+ LOG(ERROR, "fail to get domain name for domain %d", domid);
+ goto out;
+ }
+ free(d_config->c_info.name);
+ d_config->c_info.name = domname; /* steals allocation */
+ }
+
+ /* Domain UUID */
+ {
+ libxl_dominfo info;
+ rc = libxl_domain_info(ctx, &info, domid);
+ if (rc) {
+ LOG(ERROR, "fail to get domain info for domain %d", domid);
+ goto out;
+ }
+ libxl_uuid_copy(ctx, &d_config->c_info.uuid, &info.uuid);
+ }
+
+ /* Memory limits:
+ *
+ * Currently there are three memory limits:
+ * 1. "target" in xenstore (originally memory= in config file)
+ * 2. "static-max" in xenstore (originally maxmem= in config file)
+ * 3. "max_memkb" in hypervisor
+ *
+ * The third one is not visible and currently managed by
+ * toolstack. In order to rebuild a domain we only need to have
+ * "target" and "static-max".
+ */
+ {
+ uint32_t memory;
+
+ /* "target" */
+ rc = libxl__get_memory_target(gc, domid, &memory, true);
+ if (rc) {
+ LOG(ERROR, "fail to get memory target for domain %d", domid);
+ goto out;
+ }
+ /* Target memory in xenstore is different from what user has
+ * asked for. The difference is video_memkb. See
+ * libxl_set_memory_target.
+ */
+ d_config->b_info.target_memkb = memory + d_config->b_info.video_memkb;
+
+ /* "static-max" */
+ rc = libxl__get_memory_target(gc, domid, &memory, false);
+ if (rc) {
+ LOG(ERROR, "fail to get memory static-max for domain %d", domid);
+ goto out;
+ }
+ d_config->b_info.max_memkb = memory;
+ }
+
+ /* Devices: disk, nic, vtpm, pcidev etc. */
+
+ /* The MERGE macro implements following logic:
+ * 0. retrieve JSON (done by now)
+ * 1. retrieve list of device from xenstore
+ * 2. use xenstore entries as primary reference and compare JSON
+ * entries with them.
+ * a. if a device is present in xenstore and in JSON, merge the
+ * two views.
+ * b. if a device is not present in xenstore but in JSON, delete
+ * it from the result.
+ * c. it's impossible to have an entry present in xenstore but
+ * not in JSON, because we maintain an invariant that every
+ * entry in xenstore must have a corresponding entry in JSON.
+ * 3. "merge" operates on "src" and "dst". "src" points to the
+ * entry retrieved from xenstore while "dst" points to the entry
+ * retrieve from JSON.
+ */
+#define MERGE(type, ptr, compare, merge) \
+ do { \
+ libxl_device_##type *p = NULL; \
+ int i, j, num; \
+ \
+ p = libxl_device_##type##_list(CTX, domid, &num); \
+ if (p == NULL) { \
+ LOG(DEBUG, \
+ "no %s from xenstore for domain %d", \
+ #type, domid); \
+ } \
+ \
+ for (i = 0; i < d_config->num_##ptr; i++) { \
+ libxl_device_##type *q = &d_config->ptr[i]; \
+ for (j = 0; j < num; j++) { \
+ if (compare(&p[j], q)) \
+ break; \
+ } \
+ \
+ if (j < num) { /* found in xenstore */ \
+ libxl_device_##type *dst, *src; \
+ dst = q; \
+ src = &p[j]; \
+ merge; \
+ } else { /* not found in xenstore */ \
+ LOG(WARN, \
+ "Device present in JSON but not in xenstore, ignored"); \
+ \
+ libxl_device_##type##_dispose(q); \
+ \
+ for (j = i; j < d_config->num_##ptr - 1; j++) \
+ memcpy(&d_config->ptr[j], &d_config->ptr[j+1], \
+ sizeof(libxl_device_##type)); \
+ \
+ d_config->ptr = \
+ libxl__realloc(NOGC, d_config->ptr, \
+ sizeof(libxl_device_##type) * \
+ (d_config->num_##ptr - 1)); \
+ \
+ /* rewind counters */ \
+ d_config->num_##ptr--; \
+ i--; \
+ } \
+ } \
+ \
+ for (i = 0; i < num; i++) \
+ libxl_device_##type##_dispose(&p[i]); \
+ free(p); \
+ } while (0);
+
+ MERGE(nic, nics, COMPARE_DEVID, {
+ libxl__update_config_nic(gc, dst, src);
+ });
+
+ MERGE(vtpm, vtpms, COMPARE_DEVID, {
+ libxl__update_config_vtpm(gc, dst, src);
+ });
+
+ MERGE(pci, pcidevs, COMPARE_PCI, {});
+
+ /* Take care of removable device. We maintain invariant in the
+ * insert / remove operation so that:
+ * 1. if xenstore is "empty" while JSON is not, the result
+ * is "empty"
+ * 2. if xenstore has a different media than JSON, use the
+ * one in JSON
+ * 3. if xenstore and JSON have the same media, well, you
+ * know the answer :-)
+ *
+ * Currently there is only one removable device -- CDROM.
+ * Look for libxl_cdrom_insert for reference.
+ */
+ MERGE(disk, disks, COMPARE_DISK, {
+ if (src->removable) {
+ if (!src->pdev_path || *src->pdev_path == '\0') {
+ /* 1, use "empty" */
+ free(dst->pdev_path);
+ dst->pdev_path = libxl__strdup(NOGC, "");
+ dst->format = LIBXL_DISK_FORMAT_EMPTY;
+ } else {
+ /* 2 and 3, use JSON, no need to touch anything */
+ ;
+ }
+ }
+ });
+
+#undef MERGE
+
+out:
+ if (lock) libxl__unlock_domain_userdata(lock);
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index dab3a67..7243023 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -368,6 +368,14 @@ typedef struct libxl__ctx libxl_ctx;
#endif
#endif
+/* LIBXL_HAVE_RETRIEVE_DOMAIN_CONFIGURATION
+ *
+ * If this is defined we have libxl_retrieve_domain_configuration which
+ * returns the current configuration of a domain, which can be used to
+ * rebuild a domain.
+ */
+#define LIBXL_HAVE_RETRIEVE_DOMAIN_CONFIGURATION 1
+
/*
* LIBXL_HAVE_BUILDINFO_VCPU_AFFINITY_ARRAYS
*
@@ -865,6 +873,14 @@ int static inline libxl_domain_create_restore_0x040200(
void libxl_domain_config_init(libxl_domain_config *d_config);
void libxl_domain_config_dispose(libxl_domain_config *d_config);
+/*
+ * Retrieve domain configuration and filled it in d_config. The
+ * returned configuration can be used to rebuild a domain. It only
+ * works with DomU.
+ */
+int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
+ libxl_domain_config *d_config);
+
int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
int flags, /* LIBXL_SUSPEND_* */
const libxl_asyncop_how *ao_how)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 13/15] libxl: introduce libxl_userdata_unlink
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (11 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 12/15] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 11:42 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 14/15] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
2014-09-04 22:43 ` [PATCH v3 15/15] xl: long output of "list" command now contains Dom0 information Wei Liu
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
This will be used in later patch for xl to remove its "xl" userdata
file.
Both CTX lock and userdata lock are taken in this API. CTX lock is taken
to maintain locking hierarchy, but it also has a side effect to protect
against R-M-W by other threads. Userdata lock is used to protect against
domain destruction.
In general application should not rely on these internal locks to
protect its own userdata files. It should deploys its own lock if it
cares.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
more commit message
---
tools/libxl/libxl.h | 10 ++++++++++
tools/libxl/libxl_dom.c | 25 +++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7243023..ea29baa 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -67,6 +67,13 @@
* the same $(XEN_VERSION) (e.g. throughout a major release).
*/
+/* LIBXL_HAVE_USERDATA_UNLINK
+ *
+ * If it is defined, libxl has a library function called
+ * libxl_userdata_unlink.
+ */
+#define LIBXL_HAVE_USERDATA_UNLINK 1
+
/* LIBXL_HAVE_CPUPOOL_QUALIFIER_TO_CPUPOOLID
*
* If this is defined, libxl has a library function called
@@ -1263,6 +1270,9 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
* data_r and datalen_r may be 0.
* On error return, *data_r and *datalen_r are undefined.
*/
+int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
+ const char *userdata_userid);
+
int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 0dfdb08..0c753b0 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -2012,6 +2012,31 @@ out:
return rc;
}
+int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
+ const char *userdata_userid)
+{
+ GC_INIT(ctx);
+ int rc;
+ libxl__carefd *lock;
+ const char *filename;
+
+ CTX_LOCK;
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ filename = libxl__userdata_path(gc, domid, userdata_userid, "d");
+ if (unlink(filename)) rc = ERROR_FAIL;
+
+ libxl__unlock_domain_userdata(lock);
+out:
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
/*
* Local variables:
* mode: C
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 14/15] xl: use libxl_retrieve_domain_configuration and JSON format
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (12 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 13/15] libxl: introduce libxl_userdata_unlink Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
2014-09-09 11:44 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 15/15] xl: long output of "list" command now contains Dom0 information Wei Liu
14 siblings, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Before this change, xl stores domain configuration in "xl" format, which
is in fact a verbatim copy of user supplied domain config.
Now libxl provides a new API to retrieve domain configuration, switch to
that new API, store configuration in JSON format.
Tests done so far (xl.{new,old} denotes xl with{,out} "libxl-json"
support):
1. xl.new create then xl.new save, hexdump saved file: domain config
saved in JSON format
2. xl.new create, xl.new save then xl.old restore: failed on
mandatory flag check
3. xl.new create, xl.new save then xl.new restore: succeeded
4. xl.old create, xl.old save then xl.new restore: succeeded
5. xl.new create then local migrate, receiving end xl.new: succeeded
6. xl.old create then local migrate, receiving end xl.new: succeeded
Note that "xl" config is still supported and handled when restarting a
domain. "xl" config file takes precedence over "libxl-json" in that
case, so that user who uses "config-update" to store new config file
won't have regression. All other scenarios (migration, domain listing
etc.) now use the new API.
Lastly, print out warning when users invoke "config-update" to
discourage them from using this command.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
change in v3:
improve error message and document wording
---
docs/man/xl.pod.1 | 5 ++
tools/libxl/xl_cmdimpl.c | 124 ++++++++++++++++++++++++++++-----------------
tools/libxl/xl_cmdtable.c | 4 +-
3 files changed, 85 insertions(+), 48 deletions(-)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 9d1c2a5..f1e95db 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -187,6 +187,11 @@ immediate effect but will be applied when the guest is next
restarted. This command is useful to ensure that runtime modifications
made to the guest will be preserved when the guest is restarted.
+Since Xen 4.5 xl has improved capabilities to handle dynamic domain
+configuration changes and will preserve any changes made a runtime
+when necessary. Therefore it should not normally be necessary to use
+this command any more.
+
I<configfile> has to be an absolute path to a file.
B<OPTIONS>
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e6b9615..0702752 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -107,6 +107,8 @@ static const char migrate_report[]=
* from target to source
*/
+#define XL_MANDATORY_FLAG_JSON (1U << 0) /* config data is in JSON format */
+#define XL_MANDATORY_FLAG_ALL (XL_MANDATORY_FLAG_JSON)
struct save_file_header {
char magic[32]; /* savefileheader_magic */
/* All uint32_ts are in domain's byte order. */
@@ -1778,21 +1780,41 @@ skip_vfb:
}
static void reload_domain_config(uint32_t domid,
- uint8_t **config_data, int *config_len)
+ libxl_domain_config *d_config)
{
+ int rc;
uint8_t *t_data;
int ret, t_len;
+ libxl_domain_config d_config_new;
+ /* In case user has used "config-update" to store a new config
+ * file.
+ */
ret = libxl_userdata_retrieve(ctx, domid, "xl", &t_data, &t_len);
- if (ret) {
- LOG("failed to retrieve guest configuration (rc=%d). "
- "reusing old configuration", ret);
+ if (ret && errno != ENOENT) {
+ LOG("\"xl\" configuration found but failed to load\n");
+ }
+ if (t_len > 0) {
+ LOG("\"xl\" configuration found, using it\n");
+ libxl_domain_config_dispose(d_config);
+ parse_config_data("<updated>", (const char *)t_data,
+ t_len, d_config);
+ free(t_data);
+ libxl_userdata_unlink(ctx, domid, "xl");
return;
}
- free(*config_data);
- *config_data = t_data;
- *config_len = t_len;
+ libxl_domain_config_init(&d_config_new);
+ rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config_new);
+ if (rc) {
+ LOG("failed to retrieve guest configuration (rc=%d). "
+ "reusing old configuration", rc);
+ libxl_domain_config_dispose(&d_config_new);
+ } else {
+ libxl_domain_config_dispose(d_config);
+ /* Steal allocations */
+ memcpy(d_config, &d_config_new, sizeof(libxl_domain_config));
+ }
}
/* Returns 1 if domain should be restarted,
@@ -1800,7 +1822,6 @@ static void reload_domain_config(uint32_t domid,
* Can update r_domid if domain is destroyed etc */
static int handle_domain_death(uint32_t *r_domid,
libxl_event *event,
- uint8_t **config_data, int *config_len,
libxl_domain_config *d_config)
{
@@ -1858,13 +1879,12 @@ static int handle_domain_death(uint32_t *r_domid,
break;
case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
- reload_domain_config(*r_domid, config_data, config_len);
+ reload_domain_config(*r_domid, d_config);
restart = 2;
break;
case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
- reload_domain_config(*r_domid, config_data, config_len);
-
+ reload_domain_config(*r_domid, d_config);
restart = 1;
/* fall-through */
case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
@@ -2061,6 +2081,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
const char *config_source = NULL;
const char *restore_source = NULL;
int migrate_fd = dom_info->migrate_fd;
+ bool config_in_json;
int i;
int need_daemon = daemonize;
@@ -2115,7 +2136,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
restore_source, hdr.mandatory_flags, hdr.optional_flags,
hdr.optional_data_len);
- badflags = hdr.mandatory_flags & ~( 0 /* none understood yet */ );
+ badflags = hdr.mandatory_flags & ~XL_MANDATORY_FLAG_ALL;
if (badflags) {
fprintf(stderr, "Savefile has mandatory flag(s) 0x%"PRIx32" "
"which are not supported; need newer xl\n",
@@ -2143,7 +2164,9 @@ static uint32_t create_domain(struct domain_create *dom_info)
optdata_here = optdata_begin;
if (OPTDATA_LEFT) {
- fprintf(stderr, " Savefile contains xl domain config\n");
+ fprintf(stderr, " Savefile contains xl domain config%s\n",
+ !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON)
+ ? " in JSON format" : "");
WITH_OPTDATA(4, {
memcpy(u32buf.b, optdata_here, 4);
config_len = u32buf.u32;
@@ -2183,6 +2206,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
extra_config);
}
config_source=config_file;
+ config_in_json = false;
} else {
if (!config_data) {
fprintf(stderr, "Config file not specified and"
@@ -2190,12 +2214,18 @@ static uint32_t create_domain(struct domain_create *dom_info)
return ERROR_INVAL;
}
config_source = "<saved>";
+ config_in_json = !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON);
}
if (!dom_info->quiet)
printf("Parsing config from %s\n", config_source);
- parse_config_data(config_source, config_data, config_len, &d_config);
+ if (config_in_json) {
+ libxl_domain_config_from_json(ctx, &d_config,
+ (const char *)config_data);
+ } else {
+ parse_config_data(config_source, config_data, config_len, &d_config);
+ }
if (migrate_fd >= 0) {
if (d_config.c_info.name) {
@@ -2263,14 +2293,6 @@ start:
if ( ret )
goto error_out;
- ret = libxl_userdata_store(ctx, domid, "xl",
- config_data, config_len);
- if (ret) {
- perror("cannot save config file");
- ret = ERROR_FAIL;
- goto error_out;
- }
-
release_lock();
if (!paused)
@@ -2327,9 +2349,7 @@ start:
LOG("Domain %d has shut down, reason code %d 0x%x", domid,
event->u.domain_shutdown.shutdown_reason,
event->u.domain_shutdown.shutdown_reason);
- switch (handle_domain_death(&domid, event,
- (uint8_t **)&config_data, &config_len,
- &d_config)) {
+ switch (handle_domain_death(&domid, event, &d_config)) {
case 2:
if (!preserve_domain(&domid, event, &d_config)) {
/* If we fail then exit leaving the old domain in place. */
@@ -2366,12 +2386,6 @@ start:
d_config.c_info.name = strdup(common_domname);
}
- /* Reparse the configuration in case it has changed */
- libxl_domain_config_dispose(&d_config);
- libxl_domain_config_init(&d_config);
- parse_config_data(config_source, config_data, config_len,
- &d_config);
-
/*
* XXX FIXME: If this sleep is not there then domain
* re-creation fails sometimes.
@@ -3150,9 +3164,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
{
libxl_domain_config d_config;
- char *config_source;
- uint8_t *data;
- int i, len, rc;
+ int i, rc;
yajl_gen hand = NULL;
yajl_gen_status s;
@@ -3173,24 +3185,18 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
s = yajl_gen_status_ok;
for (i = 0; i < nb_domain; i++) {
+ libxl_domain_config_init(&d_config);
/* no detailed info available on dom0 */
if (info[i].domid == 0)
continue;
- rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl", &data, &len);
+ rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
if (rc)
continue;
- if (len == 0)
- continue;
- CHK_SYSCALL(asprintf(&config_source, "<domid %d data>", info[i].domid));
- libxl_domain_config_init(&d_config);
- parse_config_data(config_source, (char *)data, len, &d_config);
if (default_output_format == OUTPUT_FORMAT_JSON)
s = printf_info_one_json(hand, info[i].domid, &d_config);
else
printf_info_sexp(info[i].domid, &d_config);
libxl_domain_config_dispose(&d_config);
- free(data);
- free(config_source);
if (s != yajl_gen_status_ok)
goto out;
}
@@ -3375,22 +3381,41 @@ static void save_domain_core_begin(uint32_t domid,
int *config_len_r)
{
int rc;
+ libxl_domain_config d_config;
+ char *config_c = 0;
/* configuration file in optional data: */
+ libxl_domain_config_init(&d_config);
+
if (override_config_file) {
void *config_v = 0;
rc = libxl_read_file_contents(ctx, override_config_file,
&config_v, config_len_r);
- *config_data_r = config_v;
+ if (rc) {
+ fprintf(stderr, "unable to read overridden config file\n");
+ exit(2);
+ }
+ parse_config_data(override_config_file, config_v, *config_len_r,
+ &d_config);
+ free(config_v);
} else {
- rc = libxl_userdata_retrieve(ctx, domid, "xl",
- config_data_r, config_len_r);
+ rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config);
+ if (rc) {
+ fprintf(stderr, "unable to retrieve domain configuration\n");
+ exit(2);
+ }
}
- if (rc) {
- fputs("Unable to get config file\n",stderr);
+
+ config_c = libxl_domain_config_to_json(ctx, &d_config);
+ if (!config_c) {
+ fprintf(stderr, "unable to convert config file to JSON\n");
exit(2);
}
+ *config_data_r = (uint8_t *)config_c;
+ *config_len_r = strlen(config_c) + 1; /* including trailing '\0' */
+
+ libxl_domain_config_dispose(&d_config);
}
static void save_domain_core_writeconfig(int fd, const char *source,
@@ -3418,6 +3443,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
u32buf.u32 = config_len;
ADD_OPTDATA(u32buf.b, 4);
ADD_OPTDATA(config_data, config_len);
+ if (config_len)
+ hdr.mandatory_flags |= XL_MANDATORY_FLAG_JSON;
/* that's the optional data */
@@ -4427,6 +4454,9 @@ int main_config_update(int argc, char **argv)
exit(1);
}
+ fprintf(stderr, "WARN: xl now has better capability to manage domain configuration, "
+ "avoid using this command when possible\n");
+
domid = find_domain(argv[1]);
argc--; argv++;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 7b7fa92..06d1402 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -38,7 +38,9 @@ struct cmd_spec cmd_table[] = {
{ "config-update",
&main_config_update, 1, 1,
"Update a running domain's saved configuration, used when rebuilding "
- "the domain after reboot",
+ "the domain after reboot.\n"
+ "WARN: xl now has better capability to manage domain configuration, "
+ "avoid using this command when possible",
"<Domain> <ConfigFile> [options] [vars]",
"-h Print this help.\n"
"-f FILE, --defconfig=FILE\n Use the given configuration file.\n"
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 15/15] xl: long output of "list" command now contains Dom0 information
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
` (13 preceding siblings ...)
2014-09-04 22:43 ` [PATCH v3 14/15] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
@ 2014-09-04 22:43 ` Wei Liu
14 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-04 22:43 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
As we've already generated a JSON config for Dom0, print that out when
requested.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0702752..cc487b2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3186,9 +3186,6 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
for (i = 0; i < nb_domain; i++) {
libxl_domain_config_init(&d_config);
- /* no detailed info available on dom0 */
- if (info[i].domid == 0)
- continue;
rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
if (rc)
continue;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 08/15] libxl: introduce helper to initialise Dom0
2014-09-04 22:43 ` [PATCH v3 08/15] libxl: introduce helper to initialise Dom0 Wei Liu
@ 2014-09-05 13:22 ` Wei Liu
2014-09-09 11:03 ` Wei Liu
2014-09-09 11:16 ` Ian Campbell
1 sibling, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-05 13:22 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
On Thu, Sep 04, 2014 at 11:43:14PM +0100, Wei Liu wrote:
> This small helper is responsible for generating Dom0 JSON config
> stub and writing Dom0 xenstore entries. This helpers subsumes two calls
> to xenstore-write in xencommons script.
>
> Dom0 UUID is intentionally left untouched, so it is always all
> zeros. This makes sure that we don't leak Dom0 stubs across rebooting.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> ---
> change in v3:
> move to libxl directory
> ---
> .gitignore | 1 +
> tools/hotplug/Linux/init.d/xencommons.in.in | 5 +-
> tools/libxl/Makefile | 10 ++-
> tools/libxl/xen-init-dom0.c | 120 +++++++++++++++++++++++++++
> 4 files changed, 132 insertions(+), 4 deletions(-)
> create mode 100644 tools/libxl/xen-init-dom0.c
>
> diff --git a/.gitignore b/.gitignore
> index 6d725aa..8e34b85 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -313,6 +313,7 @@ tools/libxl/testidl.c
> tools/libxl/*.pyc
> tools/libxl/libxl-save-helper
> tools/libxl/test_timedereg
> +tools/libxl/xen-init-dom0
> tools/blktap2/control/tap-ctl
> tools/firmware/etherboot/eb-roms.h
> tools/firmware/etherboot/gpxe-git-snapshot.tar.gz
> diff --git a/tools/hotplug/Linux/init.d/xencommons.in.in b/tools/hotplug/Linux/init.d/xencommons.in.in
> index b311bb8..1d860d9 100644
> --- a/tools/hotplug/Linux/init.d/xencommons.in.in
> +++ b/tools/hotplug/Linux/init.d/xencommons.in.in
I just noticed I forgot to modify corresponding scripts for FreeBSD and
NetBSD. I will fix this either with another patch or in next version.
Wei.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 03/15] libxl: properly lock userdata store
2014-09-04 22:43 ` [PATCH v3 03/15] libxl: properly lock " Wei Liu
@ 2014-09-09 10:52 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 10:52 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> Originally libxl user data store didn't have lock at all. There could be
> such race condition as mentioned by Ian Jackson:
>
> Task 1 Task 2
> Creating the domain Trying to shut down
>
> actually create domain
> observe domid
> start domain destruction
> delete all userdata
> destroy domain
> store the userdata
> *** forbidden state created: userdata exists but domain doesn't
> *** userdata has been leaked
> [ would now bomb out ]
>
> This patch adds in proper locking to libxl user data store. The lock is
> associated with a specific domain (i.e. a per-domain lock).
>
> As for locking hierachy, we first take CTX lock (which is implemented
> with pthread recursive mutex so even if the application has taken it
> we're fine), then take the file lock. These locks are released in
> reversed order.
>
> A new libxl error code ERROR_LOCK_FAIL is introduced to describe failure
> to acquire locks.
>
> Also factor out libxl__userdata_{retrieve,store}, so that other
> functions that already hold the lock can call them to manipulate
> user data.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 07/15] libxl: disallow attaching the same device more than once
2014-09-04 22:43 ` [PATCH v3 07/15] libxl: disallow attaching the same device more than once Wei Liu
@ 2014-09-09 10:56 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 10:56 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> Originally the code allowed users to attach the same device more than
> once. It just stupidly overwrites xenstore entries. This is bogus as
> frontend will be very confused.
>
> Introduce a helper function to check if the device to be written to
> xenstore already exists. A new error code is also introduced.
>
> The check and add are within one xs transaction.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
But, it would be nice to send a followup which reduced the amount of
repetition on those transaction loops, apart from the disk one they are
all basically identical AFAICT.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 08/15] libxl: introduce helper to initialise Dom0
2014-09-05 13:22 ` Wei Liu
@ 2014-09-09 11:03 ` Wei Liu
0 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-09 11:03 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
This incremental patch fixes the issue
---8<---
>From a72585eb8e57de1555c7b60a4740759a5b9dfd8f Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 9 Sep 2014 12:01:15 +0100
Subject: [PATCH] hotplug script: subsume xenstore-write in *BSD scripts
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/hotplug/FreeBSD/rc.d/xencommons | 5 ++---
tools/hotplug/NetBSD/rc.d/xencommons | 5 ++---
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/tools/hotplug/FreeBSD/rc.d/xencommons b/tools/hotplug/FreeBSD/rc.d/xencommons
index a797016..2d9034c 100644
--- a/tools/hotplug/FreeBSD/rc.d/xencommons
+++ b/tools/hotplug/FreeBSD/rc.d/xencommons
@@ -64,9 +64,8 @@ xen_startcmd()
printf "\n"
- printf "Setting domain 0 name and domid.\n"
- ${BINDIR}/xenstore-write "/local/domain/0/name" "Domain-0"
- ${BINDIR}/xenstore-write "/local/domain/0/domid" 0
+ printf "Setting domain 0 name, domid and JSON config...\n"
+ ${PRIVATE_BINDIR}/xen-init-dom0
}
xen_stop()
diff --git a/tools/hotplug/NetBSD/rc.d/xencommons b/tools/hotplug/NetBSD/rc.d/xencommons
index b1c3531..7ac87e2 100644
--- a/tools/hotplug/NetBSD/rc.d/xencommons
+++ b/tools/hotplug/NetBSD/rc.d/xencommons
@@ -68,9 +68,8 @@ xen_startcmd()
printf "\n"
- printf "Setting domain 0 name and domid.\n"
- ${BINDIR}/xenstore-write "/local/domain/0/name" "Domain-0"
- ${BINDIR}/xenstore-write "/local/domain/0/domid" 0
+ printf "Setting domain 0 name, domid and JSON config...\n"
+ ${PRIVATE_BINDIR}/xen-init-dom0
}
xen_stop()
--
1.7.10.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device
2014-09-04 22:43 ` [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device Wei Liu
@ 2014-09-09 11:11 ` Ian Campbell
2014-09-09 11:23 ` Ian Campbell
2014-09-09 13:37 ` Wei Liu
0 siblings, 2 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:11 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> Please see comment in libxl_internal.h for correctness proof.
I'm just going to comment on that on this first pass.
> + /* If we need to udpate JSON config */
"update"
> + bool update_json;
> };
>
> /*
> @@ -2271,6 +2273,78 @@ struct libxl__multidev {
> *
> * Once finished, aodev->callback will be executed.
> */
> +/*
> + * As of Xen 4.5 we maintain various infomation, including hotplug
> + * device information, in JSON files, so that we can use this JSON
> + * file as a template to reconstruct domain configuration.
> + *
> + * In essnse there are now two views of device state, one is xenstore,
"essence"
> + * the other is JSON file. We use xenstore as primary reference.
> + *
> + * Here we maintain one invariant: every device in xenstore must have
> + * an entry in JSON file.
> + *
> + * All device hotplug routines should comply to following pattern:
> + * lock json config (json_lock)
> + * read json config
> + * update in-memory json config with new entry, rewrite
> + * if there's stale entry
Does "rewrite" here mean back to disk, or just the in-memory copy? I
think it is important for the protocol that it is the in-memory one
only.
I think you can just say "replacing any stale entry".
> + * for loop -- xs transaction
> + * open xs transaction
> + * check device existence, abort if it exists
> + * write in-memory json config to disk
> + * commit xs transaction
> + * end for loop
> + * unlock json config
> + *
> + * Device removal routines are not touched.
> + *
> + * Here is the proof that we always maintain that invariant and we
> + * don't leak files during interaction of hotplug thread and other
> + * threads / processes.
> + *
> + * # Safe against parallel add
> + *
> + * When another thread / process tries to add same device, it's
> + * blocked by json_lock. The loser of two threads will bail at
> + * existence check, so that we don't overwrite anything.
> + *
> + * # Safe against domain destruction
> + *
> + * When another thread / process tries to destroy domain, it's blocked
> + * by json_lock. If domain destruction thread is loser, it deletes
> + * every userdata file after it requires the lock. If hotplug thread
> + * is loser, it bails at acquiring lock, no device is added. Either
> + * way, no file is leaked.
I don't follow this one.
For the destructor loses case I think all you need to say is that the
json lock prevents the destruction process from removing the userdata
while the add is ongoing, the reference to deleting userdata after it
acquires the lock is just confusing.
In the "hotplug thread loses" case I think you should explain why it
bails (the existence check I suppose).
> + * # Safe against parallel removal
> + *
> + * When another thread / process tries to remove a device, it's _NOT_
> + * blocked by json_lock, but xenstore transaction can help maintain
> + * invariant. The removal threads either a) sees that device in
> + * xenstore, b) doesn't see that device in xenstore.
> + *
> + * In a), it sees that device in xenstore. At that point hotplug is
> + * already finished (both JSON and xenstore change committed). So that
> + * device can be safely removed. JSON entry is left untouched and
> + * becomes stale, but this is a valid state -- next time when a
> + * device with same identifier gets added, the stale entry gets
> + * overwritten.
> + *
> + * In b), it doesn't see that device in xenstore, but it will commence
> + * anyway. Eventually a force removal is initiated, which will forcely
forcibly.
> + * remove xenstore entry.
> + *
> + * If hotplug threads creates xenstore entry (therefore JSON entry as
> + * well) before force removal, that xenstore entry is removed. We're
> + * left with JSON stale entry but not xenstore entry, which is a valid
> + * state.
> + *
> + * If hotplug thread has not created xenstore entry when the removal
> + * is committed, we're obviously safe. Hotplug thread will add in
> + * xenstore entry afterwards. We have both JSON and xenstore entry,
> + * it's a valid state.
> + */
Despite my concerns with the description of one case I think it is good
enough that I can go look at the code now.
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 08/15] libxl: introduce helper to initialise Dom0
2014-09-04 22:43 ` [PATCH v3 08/15] libxl: introduce helper to initialise Dom0 Wei Liu
2014-09-05 13:22 ` Wei Liu
@ 2014-09-09 11:16 ` Ian Campbell
2014-09-09 12:16 ` Ian Campbell
1 sibling, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:16 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> This small helper is responsible for generating Dom0 JSON config
> stub and writing Dom0 xenstore entries. This helpers subsumes two calls
> to xenstore-write in xencommons script.
>
> Dom0 UUID is intentionally left untouched, so it is always all
> zeros. This makes sure that we don't leak Dom0 stubs across rebooting.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
With the *BSD followup folded in:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(I'll do the folding on commit unless there is some other reason to
resend).
One comment for a potential future cleanup:
> + /* Sanity check: this program can only be run once. */
I suppose parallel invocations of the initscript are unlikely. But if we
cared about such things then some locking would be needed, perhaps in
the form of a xenstore transaction or a droppings in /var/lib/subsys.
But how harmful is running this again? Since the UUID is statically all
zeroes isn't it idemopotent?
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device
2014-09-09 11:11 ` Ian Campbell
@ 2014-09-09 11:23 ` Ian Campbell
2014-09-09 13:37 ` Wei Liu
1 sibling, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:23 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Tue, 2014-09-09 at 12:11 +0100, Ian Campbell wrote:
> Despite my concerns with the description of one case I think it is good
> enough that I can go look at the code now.
The code looked good, thanks.
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert"
2014-09-04 22:43 ` [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
@ 2014-09-09 11:30 ` Ian Campbell
2014-09-09 13:38 ` Wei Liu
2014-09-15 14:38 ` Wei Liu
0 siblings, 2 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:30 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> A "cdrom insert" is always processed as "eject" + "insert", with JSON
> config updated in between. So that we can know the correct state of
> CDROM later when we try to retrieve domain configuration: if xenstore is
> "empty", then CDROM is "empty"; otherwise use the information presented
> in JSON.
>
> Note that libxl_cd_insert doesn't care about the incarnation of "empty"
> state so I don't state explictly what is in xenstore.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> change in v3:
> clarify "empty" in commit log
I think you should drop the ""s from around empty, since it keeps making
me thing that it isn't really empty in some sense or otherwise special
that I don't understand.
> ---
> tools/libxl/libxl.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index fa7d5d5..1fdb2d8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2662,8 +2662,9 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
> return 0;
> }
>
> -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
> - const libxl_asyncop_how *ao_how)
> +static int libxl__cdrom_insert(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_disk *disk,
> + const libxl_asyncop_how *ao_how)
> {
> AO_CREATE(ctx, domid, ao_how);
> int num = 0, i;
> @@ -2774,6 +2775,68 @@ out:
> return AO_INPROGRESS;
> }
>
> +/*
> + * A "cdrom insert" is always processed as "eject" + "insert", with
> + * updating JSON in between. So that we can know the current state of
> + * CDROM later when we try to retrieve domain configuration: if
> + * xenstore is "empty", then CDROM is "empty"; otherwise use the image
> + * in JSON.
> + *
> + * Note that we don't actually care about the incarnation of "empty"
> + * state so we don't state explicitly what the content in xenstore
> + * should be.
> + *
> + * In order to maintain lock hierarchy, CTX lock is taken when
> + * entering this function.
If this is because the function takes the userdata lock then you should
say so.
> + lock = libxl__lock_domain_userdata(gc, domid);
> + if (!lock) {
> + rc = ERROR_LOCK_FAIL;
> + goto out;
> + }
> + rc = libxl__get_domain_configuration(gc, domid, &d_config);
> + if (rc) goto out;
> + DEVICE_ADD(disk, disks, domid, disk, COMPARE_DISK, &d_config);
> + rc = libxl__set_domain_configuration(gc, domid, &d_config);
> + if (rc) goto out;
> + libxl__unlock_domain_userdata(lock);
> +
> + rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
> +
> +out:
> + libxl_device_disk_dispose(&empty);
> + libxl_domain_config_dispose(&d_config);
This code doesn't seem to adhere to the protocol laid down in the
previous patch and I suspect it therefore isn't safe against parallel
eject/insert calls.
INSERTION A INSERTION B
takes lock
updates json
releases lock
takes lock
updates json
releases lock
adds to XS
adds to XS
Now the json == B and the xenstore == A.
I think you need to push the json update down into libxl__cdrom_insert
and do the say dance you do elsewhere, with an XS transaction +
existence check inside the userdata locked region.
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 11/15] libxl: refactor libxl_get_memory_target
2014-09-04 22:43 ` [PATCH v3 11/15] libxl: refactor libxl_get_memory_target Wei Liu
@ 2014-09-09 11:36 ` Ian Campbell
2014-09-09 11:39 ` Ian Campbell
0 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:36 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> +
> + if (!get_target)
> + *out_mem = max_memkb;
> + else
> + *out_mem = target_memkb;
Given that you are only going to return one or the other why do you go
to all the effort of reading both and converting them from string to int
etc?
Can't you just set the path to read from based on out_target at the top
(and maybe set a const char * what for logging purposes) and then the
rest of the function just deals with that doing a single conversion etc?
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 11/15] libxl: refactor libxl_get_memory_target
2014-09-09 11:36 ` Ian Campbell
@ 2014-09-09 11:39 ` Ian Campbell
2014-09-09 13:39 ` Wei Liu
0 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:39 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Tue, 2014-09-09 at 12:36 +0100, Ian Campbell wrote:
> On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> > +
> > + if (!get_target)
> > + *out_mem = max_memkb;
> > + else
> > + *out_mem = target_memkb;
>
> Given that you are only going to return one or the other why do you go
> to all the effort of reading both and converting them from string to int
> etc?
>
> Can't you just set the path to read from based on out_target at the top
> (and maybe set a const char * what for logging purposes) and then the
> rest of the function just deals with that doing a single conversion etc?
The caller in the next patch seems to want both values, so it calls this
function twice, meaning that currently it is reading and parsing both
values twice.
Depends on what other callers there are perhaps what you really want is
a single helper to return both values at the same time, which accepts
NULL for either of the output pointers.
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 12/15] libxl: introduce libxl_retrieve_domain_configuration
2014-09-04 22:43 ` [PATCH v3 12/15] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
@ 2014-09-09 11:41 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:41 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> Introduce a new public API to return domain configuration. This returned
> configuration can be used to rebuild a domain.
>
> Note that this configuration only describes the configuration necessary
> to reproduce the guest visible state and does not necessarily include
> specific decisions made by the toolstack regarding its current
> incarnation (e.g. disk backend) unless they were specified by the
> application when the domain was created.
>
> With this approach we can preserve what user has provided in the
> original configuration as well as valuable information from xenstore.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Aside from my concerns about the libxl__get_memory_target
interface/implementation this looks good.
Ian
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 13/15] libxl: introduce libxl_userdata_unlink
2014-09-04 22:43 ` [PATCH v3 13/15] libxl: introduce libxl_userdata_unlink Wei Liu
@ 2014-09-09 11:42 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:42 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> This will be used in later patch for xl to remove its "xl" userdata
> file.
>
> Both CTX lock and userdata lock are taken in this API. CTX lock is taken
> to maintain locking hierarchy, but it also has a side effect to protect
> against R-M-W by other threads. Userdata lock is used to protect against
> domain destruction.
>
> In general application should not rely on these internal locks to
> protect its own userdata files. It should deploys its own lock if it
> cares.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 14/15] xl: use libxl_retrieve_domain_configuration and JSON format
2014-09-04 22:43 ` [PATCH v3 14/15] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
@ 2014-09-09 11:44 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 11:44 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> @@ -4427,6 +4454,9 @@ int main_config_update(int argc, char **argv)
> exit(1);
> }
>
> + fprintf(stderr, "WARN: xl now has better capability to manage domain configuration, "
> + "avoid using this command when possible\n");
Everywhere else that I can see uses WARNING:
> + "WARN: xl now has better capability to manage domain configuration, "
Likewise.
With those fixed: Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 08/15] libxl: introduce helper to initialise Dom0
2014-09-09 11:16 ` Ian Campbell
@ 2014-09-09 12:16 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 12:16 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Tue, 2014-09-09 at 12:16 +0100, Ian Campbell wrote:
> On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> > This small helper is responsible for generating Dom0 JSON config
> > stub and writing Dom0 xenstore entries. This helpers subsumes two calls
> > to xenstore-write in xencommons script.
> >
> > Dom0 UUID is intentionally left untouched, so it is always all
> > zeros. This makes sure that we don't leak Dom0 stubs across rebooting.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> With the *BSD followup folded in:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> (I'll do the folding on commit unless there is some other reason to
> resend).
I've now applied up to and including this patch, including this folding.
Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device
2014-09-09 11:11 ` Ian Campbell
2014-09-09 11:23 ` Ian Campbell
@ 2014-09-09 13:37 ` Wei Liu
2014-09-09 13:41 ` Ian Campbell
1 sibling, 1 reply; 35+ messages in thread
From: Wei Liu @ 2014-09-09 13:37 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Tue, Sep 09, 2014 at 12:11:52PM +0100, Ian Campbell wrote:
[...]
> > + * All device hotplug routines should comply to following pattern:
> > + * lock json config (json_lock)
> > + * read json config
> > + * update in-memory json config with new entry, rewrite
> > + * if there's stale entry
>
> Does "rewrite" here mean back to disk, or just the in-memory copy? I
> think it is important for the protocol that it is the in-memory one
> only.
>
> I think you can just say "replacing any stale entry".
It means replacing the entry in in-memory copy.
I will rephrase this line.
>
> > + * for loop -- xs transaction
> > + * open xs transaction
> > + * check device existence, abort if it exists
> > + * write in-memory json config to disk
> > + * commit xs transaction
> > + * end for loop
> > + * unlock json config
> > + *
> > + * Device removal routines are not touched.
> > + *
> > + * Here is the proof that we always maintain that invariant and we
> > + * don't leak files during interaction of hotplug thread and other
> > + * threads / processes.
> > + *
> > + * # Safe against parallel add
> > + *
> > + * When another thread / process tries to add same device, it's
> > + * blocked by json_lock. The loser of two threads will bail at
> > + * existence check, so that we don't overwrite anything.
> > + *
> > + * # Safe against domain destruction
> > + *
> > + * When another thread / process tries to destroy domain, it's blocked
> > + * by json_lock. If domain destruction thread is loser, it deletes
> > + * every userdata file after it requires the lock. If hotplug thread
> > + * is loser, it bails at acquiring lock, no device is added. Either
> > + * way, no file is leaked.
>
> I don't follow this one.
>
> For the destructor loses case I think all you need to say is that the
> json lock prevents the destruction process from removing the userdata
> while the add is ongoing, the reference to deleting userdata after it
> acquires the lock is just confusing.
>
> In the "hotplug thread loses" case I think you should explain why it
> bails (the existence check I suppose).
>
How about this:
If the thread / process trying to destroy domain loses the rase, it's
blocked by json_lock. If the hotplug thread is loser, it bails at
acquiring lock because lock acquisition function checks existence of
the domain.
Re typos, I will fix them in next round.
Wei.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert"
2014-09-09 11:30 ` Ian Campbell
@ 2014-09-09 13:38 ` Wei Liu
2014-09-15 14:38 ` Wei Liu
1 sibling, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-09 13:38 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Tue, Sep 09, 2014 at 12:30:33PM +0100, Ian Campbell wrote:
> On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> > A "cdrom insert" is always processed as "eject" + "insert", with JSON
> > config updated in between. So that we can know the correct state of
> > CDROM later when we try to retrieve domain configuration: if xenstore is
> > "empty", then CDROM is "empty"; otherwise use the information presented
> > in JSON.
> >
> > Note that libxl_cd_insert doesn't care about the incarnation of "empty"
> > state so I don't state explictly what is in xenstore.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > change in v3:
> > clarify "empty" in commit log
>
> I think you should drop the ""s from around empty, since it keeps making
> me thing that it isn't really empty in some sense or otherwise special
> that I don't understand.
>
OK.
> > ---
> > tools/libxl/libxl.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 65 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index fa7d5d5..1fdb2d8 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -2662,8 +2662,9 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
> > return 0;
> > }
> >
> > -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
> > - const libxl_asyncop_how *ao_how)
> > +static int libxl__cdrom_insert(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_disk *disk,
> > + const libxl_asyncop_how *ao_how)
> > {
> > AO_CREATE(ctx, domid, ao_how);
> > int num = 0, i;
> > @@ -2774,6 +2775,68 @@ out:
> > return AO_INPROGRESS;
> > }
> >
> > +/*
> > + * A "cdrom insert" is always processed as "eject" + "insert", with
> > + * updating JSON in between. So that we can know the current state of
> > + * CDROM later when we try to retrieve domain configuration: if
> > + * xenstore is "empty", then CDROM is "empty"; otherwise use the image
> > + * in JSON.
> > + *
> > + * Note that we don't actually care about the incarnation of "empty"
> > + * state so we don't state explicitly what the content in xenstore
> > + * should be.
> > + *
> > + * In order to maintain lock hierarchy, CTX lock is taken when
> > + * entering this function.
>
> If this is because the function takes the userdata lock then you should
> say so.
>
OK.
> > + lock = libxl__lock_domain_userdata(gc, domid);
> > + if (!lock) {
> > + rc = ERROR_LOCK_FAIL;
> > + goto out;
> > + }
> > + rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > + if (rc) goto out;
> > + DEVICE_ADD(disk, disks, domid, disk, COMPARE_DISK, &d_config);
> > + rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > + if (rc) goto out;
> > + libxl__unlock_domain_userdata(lock);
> > +
> > + rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
> > +
> > +out:
> > + libxl_device_disk_dispose(&empty);
> > + libxl_domain_config_dispose(&d_config);
>
> This code doesn't seem to adhere to the protocol laid down in the
> previous patch and I suspect it therefore isn't safe against parallel
> eject/insert calls.
>
> INSERTION A INSERTION B
>
> takes lock
> updates json
> releases lock
>
> takes lock
> updates json
> releases lock
>
> adds to XS
>
> adds to XS
>
> Now the json == B and the xenstore == A.
>
> I think you need to push the json update down into libxl__cdrom_insert
> and do the say dance you do elsewhere, with an XS transaction +
> existence check inside the userdata locked region.
>
Yes you're right. I will fix this.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 11/15] libxl: refactor libxl_get_memory_target
2014-09-09 11:39 ` Ian Campbell
@ 2014-09-09 13:39 ` Wei Liu
0 siblings, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-09 13:39 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Tue, Sep 09, 2014 at 12:39:05PM +0100, Ian Campbell wrote:
> On Tue, 2014-09-09 at 12:36 +0100, Ian Campbell wrote:
> > On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:
> > > +
> > > + if (!get_target)
> > > + *out_mem = max_memkb;
> > > + else
> > > + *out_mem = target_memkb;
> >
> > Given that you are only going to return one or the other why do you go
> > to all the effort of reading both and converting them from string to int
> > etc?
> >
> > Can't you just set the path to read from based on out_target at the top
> > (and maybe set a const char * what for logging purposes) and then the
> > rest of the function just deals with that doing a single conversion etc?
>
> The caller in the next patch seems to want both values, so it calls this
> function twice, meaning that currently it is reading and parsing both
> values twice.
>
> Depends on what other callers there are perhaps what you really want is
> a single helper to return both values at the same time, which accepts
> NULL for either of the output pointers.
>
OK, I will rewrite it to return both values at the same time.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device
2014-09-09 13:37 ` Wei Liu
@ 2014-09-09 13:41 ` Ian Campbell
0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-09-09 13:41 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Tue, 2014-09-09 at 14:37 +0100, Wei Liu wrote:
> How about this:
>
> If the thread / process trying to destroy domain loses the rase, it's
> blocked by json_lock. If the hotplug thread is loser, it bails at
> acquiring lock because lock acquisition function checks existence of
> the domain.
Sounds good. (typo: "race" not rase)
>
> Re typos, I will fix them in next round.
>
> Wei.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert"
2014-09-09 11:30 ` Ian Campbell
2014-09-09 13:38 ` Wei Liu
@ 2014-09-15 14:38 ` Wei Liu
1 sibling, 0 replies; 35+ messages in thread
From: Wei Liu @ 2014-09-15 14:38 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Tue, Sep 09, 2014 at 12:30:33PM +0100, Ian Campbell wrote:
[...]
> > + lock = libxl__lock_domain_userdata(gc, domid);
> > + if (!lock) {
> > + rc = ERROR_LOCK_FAIL;
> > + goto out;
> > + }
> > + rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > + if (rc) goto out;
> > + DEVICE_ADD(disk, disks, domid, disk, COMPARE_DISK, &d_config);
> > + rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > + if (rc) goto out;
> > + libxl__unlock_domain_userdata(lock);
> > +
> > + rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
> > +
> > +out:
> > + libxl_device_disk_dispose(&empty);
> > + libxl_domain_config_dispose(&d_config);
>
> This code doesn't seem to adhere to the protocol laid down in the
> previous patch and I suspect it therefore isn't safe against parallel
> eject/insert calls.
>
> INSERTION A INSERTION B
>
> takes lock
> updates json
> releases lock
>
> takes lock
> updates json
> releases lock
>
> adds to XS
>
> adds to XS
>
> Now the json == B and the xenstore == A.
>
> I think you need to push the json update down into libxl__cdrom_insert
> and do the say dance you do elsewhere, with an XS transaction +
> existence check inside the userdata locked region.
>
After discussing with Ian J IRL I come up with an approach which I will
detail below.
There should not be an existence check because we're not adding a new
device, we're just updating some nodes. We would still need to stick
with the approach, to make "cdrom insert" "eject + insert".
Here are some background information:
Back in June Ian J and I had a discussion on how to deal with removable
disk. He said it was impossible to convert xenstore backend information
back to libxl structure as there's information lost during transaction
from libxl structure to xenstore entry. One being that backend_domname
and backend_domid. User can specify either backend_domname or
backend_domid in diskspec (either specifying both is a bug or we need to
make one take precedence over the other). I think the same theory
applies to other devices as well. Even if we can convert evething back
now, it doesn't necessary mean we can in the future. This means we
basically need to rely on JSON entries to be the final version we want
to use to reconstruct a domain.
For a removable device, currently user can just "swap" media in drive
from one to another (of course there can be other changes behind the
scene that might affect other nodes in xenstore). Plus the fact that we
update JSON first then commit xenstore changes, if xenstore commit fails
the media in JSON is not the same one in xenstore, and the one in
xenstore is the domain actually sees. This becomes a corner case.
We cannot use JSON entry as template anymore.
To fix this corner case, we need to introduce an intermediate empty
state. Transition from original state to empty state should not follow
the usual protocol (JSON then xenstore). Instead we only update xenstore
but not JSON, then in the following update which writes in the disk user
supplied we follow protocol.
lock json
write empty to xenstore
// usual protocol follows
for () {
update JSON
write disk to xenstore
}
unlock json
So we ensure that if the second xenstore write ever fails we still have
a known state in xenstore (which is empty state). Without the empty
state, we can end up with a situation that JSON and xenstore disagree
with each other and we don't know which one is correct.
In later retrieval, read "disk" from xenstore:
if (disk is removable && disk is empty)
disk is empty
else
use JSON version
Does this make sense?
Wei.
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2014-09-15 14:38 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-04 22:43 [PATCH v3 00/15] libxl: synchronise domain configuration Wei Liu
2014-09-04 22:43 ` [PATCH v3 01/15] libxl: make userdata_path libxl internal function Wei Liu
2014-09-04 22:43 ` [PATCH v3 02/15] libxl: functions to lock / unlock libxl userdata store Wei Liu
2014-09-04 22:43 ` [PATCH v3 03/15] libxl: properly lock " Wei Liu
2014-09-09 10:52 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 04/15] libxl: libxl-json format and internal functions to get / set it Wei Liu
2014-09-04 22:43 ` [PATCH v3 05/15] libxl: store a copy of configuration when creating domain Wei Liu
2014-09-04 22:43 ` [PATCH v3 06/15] libxl: introduce libxl__device_from_pcidev Wei Liu
2014-09-04 22:43 ` [PATCH v3 07/15] libxl: disallow attaching the same device more than once Wei Liu
2014-09-09 10:56 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 08/15] libxl: introduce helper to initialise Dom0 Wei Liu
2014-09-05 13:22 ` Wei Liu
2014-09-09 11:03 ` Wei Liu
2014-09-09 11:16 ` Ian Campbell
2014-09-09 12:16 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device Wei Liu
2014-09-09 11:11 ` Ian Campbell
2014-09-09 11:23 ` Ian Campbell
2014-09-09 13:37 ` Wei Liu
2014-09-09 13:41 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
2014-09-09 11:30 ` Ian Campbell
2014-09-09 13:38 ` Wei Liu
2014-09-15 14:38 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 11/15] libxl: refactor libxl_get_memory_target Wei Liu
2014-09-09 11:36 ` Ian Campbell
2014-09-09 11:39 ` Ian Campbell
2014-09-09 13:39 ` Wei Liu
2014-09-04 22:43 ` [PATCH v3 12/15] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
2014-09-09 11:41 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 13/15] libxl: introduce libxl_userdata_unlink Wei Liu
2014-09-09 11:42 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 14/15] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
2014-09-09 11:44 ` Ian Campbell
2014-09-04 22:43 ` [PATCH v3 15/15] xl: long output of "list" command now contains Dom0 information Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).