* [PATCH v2 01/18] libxl: libxl error code is signed integer
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-26 21:15 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 02/18] libxl: make userdata_path libxl internal function Wei Liu
` (17 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Fix two occurences of "unsigned int rc".
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3526539..9ac2c3d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1883,7 +1883,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
flexarray_t *front;
flexarray_t *back;
libxl__device *device;
- unsigned int rc;
+ int rc;
rc = libxl__device_vtpm_setdefault(gc, vtpm);
if (rc) goto out;
@@ -2988,7 +2988,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
flexarray_t *front;
flexarray_t *back;
libxl__device *device;
- unsigned int rc;
+ int rc;
rc = libxl__device_nic_setdefault(gc, nic, domid);
if (rc) goto out;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH v2 02/18] libxl: make userdata_path libxl internal function
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
2014-07-30 18:23 ` [PATCH v2 01/18] libxl: libxl error code is signed integer Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-26 21:16 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store Wei Liu
` (16 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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>
---
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 83eb29a..a41e8fd 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1762,9 +1762,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;
@@ -1799,7 +1799,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;
@@ -1830,7 +1830,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;
@@ -1841,7 +1841,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;
@@ -1892,7 +1892,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 beb052e..4b2e94f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -991,6 +991,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] 70+ messages in thread* [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
2014-07-30 18:23 ` [PATCH v2 01/18] libxl: libxl error code is signed integer Wei Liu
2014-07-30 18:23 ` [PATCH v2 02/18] libxl: make userdata_path libxl internal function Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-26 21:21 ` Ian Campbell
2014-09-03 12:40 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 04/18] libxl: properly lock " Wei Liu
` (15 subsequent siblings)
18 siblings, 2 replies; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
The term "domain data" means libxl's knowledge of a domain, which
includes but not limit to domain configuration. A new "libxl-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.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.h | 2 ++
tools/libxl/libxl_internal.c | 69 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 5 +++
3 files changed, 76 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5ae6532..82f28bd 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1180,6 +1180,8 @@ 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
+ * "libxl-lock" lock file to protect domain data in libxl. It's 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 81f8985..7dd84b6 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_data(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, "libxl-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,
+ "unexpcted 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_data(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 4b2e94f..60316ff 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>
@@ -3222,6 +3223,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_data(libxl__gc *gc, uint32_t domid);
+void libxl__unlock_domain_data(libxl__carefd *lock_carefd);
+
#endif
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store
2014-07-30 18:23 ` [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store Wei Liu
@ 2014-08-26 21:21 ` Ian Campbell
2014-09-03 14:27 ` Wei Liu
2014-09-03 12:40 ` Ian Campbell
1 sibling, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-26 21:21 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> The term "domain data" means libxl's knowledge of a domain, which
> includes but not limit to domain configuration. A new "libxl-lock" entry
"is not limited to"
> Portability note: this lock utilises flock(2) so a proper implementation
> of flock(2) is required.
I suppose Linux provides this, do you know about BSD? (IOW, how much of
a problem is this in reality)
> + * "libxl-lock" lock file to protect domain data in libxl. It's per-domain
"It's a per-domain..." or just "Per-domain...".
> + default:
> + /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
> + LOGE(ERROR,
> + "unexpcted error while trying to lock %s, fd=%d, errno=%d",
"unexpected"
Other than those nits: Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store
2014-08-26 21:21 ` Ian Campbell
@ 2014-09-03 14:27 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-09-03 14:27 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Tue, Aug 26, 2014 at 10:21:34PM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > The term "domain data" means libxl's knowledge of a domain, which
> > includes but not limit to domain configuration. A new "libxl-lock" entry
>
> "is not limited to"
>
> > Portability note: this lock utilises flock(2) so a proper implementation
> > of flock(2) is required.
>
> I suppose Linux provides this, do you know about BSD? (IOW, how much of
> a problem is this in reality)
>
Flock first appeared in 4.2BSD so I think by now the major BSDs we care
about should already have proper implementation.
Wei.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store
2014-07-30 18:23 ` [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store Wei Liu
2014-08-26 21:21 ` Ian Campbell
@ 2014-09-03 12:40 ` Ian Campbell
2014-09-03 15:09 ` Wei Liu
1 sibling, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 12:40 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> The term "domain data" means libxl's knowledge of a domain, which
> includes but not limit to domain configuration.
I think this may be the root of my confusion about the locking strategy
you are deploying in this series. I took "domain configuration" here to
mean exactly the libxl-json userdata.
It seems like you are also including any application specific userdata
associated with the domain in "libxl's knowledge of a domain". Is that
right?
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store
2014-09-03 12:40 ` Ian Campbell
@ 2014-09-03 15:09 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-09-03 15:09 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Sep 03, 2014 at 01:40:42PM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > The term "domain data" means libxl's knowledge of a domain, which
> > includes but not limit to domain configuration.
>
> I think this may be the root of my confusion about the locking strategy
> you are deploying in this series. I took "domain configuration" here to
> mean exactly the libxl-json userdata.
>
> It seems like you are also including any application specific userdata
> associated with the domain in "libxl's knowledge of a domain". Is that
> right?
>
I've mentioned this in other email but I reiterate the important bit
here for the record.
Yes, this "libxl's knowledge of a domain". The registry entry will be
changed to domain-userdata-lock.
This lock is used to protect against domain destruction.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 04/18] libxl: properly lock user data store
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (2 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 03/18] libxl: functions to lock / unlock domain data in libxl user data store Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-26 21:24 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 05/18] libxl: libxl-json format and internal functions to get / set it Wei Liu
` (14 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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>
---
tools/libxl/libxl_dom.c | 81 +++++++++++++++++++++++++++++++++++-------
tools/libxl/libxl_internal.h | 9 +++++
tools/libxl/libxl_types.idl | 1 +
3 files changed, 79 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a41e8fd..3b9eade 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1798,6 +1798,12 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid)
const char *pattern;
glob_t gl;
int r, i;
+ libxl__carefd *lock;
+
+ CTX_LOCK;
+ lock = libxl__lock_domain_data(gc, domid);
+ if (!lock)
+ goto out;
pattern = libxl__userdata_path(gc, domid, "*", "?");
if (!pattern)
@@ -1817,14 +1823,15 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid)
}
globfree(&gl);
out:
+ if (lock) libxl__unlock_domain_data(lock);
+ CTX_UNLOCK;
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;
@@ -1853,7 +1860,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) {
@@ -1875,18 +1882,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_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__userdata_store(gc, domid, userdata_userid,
+ data, datalen);
+
+ libxl__unlock_domain_data(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;
@@ -1898,13 +1929,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;
@@ -1913,7 +1944,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_data(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_data(lock);
out:
+ CTX_UNLOCK;
GC_FREE;
return rc;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 60316ff..03ed7ec 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -996,6 +996,15 @@ _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 data store lock before calling
+ * libxl__userdata_{retrieve,store}
+ */
+_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 a412f9c..17b4453 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] 70+ messages in thread* [PATCH v2 05/18] libxl: libxl-json format and internal functions to get / set it
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (3 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 04/18] libxl: properly lock " Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-07-30 18:23 ` [PATCH v2 06/18] libxl: store a copy of configuration when creating domain Wei Liu
` (13 subsequent siblings)
18 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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>
---
tools/libxl/libxl.h | 3 +++
tools/libxl/libxl_internal.c | 56 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 11 +++++++++
tools/libxl/libxl_types.idl | 1 +
4 files changed, 71 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 82f28bd..11e391d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1182,6 +1182,9 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
* http://libvirt.org/formatdomain.html
* "libxl-lock" lock file to protect domain data in libxl. It's 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.
*
* 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 7dd84b6..052315b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -450,6 +450,62 @@ void libxl__unlock_domain_data(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 03ed7ec..52d4d6c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3236,6 +3236,17 @@ int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
libxl__carefd *libxl__lock_domain_data(libxl__gc *gc, uint32_t domid);
void libxl__unlock_domain_data(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 17b4453..d0a373b 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] 70+ messages in thread* [PATCH v2 06/18] libxl: store a copy of configuration when creating domain
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (4 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 05/18] libxl: libxl-json format and internal functions to get / set it Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 1:34 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 07/18] libxl: separate device add/rm complete callbacks Wei Liu
` (12 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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 has 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>
---
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 0686f96..c997d7a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1350,10 +1350,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_data(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_data(lock);
+ }
+ }
+
+ libxl_domain_config_dispose(d_config_saved);
+
if (rc) {
if (dcs->guest_domid) {
dcs->dds.ao = ao;
@@ -1404,6 +1424,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 052315b..a002899 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 52d4d6c..02708c7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2779,6 +2779,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;
@@ -3247,6 +3248,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] 70+ messages in thread* Re: [PATCH v2 06/18] libxl: store a copy of configuration when creating domain
2014-07-30 18:23 ` [PATCH v2 06/18] libxl: store a copy of configuration when creating domain Wei Liu
@ 2014-08-27 1:34 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 1:34 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> 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 has a window that the domain exists but has no JSON config in
"therefore have a window".
> 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 0686f96..c997d7a 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1350,10 +1350,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_data(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_data(lock);
> + }
> + }
> +
> + libxl_domain_config_dispose(d_config_saved);
> +
> if (rc) {
> if (dcs->guest_domid) {
> dcs->dds.ao = ao;
> @@ -1404,6 +1424,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 052315b..a002899 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 52d4d6c..02708c7 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2779,6 +2779,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;
> @@ -3247,6 +3248,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
>
> /*
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 07/18] libxl: separate device add/rm complete callbacks
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (5 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 06/18] libxl: store a copy of configuration when creating domain Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 1:41 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 08/18] libxl: introduce libxl__device_from_pcidev Wei Liu
` (11 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
This is in preparation for device hotplug / unplug configuration
synchronisation. No functional change introduced. This change is
necessary because rm is type specific, using a single callback cannot
meet our need.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 54 +++++++++++++++++++++++++++++++++++++++------------
1 file changed, 42 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9ac2c3d..01dffa0 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1798,22 +1798,26 @@ out:
}
/******************************************************************************/
+#define DEVICE_AO_FAILED_MSG \
+ do { \
+ if (aodev->dev) { \
+ LOG(ERROR, "unable to %s %s with id %u", \
+ libxl__device_action_to_string(aodev->action), \
+ libxl__device_kind_to_string(aodev->dev->kind), \
+ aodev->dev->devid); \
+ } else { \
+ LOG(ERROR, "unable to %s device", \
+ libxl__device_action_to_string(aodev->action)); \
+ } \
+ } while (0)
/* generic callback for devices that only need to set ao_complete */
-static void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
+static void device_add_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
if (aodev->rc) {
- if (aodev->dev) {
- LOG(ERROR, "unable to %s %s with id %u",
- libxl__device_action_to_string(aodev->action),
- libxl__device_kind_to_string(aodev->dev->kind),
- aodev->dev->devid);
- } else {
- LOG(ERROR, "unable to %s device",
- libxl__device_action_to_string(aodev->action));
- }
+ DEVICE_AO_FAILED_MSG;
goto out;
}
@@ -3537,6 +3541,32 @@ out:
}
/******************************************************************************/
+#define DEFINE_DEVICE_RM_AOCOMPLETE(type) \
+ static void device_##type##_rm_aocomplete(libxl__egc *egc, \
+ libxl__ao_device *aodev) \
+ { \
+ STATE_AO_GC(aodev->ao); \
+ if (aodev->rc) { \
+ DEVICE_AO_FAILED_MSG; \
+ goto out; \
+ } \
+ \
+ out: \
+ libxl__ao_complete(egc, ao, aodev->rc); \
+ }
+
+/*
+ * device_vtpm_rm_aocomplete
+ * device_nic_rm_aocomplete
+ * device_disk_rm_aocomplete
+ * device_vfb_rm_aocomplete
+ * device_vkb_rm_aocomplete
+ */
+DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
+DEFINE_DEVICE_RM_AOCOMPLETE(nic);
+DEFINE_DEVICE_RM_AOCOMPLETE(disk);
+DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
+DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
/* Macro for defining device remove/destroy functions in a compact way */
/* The following functions are defined:
@@ -3569,7 +3599,7 @@ out:
libxl__prepare_ao_device(ao, aodev); \
aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
aodev->dev = device; \
- aodev->callback = device_addrm_aocomplete; \
+ aodev->callback = device_##type##_rm_aocomplete; \
aodev->force = f; \
libxl__initiate_device_remove(egc, aodev); \
\
@@ -3622,7 +3652,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
\
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
- aodev->callback = device_addrm_aocomplete; \
+ aodev->callback = device_add_aocomplete; \
libxl__device_##type##_add(egc, domid, type, aodev); \
\
return AO_INPROGRESS; \
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH v2 07/18] libxl: separate device add/rm complete callbacks
2014-07-30 18:23 ` [PATCH v2 07/18] libxl: separate device add/rm complete callbacks Wei Liu
@ 2014-08-27 1:41 ` Ian Campbell
2014-08-28 10:34 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 1:41 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> This is in preparation for device hotplug / unplug configuration
> synchronisation. No functional change introduced. This change is
> necessary because rm is type specific, using a single callback cannot
Not "is" but "is going to become" I think?
> meet our need.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/libxl.c | 54 +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9ac2c3d..01dffa0 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1798,22 +1798,26 @@ out:
> }
>
> /******************************************************************************/
> +#define DEVICE_AO_FAILED_MSG \
I don't see why this couldn't be a static log_ao_failure message, but
since it is used like a function it should be defined/called as
DEVICE_AO_FAILED_MSG() so it looks normal. I think it should just be a
function though.
> @@ -3537,6 +3541,32 @@ out:
> }
>
> /******************************************************************************/
> +#define DEFINE_DEVICE_RM_AOCOMPLETE(type) \
> + static void device_##type##_rm_aocomplete(libxl__egc *egc, \
> + libxl__ao_device *aodev) \
> + { \
> + STATE_AO_GC(aodev->ao); \
> + if (aodev->rc) { \
> + DEVICE_AO_FAILED_MSG; \
> + goto out; \
> + } \
> + \
> + out: \
Perhaps you are going to add stuff between } and out: in a future patch,
but if not then the goto+label are a bit pointless (I see now that you
inherited that from the original code, so feel free to leave it if you
prefer).
> + libxl__ao_complete(egc, ao, aodev->rc); \
> + }
> +
> +/*
> + * device_vtpm_rm_aocomplete
> + * device_nic_rm_aocomplete
> + * device_disk_rm_aocomplete
> + * device_vfb_rm_aocomplete
> + * device_vkb_rm_aocomplete
> + */
> +DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
> +DEFINE_DEVICE_RM_AOCOMPLETE(nic);
> +DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> +DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
> +DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
I was going to suggest merging this with DEFINE_DEVICE_REMOVE but that
is used twice for each device so that won't work. But perhaps consider
adding these in the same blocks as the usage of DEFINE_DEVICE_REMOVE?
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 07/18] libxl: separate device add/rm complete callbacks
2014-08-27 1:41 ` Ian Campbell
@ 2014-08-28 10:34 ` Wei Liu
2014-09-03 11:53 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 10:34 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 02:41:55AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > This is in preparation for device hotplug / unplug configuration
> > synchronisation. No functional change introduced. This change is
> > necessary because rm is type specific, using a single callback cannot
>
> Not "is" but "is going to become" I think?
>
Yes, you're right.
> > meet our need.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > tools/libxl/libxl.c | 54 +++++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 42 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 9ac2c3d..01dffa0 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1798,22 +1798,26 @@ out:
> > }
> >
> > /******************************************************************************/
> > +#define DEVICE_AO_FAILED_MSG \
>
> I don't see why this couldn't be a static log_ao_failure message, but
> since it is used like a function it should be defined/called as
> DEVICE_AO_FAILED_MSG() so it looks normal. I think it should just be a
> function though.
OK, I will make it function.
> > @@ -3537,6 +3541,32 @@ out:
> > }
> >
> > /******************************************************************************/
> > +#define DEFINE_DEVICE_RM_AOCOMPLETE(type) \
> > + static void device_##type##_rm_aocomplete(libxl__egc *egc, \
> > + libxl__ao_device *aodev) \
> > + { \
> > + STATE_AO_GC(aodev->ao); \
> > + if (aodev->rc) { \
> > + DEVICE_AO_FAILED_MSG; \
> > + goto out; \
> > + } \
> > + \
> > + out: \
>
> Perhaps you are going to add stuff between } and out: in a future patch,
> but if not then the goto+label are a bit pointless (I see now that you
> inherited that from the original code, so feel free to leave it if you
> prefer).
>
I will leave it as it is, in case we have other things to add in between
goto and out label.
> > + libxl__ao_complete(egc, ao, aodev->rc); \
> > + }
> > +
> > +/*
> > + * device_vtpm_rm_aocomplete
> > + * device_nic_rm_aocomplete
> > + * device_disk_rm_aocomplete
> > + * device_vfb_rm_aocomplete
> > + * device_vkb_rm_aocomplete
> > + */
> > +DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(nic);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
>
> I was going to suggest merging this with DEFINE_DEVICE_REMOVE but that
> is used twice for each device so that won't work. But perhaps consider
> adding these in the same blocks as the usage of DEFINE_DEVICE_REMOVE?
>
They're already placed right before DEFINE_DEVICE_RM macro. You
requested this in your last review so I've done it already. ;-)
Wei.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 07/18] libxl: separate device add/rm complete callbacks
2014-08-28 10:34 ` Wei Liu
@ 2014-09-03 11:53 ` Ian Campbell
2014-09-03 11:55 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 11:53 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 11:34 +0100, Wei Liu wrote:
> > > + libxl__ao_complete(egc, ao, aodev->rc); \
> > > + }
> > > +
> > > +/*
> > > + * device_vtpm_rm_aocomplete
> > > + * device_nic_rm_aocomplete
> > > + * device_disk_rm_aocomplete
> > > + * device_vfb_rm_aocomplete
> > > + * device_vkb_rm_aocomplete
> > > + */
> > > +DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
> > > +DEFINE_DEVICE_RM_AOCOMPLETE(nic);
> > > +DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> > > +DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
> > > +DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
> >
> > I was going to suggest merging this with DEFINE_DEVICE_REMOVE but that
> > is used twice for each device so that won't work. But perhaps consider
> > adding these in the same blocks as the usage of DEFINE_DEVICE_REMOVE?
> >
>
> They're already placed right before DEFINE_DEVICE_RM macro. You
> requested this in your last review so I've done it already. ;-)
Heh, what I meant there was to end up with:
/* disk */
DEFINE_DEVICE_RM_AOCOMPLETE(disk);
DEFINE_DEVICE_REMOVE(disk, remove, 0)
DEFINE_DEVICE_REMOVE(disk, destroy, 1)
/* nic */
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 07/18] libxl: separate device add/rm complete callbacks
2014-09-03 11:53 ` Ian Campbell
@ 2014-09-03 11:55 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 11:55 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-09-03 at 12:53 +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 11:34 +0100, Wei Liu wrote:
> > > > + libxl__ao_complete(egc, ao, aodev->rc); \
> > > > + }
> > > > +
> > > > +/*
> > > > + * device_vtpm_rm_aocomplete
> > > > + * device_nic_rm_aocomplete
> > > > + * device_disk_rm_aocomplete
> > > > + * device_vfb_rm_aocomplete
> > > > + * device_vkb_rm_aocomplete
> > > > + */
> > > > +DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
> > > > +DEFINE_DEVICE_RM_AOCOMPLETE(nic);
> > > > +DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> > > > +DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
> > > > +DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
> > >
> > > I was going to suggest merging this with DEFINE_DEVICE_REMOVE but that
> > > is used twice for each device so that won't work. But perhaps consider
> > > adding these in the same blocks as the usage of DEFINE_DEVICE_REMOVE?
> > >
> >
> > They're already placed right before DEFINE_DEVICE_RM macro. You
> > requested this in your last review so I've done it already. ;-)
>
> Heh, what I meant there was to end up with:
>
> /* disk */
> DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> DEFINE_DEVICE_REMOVE(disk, remove, 0)
> DEFINE_DEVICE_REMOVE(disk, destroy, 1)
>
> /* nic */
... oops, that button was "Send" not "Paste". Anyway I think I got the
important bits in ;-)
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 08/18] libxl: introduce libxl__device_from_pcidev
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (6 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 07/18] libxl: separate device add/rm complete callbacks Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 1:45 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 09/18] libxl: disallow attaching the same device more than once Wei Liu
` (10 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Signed-off-by: Wei Liu <wei.liu2@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] 70+ messages in thread* [PATCH v2 09/18] libxl: disallow attaching the same device more than once
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (7 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 08/18] libxl: introduce libxl__device_from_pcidev Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 1:48 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0 Wei Liu
` (9 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 50 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_device.c | 19 ++++++++++++++++
tools/libxl/libxl_internal.h | 3 +++
tools/libxl/libxl_pci.c | 12 ++++++++++
tools/libxl/libxl_types.idl | 1 +
5 files changed, 85 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 01dffa0..e6fe238 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1906,6 +1906,15 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
if ( rc != 0 ) goto out;
+ rc = libxl__device_exists(gc, XBT_NULL, 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;
+ }
+
flexarray_append(back, "frontend-id");
flexarray_append(back, GCSPRINTF("%d", domid));
flexarray_append(back, "online");
@@ -2154,6 +2163,13 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
*
* The passed get_vdev function is also in charge of printing
* the corresponding error message when appropiate.
+ *
+ * Note:
+ *
+ * The presence of get_vdev is also used to distinguish two
+ * cases: if get_vdev != NULL, then it is called by local attach
+ * function to run bootloader, otherwise it is called by
+ * libxl__device_disk_add wrapper.
*/
static void device_disk_add(libxl__egc *egc, uint32_t domid,
libxl_device_disk *disk,
@@ -2177,6 +2193,31 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
goto out;
}
+ /* If we're not doing local attach, we're trying to do
+ * "block-attach". We already have a valid vdev at this point, and
+ * we need to check if this device already exists in xenstore.
+ */
+ if (!get_vdev) {
+ /* Construct libxl__device from libxl_device_disk */
+ libxl__device_disk_setdefault(gc, disk);
+ GCNEW(device);
+ rc = libxl__device_from_disk(gc, domid, disk, device);
+ if (rc != 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+ " virtual disk identifier %s", disk->vdev);
+ goto out;
+ }
+
+ rc = libxl__device_exists(gc, XBT_NULL, 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;
+ }
+ }
+
for (;;) {
rc = libxl__xs_transaction_start(gc, &t);
if (rc) goto out;
@@ -3011,6 +3052,15 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
rc = libxl__device_from_nic(gc, domid, nic, device);
if ( rc != 0 ) goto out;
+ rc = libxl__device_exists(gc, XBT_NULL, 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;
+ }
+
flexarray_append(back, "frontend-id");
flexarray_append(back, libxl__sprintf(gc, "%d", domid));
flexarray_append(back, "online");
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 02708c7..1b44c2c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1031,6 +1031,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..e05f047 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1042,6 +1042,7 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
unsigned int orig_vdev, pfunc_mask;
libxl_device_pci *assigned;
int num_assigned, i, rc;
+ libxl__device device;
int stubdomid = 0;
if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
@@ -1059,6 +1060,17 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
rc = libxl__device_pci_setdefault(gc, pcidev);
if (rc) goto out;
+ if (!starting) {
+ libxl__device_from_pcidev(gc, domid, pcidev, &device);
+ rc = libxl__device_exists(gc, XBT_NULL, &device);
+ if (rc < 0) goto out;
+ if (rc == 1) { /* already exists in xenstore */
+ LOG(ERROR, "device already exists in xenstore");
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
+ }
+ }
+
if (pcidev->seize && !pciback_dev_is_assigned(gc, pcidev)) {
rc = libxl__device_pci_assignable_add(gc, pcidev, 1);
if ( rc )
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d0a373b..1beb63d 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] 70+ messages in thread* Re: [PATCH v2 09/18] libxl: disallow attaching the same device more than once
2014-07-30 18:23 ` [PATCH v2 09/18] libxl: disallow attaching the same device more than once Wei Liu
@ 2014-08-27 1:48 ` Ian Campbell
2014-08-28 10:55 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 1:48 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +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.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/libxl/libxl.c | 50 ++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_device.c | 19 ++++++++++++++++
> tools/libxl/libxl_internal.h | 3 +++
> tools/libxl/libxl_pci.c | 12 ++++++++++
> tools/libxl/libxl_types.idl | 1 +
> 5 files changed, 85 insertions(+)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 01dffa0..e6fe238 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1906,6 +1906,15 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
> if ( rc != 0 ) goto out;
>
> + rc = libxl__device_exists(gc, XBT_NULL, device);
Do we hold any locks from here until the code which actually creates the
device? I don't think we have an active transaction either which would
serve a similar purpose.
Otherwise can't another thread come and create another device after the
check and similarly confuse us?
(likewise for the other cases).
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 09/18] libxl: disallow attaching the same device more than once
2014-08-27 1:48 ` Ian Campbell
@ 2014-08-28 10:55 ` Wei Liu
2014-09-03 11:52 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 10:55 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 02:48:30AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +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.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > tools/libxl/libxl.c | 50 ++++++++++++++++++++++++++++++++++++++++++
> > tools/libxl/libxl_device.c | 19 ++++++++++++++++
> > tools/libxl/libxl_internal.h | 3 +++
> > tools/libxl/libxl_pci.c | 12 ++++++++++
> > tools/libxl/libxl_types.idl | 1 +
> > 5 files changed, 85 insertions(+)
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 01dffa0..e6fe238 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1906,6 +1906,15 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> > rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
> > if ( rc != 0 ) goto out;
> >
> > + rc = libxl__device_exists(gc, XBT_NULL, device);
>
> Do we hold any locks from here until the code which actually creates the
> device? I don't think we have an active transaction either which would
> serve a similar purpose.
>
At this point we hold libxl context lock, so it should be safe against
other threads in the same process.
But I can see there's problem WRT to other processes trying to
manipulate device. I think we need to have a xs transaction here. What
do you think?
Wei.
> Otherwise can't another thread come and create another device after the
> check and similarly confuse us?
>
> (likewise for the other cases).
>
> Ian.
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 09/18] libxl: disallow attaching the same device more than once
2014-08-28 10:55 ` Wei Liu
@ 2014-09-03 11:52 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 11:52 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 11:55 +0100, Wei Liu wrote:
> On Wed, Aug 27, 2014 at 02:48:30AM +0100, Ian Campbell wrote:
> > On Wed, 2014-07-30 at 19:23 +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.
> > >
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > tools/libxl/libxl.c | 50 ++++++++++++++++++++++++++++++++++++++++++
> > > tools/libxl/libxl_device.c | 19 ++++++++++++++++
> > > tools/libxl/libxl_internal.h | 3 +++
> > > tools/libxl/libxl_pci.c | 12 ++++++++++
> > > tools/libxl/libxl_types.idl | 1 +
> > > 5 files changed, 85 insertions(+)
> > >
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 01dffa0..e6fe238 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -1906,6 +1906,15 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> > > rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
> > > if ( rc != 0 ) goto out;
> > >
> > > + rc = libxl__device_exists(gc, XBT_NULL, device);
> >
> > Do we hold any locks from here until the code which actually creates the
> > device? I don't think we have an active transaction either which would
> > serve a similar purpose.
> >
>
> At this point we hold libxl context lock, so it should be safe against
> other threads in the same process.
>
> But I can see there's problem WRT to other processes trying to
> manipulate device. I think we need to have a xs transaction here. What
> do you think?
I'm not sure, but it sounds plausible.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (8 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 09/18] libxl: disallow attaching the same device more than once Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-07-31 8:34 ` Ian Campbell
2014-08-27 1:52 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device Wei Liu
` (8 subsequent siblings)
18 siblings, 2 replies; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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.
Modify tools/Makefile to move "misc" after "libxl" as it now depends on
libxl to build.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
.gitignore | 1 +
tools/Makefile | 2 +-
tools/hotplug/Linux/init.d/xencommons | 5 +-
tools/misc/Makefile | 8 +-
tools/misc/xen-init-dom0.c | 130 +++++++++++++++++++++++++++++++++
5 files changed, 140 insertions(+), 6 deletions(-)
create mode 100644 tools/misc/xen-init-dom0.c
diff --git a/.gitignore b/.gitignore
index fefe13c..a6370d8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -183,6 +183,7 @@ tools/misc/gtracestat
tools/misc/xenlockprof
tools/misc/lowmemd
tools/misc/xencov
+tools/misc/xen-init-dom0
tools/pygrub/build/*
tools/python/build/*
tools/python/xen/util/path.py
diff --git a/tools/Makefile b/tools/Makefile
index b6476c9..89fc698 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -6,7 +6,6 @@ SUBDIRS-y += include
SUBDIRS-y += libxc
SUBDIRS-$(FLASK_ENABLE) += flask
SUBDIRS-y += xenstore
-SUBDIRS-y += misc
SUBDIRS-y += examples
SUBDIRS-y += hotplug
SUBDIRS-y += xentrace
@@ -30,6 +29,7 @@ endif
SUBDIRS-y += xenpmd
SUBDIRS-y += libxl
+SUBDIRS-y += misc
SUBDIRS-$(CONFIG_X86) += xenpaging
SUBDIRS-$(CONFIG_X86) += debugger/gdbsx
SUBDIRS-$(CONFIG_X86) += debugger/kdd
diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
index 4ebd636..1811eee 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons
@@ -108,9 +108,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/misc/Makefile b/tools/misc/Makefile
index 69b1817..1223e19 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -6,10 +6,11 @@ CFLAGS += -Werror
CFLAGS += $(CFLAGS_libxenctrl)
CFLAGS += $(CFLAGS_xeninclude)
CFLAGS += $(CFLAGS_libxenstore)
+CFLAGS += $(CFLAGS_libxenlight)
HDRS = $(wildcard *.h)
-TARGETS-y := xenperf xenpm xen-tmem-list-parse gtraceview gtracestat xenlockprof xenwatchdogd xencov
+TARGETS-y := xenperf xenpm xen-tmem-list-parse gtraceview gtracestat xenlockprof xenwatchdogd xencov xen-init-dom0
TARGETS-$(CONFIG_X86) += xen-detect xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump
TARGETS-$(CONFIG_MIGRATE) += xen-hptool
TARGETS := $(TARGETS-y)
@@ -26,7 +27,7 @@ INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump
INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
INSTALL_SBIN := $(INSTALL_SBIN-y)
-INSTALL_PRIVBIN-y := xenpvnetboot
+INSTALL_PRIVBIN-y := xenpvnetboot xen-init-dom0
INSTALL_PRIVBIN := $(INSTALL_PRIVBIN-y)
# Include configure output (config.h) to headers search path
@@ -90,4 +91,7 @@ gtraceview: gtraceview.o
xencov: xencov.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+xen-init-dom0: xen-init-dom0.o
+ $(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
+
-include $(DEPS)
diff --git a/tools/misc/xen-init-dom0.c b/tools/misc/xen-init-dom0.c
new file mode 100644
index 0000000..4bb1a96
--- /dev/null
+++ b/tools/misc/xen-init-dom0.c
@@ -0,0 +1,130 @@
+#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 xc_interface *xch;
+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);
+ }
+ xch = xc_interface_open(0, 0, 0);
+ if (!xch) {
+ fprintf(stderr, "cannot open libxc handle\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 (xch) {
+ xc_interface_close(xch);
+ xch = 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 /* inlcude '\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] 70+ messages in thread* Re: [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0
2014-07-30 18:23 ` [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0 Wei Liu
@ 2014-07-31 8:34 ` Ian Campbell
2014-08-27 1:52 ` Ian Campbell
1 sibling, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-07-31 8:34 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
I don't think I'm going to have a chance to review this series in detail
but I noticed two little things while skimming:
> + xch = xc_interface_open(0, 0, 0);
> + if (!xch) {
> + fprintf(stderr, "cannot open libxc handle\n");
> + exit(1);
> + }
AFAICT you don't do anything with this apart from closing it again.
> + rc = libxl_userdata_store(ctx, 0, "libxl-json",
> + (const uint8_t *)json,
> + strlen(json) + 1 /* inlcude '\0' */);
"include"
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0
2014-07-30 18:23 ` [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0 Wei Liu
2014-07-31 8:34 ` Ian Campbell
@ 2014-08-27 1:52 ` Ian Campbell
2014-08-28 10:58 ` Wei Liu
1 sibling, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 1:52 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> index 69b1817..1223e19 100644
> --- a/tools/misc/Makefile
> +++ b/tools/misc/Makefile
> @@ -6,10 +6,11 @@ CFLAGS += -Werror
> CFLAGS += $(CFLAGS_libxenctrl)
> CFLAGS += $(CFLAGS_xeninclude)
> CFLAGS += $(CFLAGS_libxenstore)
> +CFLAGS += $(CFLAGS_libxenlight)
This Makefile is already a bit of a dumping ground but I wonder if we
can do
xen-init-dom0.o: CFLAGS += $(CFLAGS_libxenlight)
to start separating things out a bit?
Or arguably this whole thing could live in tools/libxl.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0
2014-08-27 1:52 ` Ian Campbell
@ 2014-08-28 10:58 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-08-28 10:58 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 02:52:53AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> > index 69b1817..1223e19 100644
> > --- a/tools/misc/Makefile
> > +++ b/tools/misc/Makefile
> > @@ -6,10 +6,11 @@ CFLAGS += -Werror
> > CFLAGS += $(CFLAGS_libxenctrl)
> > CFLAGS += $(CFLAGS_xeninclude)
> > CFLAGS += $(CFLAGS_libxenstore)
> > +CFLAGS += $(CFLAGS_libxenlight)
>
> This Makefile is already a bit of a dumping ground but I wonder if we
> can do
> xen-init-dom0.o: CFLAGS += $(CFLAGS_libxenlight)
> to start separating things out a bit?
>
> Or arguably this whole thing could live in tools/libxl.
>
I'm fine with putting this in tools/libxl.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (9 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 10/18] tools/misc: introduce helper to initialise Dom0 Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 2:00 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 12/18] libxl: synchronise configuration when we remove/destroy " Wei Liu
` (7 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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 locking pattern is as followed:
1. lock JSON domain config
2. write JSON entry
3. write xenstore entry for a device
4. unlock JSON domain config
And we ensure in code that JSON entry is always written before the
xenstore entry is written. That is, if 2 fails, 3 will not be executed.
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 | 64 ++++++++++++++++++
tools/libxl/libxl_internal.h | 151 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_pci.c | 20 ++++++
3 files changed, 235 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e6fe238..184e126 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1888,6 +1888,11 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
flexarray_t *back;
libxl__device *device;
int rc;
+ libxl_device_vtpm vtpm_saved;
+ libxl__carefd *lock = NULL;
+
+ 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;
@@ -1902,6 +1907,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;
@@ -1936,6 +1943,18 @@ 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_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = DEVICE_ADD_JSON(vtpm, vtpms, num_vtpms, domid, &vtpm_saved,
+ COMPARE_DEVID);
+ if (rc) goto out;
+ }
+
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),
@@ -1947,6 +1966,8 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
+ if (lock) libxl__unlock_domain_data(lock);
+ libxl_device_vtpm_dispose(&vtpm_saved);
aodev->rc = rc;
if(rc) aodev->callback(egc, aodev);
return;
@@ -2186,6 +2207,10 @@ 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_device_disk disk_saved;
+ libxl__carefd *lock = NULL;
+
+ libxl_device_disk_init(&disk_saved);
libxl_domain_type type = libxl__domain_type(gc, domid);
if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -2198,6 +2223,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
* we need to check if this device already exists in xenstore.
*/
if (!get_vdev) {
+ libxl_device_disk_copy(CTX, &disk_saved, disk);
+
/* Construct libxl__device from libxl_device_disk */
libxl__device_disk_setdefault(gc, disk);
GCNEW(device);
@@ -2216,6 +2243,18 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
rc = ERROR_DEVICE_EXISTS;
goto out;
}
+
+ if (aodev && aodev->update_json) {
+ lock = libxl__lock_domain_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = DEVICE_ADD_JSON(disk, disks, num_disks, domid,
+ &disk_saved, COMPARE_DISK);
+ if (rc) goto out;
+ }
}
for (;;) {
@@ -2378,6 +2417,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
+ if (lock) libxl__unlock_domain_data(lock);
+ libxl_device_disk_dispose(&disk_saved);
libxl__xs_transaction_abort(gc, &t);
aodev->rc = rc;
if (rc) aodev->callback(egc, aodev);
@@ -3034,6 +3075,11 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
flexarray_t *back;
libxl__device *device;
int rc;
+ libxl_device_nic nic_saved;
+ libxl__carefd *lock = NULL;
+
+ 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;
@@ -3048,6 +3094,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;
@@ -3113,6 +3161,19 @@ 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)));
+
+ if (aodev->update_json) {
+ lock = libxl__lock_domain_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = DEVICE_ADD_JSON(nic, nics, num_nics, domid, &nic_saved,
+ COMPARE_DEVID);
+ if (rc) goto out;
+ }
+
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),
@@ -3124,6 +3185,8 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
rc = 0;
out:
+ if (lock) libxl__unlock_domain_data(lock);
+ libxl_device_nic_dispose(&nic_saved);
aodev->rc = rc;
if (rc) aodev->callback(egc, aodev);
return;
@@ -3702,6 +3765,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
\
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
+ aodev->update_json = true; \
aodev->callback = device_add_aocomplete; \
libxl__device_##type##_add(egc, domid, type, aodev); \
\
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1b44c2c..a5eb7d4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2144,6 +2144,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;
};
/*
@@ -3271,6 +3273,155 @@ 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)
+
+/* Expression int DEVICE_{ADD,REMOVE,UPDATE}_JSON
+ *
+ * Add / remove / update a device in JSON config file. Caller
+ * must hold data store lock before invoking these macros.
+ *
+ * They take 6 parameters:
+ * type: the type of the device, say nic, vtpm, disk, pci etc
+ * ptr: the pointer to array inside libxl_domain_config
+ * cnt: the counter of array
+ * 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
+ *
+ * Return 0 if no error occurs, ERROR_* otherwise.
+ *
+ * For most device types (nic, vtpm), the array name @ptr and array
+ * counter @cnt can be derived from @type, pci device being the
+ * exception, hence we need to have @ptr and @cnt.
+ */
+
+/* Add a device to JSON config. If there is already device with the
+ * same identifier in JSON config, no action will be taken.
+ * Use the "update" macro to update JSON config of a device.
+ */
+#define DEVICE_ADD_JSON(type, ptr, cnt, domid, dev, compare) \
+ ({ \
+ int DAJ_x, DAJ_rc; \
+ libxl_domain_config DAJ_d_config; \
+ libxl_device_##type *DAJ_p; \
+ \
+ libxl_domain_config_init(&DAJ_d_config); \
+ \
+ DAJ_rc = libxl__get_domain_configuration(gc, (domid), \
+ &DAJ_d_config); \
+ if (DAJ_rc) \
+ goto DAJ_out; \
+ \
+ /* Check for duplicated device */ \
+ for (DAJ_x = 0; DAJ_x < DAJ_d_config.cnt; DAJ_x++) { \
+ if (compare(&DAJ_d_config.ptr[DAJ_x], (dev))) { \
+ DAJ_rc = 0; \
+ goto DAJ_out; \
+ } \
+ } \
+ \
+ DAJ_d_config.ptr = \
+ libxl__realloc(NOGC, DAJ_d_config.ptr, \
+ (DAJ_d_config.cnt + 1) * \
+ sizeof(libxl_device_##type)); \
+ DAJ_p = &DAJ_d_config.ptr[DAJ_d_config.cnt]; \
+ DAJ_d_config.cnt++; \
+ libxl_device_##type##_copy(CTX, DAJ_p, (dev)); \
+ \
+ DAJ_rc = libxl__set_domain_configuration(gc, (domid), \
+ &DAJ_d_config); \
+ \
+ DAJ_out: \
+ libxl_domain_config_dispose(&DAJ_d_config); \
+ DAJ_rc; \
+ })
+
+/* Remove a device from JSON config. */
+#define DEVICE_REMOVE_JSON(type, ptr, cnt, domid, dev, compare) \
+ ({ \
+ int DRJ_i, DRJ_j, DRJ_rc; \
+ libxl_device_##type *DRJ_p = dev; \
+ libxl_domain_config DRJ_d_config; \
+ \
+ libxl_domain_config_init(&DRJ_d_config); \
+ \
+ DRJ_rc = libxl__get_domain_configuration(gc, (domid), \
+ &DRJ_d_config); \
+ if (DRJ_rc) \
+ goto DRJ_out; \
+ \
+ /* This loop may shuffle down the array */ \
+ for (DRJ_i = DRJ_j = 0; DRJ_i < DRJ_d_config.cnt; DRJ_i++) { \
+ if (!compare(&DRJ_d_config.ptr[DRJ_i], DRJ_p)) { \
+ if (DRJ_i != DRJ_j) { \
+ libxl_device_##type DRJ_tmp; \
+ DRJ_tmp = DRJ_d_config.ptr[DRJ_j]; \
+ DRJ_d_config.ptr[DRJ_j] = DRJ_d_config.ptr[DRJ_i]; \
+ DRJ_d_config.ptr[DRJ_i] = DRJ_tmp; \
+ } \
+ DRJ_j++; \
+ } \
+ } \
+ \
+ if (DRJ_i == DRJ_j) /* no matching entry found */ \
+ goto DRJ_out; \
+ \
+ libxl_device_##type##_dispose(&DRJ_d_config.ptr[DRJ_j]); \
+ DRJ_d_config.ptr = \
+ libxl__realloc(NOGC, DRJ_d_config.ptr, \
+ DRJ_j * sizeof(libxl_device_##type)); \
+ DRJ_d_config.cnt = DRJ_j; \
+ \
+ DRJ_rc = libxl__set_domain_configuration(gc, (domid), \
+ &DRJ_d_config); \
+ \
+ DRJ_out: \
+ libxl_domain_config_dispose(&DRJ_d_config); \
+ DRJ_rc; \
+ })
+
+/* Search device list for device with the same identifier as "dev",
+ * update that device with the content pointed to by "dev".
+ */
+#define DEVICE_UPDATE_JSON(type, ptr, cnt, domid, dev, compare) \
+ ({ \
+ int DUJ_x, DUJ_rc; \
+ libxl_domain_config DUJ_d_config; \
+ bool DUJ_updated = false; \
+ \
+ libxl_domain_config_init(&DUJ_d_config); \
+ \
+ DUJ_rc = libxl__get_domain_configuration(gc, domid, \
+ &DUJ_d_config); \
+ if (DUJ_rc) \
+ goto DUJ_out; \
+ \
+ for (DUJ_x = 0; DUJ_x < DUJ_d_config.cnt; DUJ_x++) { \
+ if (compare(&DUJ_d_config.ptr[DUJ_x], (dev))) { \
+ libxl_device_##type##_dispose( \
+ &DUJ_d_config.ptr[DUJ_x]); \
+ libxl_device_##type##_copy(CTX, \
+ &DUJ_d_config.ptr[DUJ_x], \
+ (dev)); \
+ DUJ_updated = true; \
+ } \
+ } \
+ if (DUJ_updated) \
+ DUJ_rc = libxl__set_domain_configuration(gc, domid, \
+ &DUJ_d_config); \
+ \
+ DUJ_out: \
+ libxl_domain_config_dispose(&DUJ_d_config); \
+ DUJ_rc; \
+ })
+
#endif
/*
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index e05f047..357e274 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1044,6 +1044,10 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
int num_assigned, i, rc;
libxl__device device;
int stubdomid = 0;
+ libxl_device_pci pcidev_saved;
+ libxl__carefd *lock = NULL;
+
+ libxl_device_pci_init(&pcidev_saved);
if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
rc = xc_test_assign_device(ctx->xch, domid, pcidev_encode_bdf(pcidev));
@@ -1057,6 +1061,8 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
}
}
+ libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
+
rc = libxl__device_pci_setdefault(gc, pcidev);
if (rc) goto out;
@@ -1125,6 +1131,18 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
pfunc_mask = (1 << pcidev->func);
}
+ if (!starting) {
+ lock = libxl__lock_domain_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = DEVICE_ADD_JSON(pci, pcidevs, num_pcidevs, domid, pcidev,
+ COMPARE_PCI);
+ if (rc) goto out;
+ }
+
for(rc = 0, i = 7; i >= 0; --i) {
if ( (1 << i) & pfunc_mask ) {
if ( pcidev->vfunc_mask == pfunc_mask ) {
@@ -1143,6 +1161,8 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
}
out:
+ if (lock) libxl__unlock_domain_data(lock);
+ libxl_device_pci_dispose(&pcidev_saved);
return rc;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device
2014-07-30 18:23 ` [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device Wei Liu
@ 2014-08-27 2:00 ` Ian Campbell
2014-08-28 11:18 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 2:00 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> 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.
Rather than carry a flag around did you consider just checking for the
presence of the file? I think you indicated in an earlier patch that you
were going to treat lack of the file as meaning creation/destruction was
happening.
> + * They take 6 parameters:
> + * type: the type of the device, say nic, vtpm, disk, pci etc
> + * ptr: the pointer to array inside libxl_domain_config
To the array or to a specific element of the array? I think the latter.
You might also want to indicate that the ptr must be of type
libxl_device_#type?
> + * cnt: the counter of array
I think you either mean index or length, I suspect the former?
> + * 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
> + *
> + * Return 0 if no error occurs, ERROR_* otherwise.
> + *
> + * For most device types (nic, vtpm), the array name @ptr and array
> + * counter @cnt can be derived from @type, pci device being the
> + * exception, hence we need to have @ptr and @cnt.
You could get away with a single variable naming the Array ("nics",
"pcidevs") to which you paste num_ on the front when you need cnt.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device
2014-08-27 2:00 ` Ian Campbell
@ 2014-08-28 11:18 ` Wei Liu
2014-09-03 11:57 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 11:18 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 03:00:39AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > 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.
>
> Rather than carry a flag around did you consider just checking for the
> presence of the file? I think you indicated in an earlier patch that you
> were going to treat lack of the file as meaning creation/destruction was
> happening.
>
I think a flag is more explict.
These libxl__device_*_add functions are called under two circumstances,
a) domain creation, b) device hotplug.
In b) if the file is not present it could also be in abnormal state --
file is deleted by accident, file system error etc. Carrying a flag can
help up distinguish normal and abnormal state. (Though I can see my code
currently lacks checking this at the moment)
>
> > + * They take 6 parameters:
> > + * type: the type of the device, say nic, vtpm, disk, pci etc
> > + * ptr: the pointer to array inside libxl_domain_config
>
> To the array or to a specific element of the array? I think the latter.
To the array of libxl_device_#type. It doesn't point to specific
element.
> You might also want to indicate that the ptr must be of type
> libxl_device_#type?
>
Sure.
> > + * cnt: the counter of array
>
> I think you either mean index or length, I suspect the former?
>
> > + * 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
> > + *
> > + * Return 0 if no error occurs, ERROR_* otherwise.
> > + *
> > + * For most device types (nic, vtpm), the array name @ptr and array
> > + * counter @cnt can be derived from @type, pci device being the
> > + * exception, hence we need to have @ptr and @cnt.
>
> You could get away with a single variable naming the Array ("nics",
> "pcidevs") to which you paste num_ on the front when you need cnt.
>
OK.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device
2014-08-28 11:18 ` Wei Liu
@ 2014-09-03 11:57 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 11:57 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 12:18 +0100, Wei Liu wrote:
> On Wed, Aug 27, 2014 at 03:00:39AM +0100, Ian Campbell wrote:
> > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > 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.
> >
> > Rather than carry a flag around did you consider just checking for the
> > presence of the file? I think you indicated in an earlier patch that you
> > were going to treat lack of the file as meaning creation/destruction was
> > happening.
> >
>
> I think a flag is more explict.
>
> These libxl__device_*_add functions are called under two circumstances,
> a) domain creation, b) device hotplug.
>
> In b) if the file is not present it could also be in abnormal state --
> file is deleted by accident, file system error etc. Carrying a flag can
> help up distinguish normal and abnormal state. (Though I can see my code
> currently lacks checking this at the moment)
OK.
> >
> > > + * They take 6 parameters:
> > > + * type: the type of the device, say nic, vtpm, disk, pci etc
> > > + * ptr: the pointer to array inside libxl_domain_config
> >
> > To the array or to a specific element of the array? I think the latter.
>
> To the array of libxl_device_#type. It doesn't point to specific
> element.
OK. "...pointer to the start of the array..." would be unambiguous
(assuming the later comment about pasting num_ prefixes didn't make this
comment go away altogether)
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 12/18] libxl: synchronise configuration when we remove/destroy a device
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (10 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 2:01 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
` (6 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Xenstore is updated first, JSON is updated later, so that we maintain
the same invariant as previous commit: any device which is present in
xenstore has an entry in JSON.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 28 ++++++++++++++++++++++------
tools/libxl/libxl_internal.h | 2 ++
tools/libxl/libxl_pci.c | 15 +++++++++++++++
3 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 184e126..430a45f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3654,7 +3654,7 @@ out:
}
/******************************************************************************/
-#define DEFINE_DEVICE_RM_AOCOMPLETE(type) \
+#define DEFINE_DEVICE_RM_AOCOMPLETE(type, compare) \
static void device_##type##_rm_aocomplete(libxl__egc *egc, \
libxl__ao_device *aodev) \
{ \
@@ -3662,6 +3662,21 @@ out:
if (aodev->rc) { \
DEVICE_AO_FAILED_MSG; \
goto out; \
+ } else { \
+ libxl__carefd *lock; \
+ \
+ lock = libxl__lock_domain_data(gc, aodev->dev->domid); \
+ if (!lock) { \
+ aodev->rc = ERROR_LOCK_FAIL; \
+ goto out; \
+ } \
+ \
+ aodev->rc = DEVICE_REMOVE_JSON(type, type##s, \
+ num_##type##s, \
+ aodev->dev->domid, \
+ aodev->pdev, compare); \
+ \
+ libxl__unlock_domain_data(lock); \
} \
\
out: \
@@ -3675,11 +3690,11 @@ out:
* device_vfb_rm_aocomplete
* device_vkb_rm_aocomplete
*/
-DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
-DEFINE_DEVICE_RM_AOCOMPLETE(nic);
-DEFINE_DEVICE_RM_AOCOMPLETE(disk);
-DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
-DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
+DEFINE_DEVICE_RM_AOCOMPLETE(vtpm, COMPARE_DEVID);
+DEFINE_DEVICE_RM_AOCOMPLETE(nic, COMPARE_DEVID);
+DEFINE_DEVICE_RM_AOCOMPLETE(disk, COMPARE_DISK);
+DEFINE_DEVICE_RM_AOCOMPLETE(vfb, COMPARE_DEVID);
+DEFINE_DEVICE_RM_AOCOMPLETE(vkb, COMPARE_DEVID);
/* Macro for defining device remove/destroy functions in a compact way */
/* The following functions are defined:
@@ -3710,6 +3725,7 @@ DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
\
GCNEW(aodev); \
libxl__prepare_ao_device(ao, aodev); \
+ aodev->pdev = type; \
aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
aodev->dev = device; \
aodev->callback = device_##type##_rm_aocomplete; \
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a5eb7d4..eeeffdb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2146,6 +2146,8 @@ struct libxl__ao_device {
libxl__async_exec_state aes;
/* If we need to udpate JSON config */
bool update_json;
+ /* point to libxl_device_TYPE */
+ void *pdev;
};
/*
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 357e274..9ad1f19 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1372,6 +1372,21 @@ int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid,
rc = libxl__device_pci_remove_common(gc, domid, pcidev, 0);
+ if (!rc) {
+ libxl__carefd *lock;
+
+ lock = libxl__lock_domain_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = DEVICE_REMOVE_JSON(pci, pcidevs, num_pcidevs, domid,
+ pcidev, COMPARE_PCI);
+
+ libxl__unlock_domain_data(lock);
+ }
+out:
libxl__ao_complete(egc, ao, rc);
return AO_INPROGRESS;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert"
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (11 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 12/18] libxl: synchronise configuration when we remove/destroy " Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 2:04 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max Wei Liu
` (5 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 430a45f..d64d8f1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2640,8 +2640,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;
@@ -2752,6 +2753,58 @@ 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.
+ *
+ * In order to maintain lock hierachy, 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_device_disk empty;
+ int rc;
+ libxl__carefd *lock = NULL;
+
+ CTX_LOCK;
+
+ 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_data(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+ rc = DEVICE_UPDATE_JSON(disk, disks, num_disks, domid, disk,
+ COMPARE_DISK);
+ libxl__unlock_domain_data(lock);
+
+ rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
+
+out:
+ libxl_device_disk_dispose(&empty);
+ 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] 70+ messages in thread* Re: [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert"
2014-07-30 18:23 ` [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
@ 2014-08-27 2:04 ` Ian Campbell
2014-08-28 11:25 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 2:04 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +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.
When you say "empty" do you mean literally that the xenstore nodes do
not exist or that one of them contains the string "empty"? If, as I
suspect, you mean the former then writing
if xenstore nodes are not present then CDROM is "empty"
would confuse me less.
> + rc = libxl__cdrom_insert(ctx, domid, &empty, NULL);
> + if (rc)
> + goto out;
> +
> + /* Optimisation: don't insert empty disk twice, and skip
> + * manipulating JSON. */
Do we not need to update the JSON in this case to reflect that it is now
empty?
Ian/
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert"
2014-08-27 2:04 ` Ian Campbell
@ 2014-08-28 11:25 ` Wei Liu
2014-08-28 18:14 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 11:25 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 03:04:46AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +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.
>
> When you say "empty" do you mean literally that the xenstore nodes do
> not exist or that one of them contains the string "empty"? If, as I
> suspect, you mean the former then writing
> if xenstore nodes are not present then CDROM is "empty"
> would confuse me less.
>
The xenstore node still exists, just that "params" doesn't point to
media. I can rephrase this:
If xenstore entry doesn't point to media file then CDROM is "empty".
> > + rc = libxl__cdrom_insert(ctx, domid, &empty, NULL);
> > + if (rc)
> > + goto out;
> > +
> > + /* Optimisation: don't insert empty disk twice, and skip
> > + * manipulating JSON. */
>
> Do we not need to update the JSON in this case to reflect that it is now
> empty?
>
We could but we don't have to -- xenstore is primary reference.
Wei.
> Ian/
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert"
2014-08-28 11:25 ` Wei Liu
@ 2014-08-28 18:14 ` Ian Campbell
2014-08-28 18:38 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-28 18:14 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 12:25 +0100, Wei Liu wrote:
> On Wed, Aug 27, 2014 at 03:04:46AM +0100, Ian Campbell wrote:
> > On Wed, 2014-07-30 at 19:23 +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.
> >
> > When you say "empty" do you mean literally that the xenstore nodes do
> > not exist or that one of them contains the string "empty"? If, as I
> > suspect, you mean the former then writing
> > if xenstore nodes are not present then CDROM is "empty"
> > would confuse me less.
> >
>
> The xenstore node still exists, just that "params" doesn't point to
> media.
"doesn't point to" means doesn't exist or contains the empty string? I
think it would be good to be explicit there.
> I can rephrase this:
>
> If xenstore entry doesn't point to media file then CDROM is "empty".
>
> > > + rc = libxl__cdrom_insert(ctx, domid, &empty, NULL);
> > > + if (rc)
> > > + goto out;
> > > +
> > > + /* Optimisation: don't insert empty disk twice, and skip
> > > + * manipulating JSON. */
> >
> > Do we not need to update the JSON in this case to reflect that it is now
> > empty?
> >
>
> We could but we don't have to -- xenstore is primary reference.
OK.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert"
2014-08-28 18:14 ` Ian Campbell
@ 2014-08-28 18:38 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-08-28 18:38 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Thu, Aug 28, 2014 at 07:14:35PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 12:25 +0100, Wei Liu wrote:
> > On Wed, Aug 27, 2014 at 03:04:46AM +0100, Ian Campbell wrote:
> > > On Wed, 2014-07-30 at 19:23 +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.
> > >
> > > When you say "empty" do you mean literally that the xenstore nodes do
> > > not exist or that one of them contains the string "empty"? If, as I
> > > suspect, you mean the former then writing
> > > if xenstore nodes are not present then CDROM is "empty"
> > > would confuse me less.
> > >
> >
> > The xenstore node still exists, just that "params" doesn't point to
> > media.
>
> "doesn't point to" means doesn't exist or contains the empty string? I
> think it would be good to be explicit there.
>
Hrm, it's not necessary NIL, it's what the backend thinks there's no
media inside. ISTR seeing "aio:" or just "". They both represent an
empty cdrom. That's why I only use the word "empty" to refer to its
state instead of being explict about the content in "params". This patch
itself won't care about the incarnation of that state anyway.
Wei.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (12 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 13/18] libxl: make libxl_cd_insert "eject" + "insert" Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 2:09 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
` (4 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
... which returns the "static-max" knob of a domain. It will be used in
later patch to retrieve memory static-max value of a domain.
As libxl_get_memory_{target,static_max} have similar, logic, a helper
function is introduced to avoid code duplication.
libxl__fill_dom0_memory_info is adjusted as needed.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/libxl.c | 93 ++++++++++++++++++++++++++++++++++++++++++---------
tools/libxl/libxl.h | 8 +++++
2 files changed, 86 insertions(+), 15 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d64d8f1..6083202 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4233,7 +4233,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;
@@ -4267,6 +4268,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;
@@ -4313,12 +4325,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;
@@ -4341,6 +4353,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;
@@ -4356,7 +4369,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;
@@ -4471,38 +4485,87 @@ out_no_transaction:
return rc;
}
-int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target)
+/* type determines which node to return:
+ * 1 "target"
+ * 2 "static-max"
+ */
+static int libxl__get_memory_target(libxl__gc *gc, uint32_t domid,
+ uint32_t *out_mem, int type)
{
- 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;
+ }
+
+ }
+ switch (type) {
+ case 1:
+ *out_mem = target_memkb; break;
+ case 2:
+ *out_mem = max_memkb; break;
}
- *out_target = 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, 1);
+
+ GC_FREE;
+ return rc;
+}
+
+int libxl_get_memory_static_max(libxl_ctx *ctx, uint32_t domid,
+ uint32_t *out_static_max)
+{
+ GC_INIT(ctx);
+ int rc;
+
+ rc = libxl__get_memory_target(gc, domid, out_static_max, 2);
+
GC_FREE;
return rc;
}
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 11e391d..7a0b58c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -358,6 +358,13 @@ typedef struct libxl__ctx libxl_ctx;
#endif
#endif
+/* LIBXL_HAVE_GET_MEMORY_STATIC_MAX
+ *
+ * If this is defined we have an API called libxl_get_memory_static_max
+ * to return the memory "static-max" value from xenstore.
+ */
+#define LIBXL_HAVE_GET_MEMORY_STATIC_MAX 1
+
/*
* LIBXL_HAVE_BUILDINFO_VCPU_AFFINITY_ARRAYS
*
@@ -884,6 +891,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
+int libxl_get_memory_static_max(libxl_ctx *ctx, uint32_t domid, uint32_t *out_static_max);
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max
2014-07-30 18:23 ` [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max Wei Liu
@ 2014-08-27 2:09 ` Ian Campbell
2014-08-28 11:31 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 2:09 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> +/* type determines which node to return:
> + * 1 "target"
> + * 2 "static-max"
You've just invented enums ;-)
Either use an actual enum, or a bool flag (since this is internal only I
don't mind which)
> +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, 1);
FWIW for trivial helper wrappers like this where the wrapper does
nothing I think it can be acceptable to bend the usual rule about
internal things taking a gc a bit. Although that can just be saving
problems for later when someone adds some new magic to a wrapper.
> @@ -884,6 +891,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
> int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
> int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
> +int libxl_get_memory_static_max(libxl_ctx *ctx, uint32_t domid, uint32_t *out_static_max);
Do you need this to be public, or did you just do it for consistency?
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max
2014-08-27 2:09 ` Ian Campbell
@ 2014-08-28 11:31 ` Wei Liu
2014-08-28 18:16 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 11:31 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 03:09:09AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > +/* type determines which node to return:
> > + * 1 "target"
> > + * 2 "static-max"
>
> You've just invented enums ;-)
>
> Either use an actual enum, or a bool flag (since this is internal only I
> don't mind which)
>
Bool flag it is.
> > +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, 1);
>
> FWIW for trivial helper wrappers like this where the wrapper does
> nothing I think it can be acceptable to bend the usual rule about
> internal things taking a gc a bit. Although that can just be saving
> problems for later when someone adds some new magic to a wrapper.
>
I would rather keep the code as it is now.
> > @@ -884,6 +891,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
> > int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
> > int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> > int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
> > +int libxl_get_memory_static_max(libxl_ctx *ctx, uint32_t domid, uint32_t *out_static_max);
>
> Do you need this to be public, or did you just do it for consistency?
>
I did it for consistency, in case it's useful to library users. If you
would rather not expose this I can make it internal.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max
2014-08-28 11:31 ` Wei Liu
@ 2014-08-28 18:16 ` Ian Campbell
2014-08-28 18:39 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-28 18:16 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 12:31 +0100, Wei Liu wrote:
> > > @@ -884,6 +891,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
> > > int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
> > > int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> > > int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
> > > +int libxl_get_memory_static_max(libxl_ctx *ctx, uint32_t domid, uint32_t *out_static_max);
> >
> > Do you need this to be public, or did you just do it for consistency?
> >
>
> I did it for consistency, in case it's useful to library users. If you
> would rather not expose this I can make it internal.
In general I'm wary of adding to the API "just because". Refactoring an
internal function to be externally available when a requirement arises
is pretty easy, whereas changing an already public function because the
eventual requirement turned out not to be what we expected is harder.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max
2014-08-28 18:16 ` Ian Campbell
@ 2014-08-28 18:39 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-08-28 18:39 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Thu, Aug 28, 2014 at 07:16:03PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 12:31 +0100, Wei Liu wrote:
>
> > > > @@ -884,6 +891,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
> > > > int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
> > > > int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> > > > int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
> > > > +int libxl_get_memory_static_max(libxl_ctx *ctx, uint32_t domid, uint32_t *out_static_max);
> > >
> > > Do you need this to be public, or did you just do it for consistency?
> > >
> >
> > I did it for consistency, in case it's useful to library users. If you
> > would rather not expose this I can make it internal.
>
> In general I'm wary of adding to the API "just because". Refactoring an
> internal function to be externally available when a requirement arises
> is pretty easy, whereas changing an already public function because the
> eventual requirement turned out not to be what we expected is harder.
>
OK, let's make it an internal function.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (13 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 14/18] libxl: introduce libxl_get_memory_static_max Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 2:13 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink Wei Liu
` (3 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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>
---
tools/libxl/libxl.c | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl.h | 16 +++++
2 files changed, 213 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6083202..bcb6d87 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6116,6 +6116,203 @@ 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_data(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(ctx, domid, &memory);
+ if (rc) {
+ LOG(ERROR, "fail to get memory target for domain %d", domid);
+ goto out;
+ }
+ /* If the domain is HVM domain, target memory in xenstore is
+ * smaller than what user has asked for. The difference is
+ * video_memkb, so add it back. Otherwise domain rebuild will
+ * fail.
+ */
+ if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM)
+ d_config->b_info.target_memkb =
+ memory + d_config->b_info.video_memkb;
+
+ /* "static-max" */
+ rc = libxl_get_memory_static_max(ctx, domid, &memory);
+ 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 */
+
+ /* The MERGE macro implements following logic:
+ * 0. retrieve JSON
+ * 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, cnt, 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->cnt; 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->cnt - 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->cnt - 1)); \
+ \
+ /* rewind counters */ \
+ d_config->cnt--; \
+ i--; \
+ } \
+ } \
+ \
+ for (i = 0; i < num; i++) \
+ libxl_device_##type##_dispose(&p[i]); \
+ free(p); \
+ } while (0);
+
+ MERGE(nic, nics, num_nics, COMPARE_DEVID, {
+ libxl__update_config_nic(gc, dst, src);
+ });
+
+ MERGE(vtpm, vtpms, num_vtpms, COMPARE_DEVID, {
+ libxl__update_config_vtpm(gc, dst, src);
+ });
+
+ MERGE(pci, pcidevs, num_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, num_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_data(lock);
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7a0b58c..d12f166 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -358,6 +358,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_GET_MEMORY_STATIC_MAX
*
* If this is defined we have an API called libxl_get_memory_static_max
@@ -832,6 +840,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] 70+ messages in thread* Re: [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration
2014-07-30 18:23 ` [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
@ 2014-08-27 2:13 ` Ian Campbell
2014-08-28 11:39 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 2:13 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> + /* Domain name */
> + /* Domain UUID */
Wasn't there a new libxl__update_domain_configuration which does a bunch
of this for you already?
> + /* "target" */
> + rc = libxl_get_memory_target(ctx, domid, &memory);
> + if (rc) {
> + LOG(ERROR, "fail to get memory target for domain %d", domid);
> + goto out;
> + }
> + /* If the domain is HVM domain, target memory in xenstore is
> + * smaller than what user has asked for. The difference is
> + * video_memkb, so add it back. Otherwise domain rebuild will
> + * fail.
Ahem, lovely. Could you add a note at the point on creation where the
opposite happens pointing here for the benefit of anyone who touched
it...
> + */
> + if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM)
> + d_config->b_info.target_memkb =
> + memory + d_config->b_info.video_memkb;
> +
> + /* "static-max" */
> + rc = libxl_get_memory_static_max(ctx, domid, &memory);
This and the libxl_get_memory_target are implemented in terms of a very
handy internal helper that you added which returns both at the same
time ;-)
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration
2014-08-27 2:13 ` Ian Campbell
@ 2014-08-28 11:39 ` Wei Liu
2014-08-28 18:17 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 11:39 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 03:13:52AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > + /* Domain name */
>
> > + /* Domain UUID */
>
> Wasn't there a new libxl__update_domain_configuration which does a bunch
> of this for you already?
>
It updates UUID in JSON config but not domain name. I would still prefer
to use xenstore as primary reference for information like domain name
and UUID etc.
> > + /* "target" */
> > + rc = libxl_get_memory_target(ctx, domid, &memory);
> > + if (rc) {
> > + LOG(ERROR, "fail to get memory target for domain %d", domid);
> > + goto out;
> > + }
> > + /* If the domain is HVM domain, target memory in xenstore is
> > + * smaller than what user has asked for. The difference is
> > + * video_memkb, so add it back. Otherwise domain rebuild will
> > + * fail.
>
> Ahem, lovely. Could you add a note at the point on creation where the
> opposite happens pointing here for the benefit of anyone who touched
> it...
>
Sure, I will see what I can do.
> > + */
> > + if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM)
> > + d_config->b_info.target_memkb =
> > + memory + d_config->b_info.video_memkb;
> > +
> > + /* "static-max" */
> > + rc = libxl_get_memory_static_max(ctx, domid, &memory);
>
> This and the libxl_get_memory_target are implemented in terms of a very
> handy internal helper that you added which returns both at the same
> time ;-)
>
Yes I can use that internal function to do this.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration
2014-08-28 11:39 ` Wei Liu
@ 2014-08-28 18:17 ` Ian Campbell
2014-08-28 18:51 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-28 18:17 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 12:39 +0100, Wei Liu wrote:
> On Wed, Aug 27, 2014 at 03:13:52AM +0100, Ian Campbell wrote:
> > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > + /* Domain name */
> >
> > > + /* Domain UUID */
> >
> > Wasn't there a new libxl__update_domain_configuration which does a bunch
> > of this for you already?
> >
>
> It updates UUID in JSON config but not domain name. I would still prefer
> to use xenstore as primary reference for information like domain name
> and UUID etc.
Calling libxl__update_domain_configuration doesn't violate that though,
does it? IOW that function uses xenstore as the primary reference for
the UUID, so why not reuse it instead of open coding it a second time.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration
2014-08-28 18:17 ` Ian Campbell
@ 2014-08-28 18:51 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-08-28 18:51 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Thu, Aug 28, 2014 at 07:17:14PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 12:39 +0100, Wei Liu wrote:
> > On Wed, Aug 27, 2014 at 03:13:52AM +0100, Ian Campbell wrote:
> > > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > > + /* Domain name */
> > >
> > > > + /* Domain UUID */
> > >
> > > Wasn't there a new libxl__update_domain_configuration which does a bunch
> > > of this for you already?
> > >
> >
> > It updates UUID in JSON config but not domain name. I would still prefer
> > to use xenstore as primary reference for information like domain name
> > and UUID etc.
>
> Calling libxl__update_domain_configuration doesn't violate that though,
> does it? IOW that function uses xenstore as the primary reference for
> the UUID, so why not reuse it instead of open coding it a second time.
>
No it doesn't read from xenstore or look into hypervisor for most up to
date information, it only copies information from reference d_config to
another.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (14 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 15/18] libxl: introduce libxl_retrieve_domain_configuration Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-08-27 2:16 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 17/18] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
` (2 subsequent siblings)
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
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 d12f166..b124753 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
@@ -1228,6 +1235,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 3b9eade..f111cf1 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1975,6 +1975,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_data(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_data(lock);
+out:
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
/*
* Local variables:
* mode: C
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-07-30 18:23 ` [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink Wei Liu
@ 2014-08-27 2:16 ` Ian Campbell
2014-08-28 11:50 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-27 2:16 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> + CTX_LOCK;
> + lock = libxl__lock_domain_data(gc, domid);
> + if (!lock) {
This seems like a quirk which deserves commenting on since this lock
only protects a specific type of userdata, specifically the libxl-json
userdata.
Any app defined data is not actually protected by this lock in general.
I haven't yet seen the caller but it seems odd that the application
would be allowed to unlink the libxl-json file, so I'm not sure why this
function needs to worry about the lock (instead of insisting that the
libxl-internal caller deals with it)
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-27 2:16 ` Ian Campbell
@ 2014-08-28 11:50 ` Wei Liu
2014-08-28 18:20 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 11:50 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Aug 27, 2014 at 03:16:59AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > + CTX_LOCK;
> > + lock = libxl__lock_domain_data(gc, domid);
> > + if (!lock) {
>
> This seems like a quirk which deserves commenting on since this lock
> only protects a specific type of userdata, specifically the libxl-json
> userdata.
>
The original intent for that lock is to protect all application defined
data -- the name in registry is "libxl-lock" not "libxl-json-lock".
Though through out this series it's mostly used to protect libxl-json
data, it doesn't preclude us from using it to protect other type of
userdata.
> Any app defined data is not actually protected by this lock in general.
> I haven't yet seen the caller but it seems odd that the application
> would be allowed to unlink the libxl-json file, so I'm not sure why this
> function needs to worry about the lock (instead of insisting that the
> libxl-internal caller deals with it)
>
This function is specificly introduced for next patch.
During review last round we discussed how we should deal with "xl
config-udpate" command. The conclusion is that we still honour user
supplied config file and it has higher priority than libxl-json. We
would like to transform the config file supplied by user to libxl-json,
then remove that user supplied file, so that next time domain is
rebooted it always has the config tracked by libxl. Without this patch
xl has no way to unlink that file and it will still take effect during
next reboot, which is not what we want.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-28 11:50 ` Wei Liu
@ 2014-08-28 18:20 ` Ian Campbell
2014-08-28 19:04 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-28 18:20 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 12:50 +0100, Wei Liu wrote:
> On Wed, Aug 27, 2014 at 03:16:59AM +0100, Ian Campbell wrote:
> > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > + CTX_LOCK;
> > > + lock = libxl__lock_domain_data(gc, domid);
> > > + if (!lock) {
> >
> > This seems like a quirk which deserves commenting on since this lock
> > only protects a specific type of userdata, specifically the libxl-json
> > userdata.
> >
>
> The original intent for that lock is to protect all application defined
> data -- the name in registry is "libxl-lock" not "libxl-json-lock".
libxl-lock suggests to me that it locks things internal to libxl, not
"application userdata lock".
> Though through out this series it's mostly used to protect libxl-json
> data, it doesn't preclude us from using it to protect other type of
> userdata.
>
Except the locking functions aren't public, are they? and since the
interface uses carefd's I don't think it can be (easily) made public.
I guess what I'm saying is that if an app wants to lock accesses to its
userdata then it has to do that itself, it can't be done internally to
libxl.
> During review last round we discussed how we should deal with "xl
> config-udpate" command. The conclusion is that we still honour user
> supplied config file and it has higher priority than libxl-json. We
> would like to transform the config file supplied by user to libxl-json,
> then remove that user supplied file, so that next time domain is
> rebooted it always has the config tracked by libxl. Without this patch
> xl has no way to unlink that file and it will still take effect during
> next reboot, which is not what we want.
OK, so this is about removing the existing xl config, not the
libxl-json. I don't think this should take the libxl lock then -- that
lock doesn't protect the xl cfg userdata in any meaningful way AFAICT.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-28 18:20 ` Ian Campbell
@ 2014-08-28 19:04 ` Wei Liu
2014-08-28 19:31 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 19:04 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Thu, Aug 28, 2014 at 07:20:31PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 12:50 +0100, Wei Liu wrote:
> > On Wed, Aug 27, 2014 at 03:16:59AM +0100, Ian Campbell wrote:
> > > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > > + CTX_LOCK;
> > > > + lock = libxl__lock_domain_data(gc, domid);
> > > > + if (!lock) {
> > >
> > > This seems like a quirk which deserves commenting on since this lock
> > > only protects a specific type of userdata, specifically the libxl-json
> > > userdata.
> > >
> >
> > The original intent for that lock is to protect all application defined
> > data -- the name in registry is "libxl-lock" not "libxl-json-lock".
>
> libxl-lock suggests to me that it locks things internal to libxl, not
> "application userdata lock".
>
Yes it's internal.
> > Though through out this series it's mostly used to protect libxl-json
> > data, it doesn't preclude us from using it to protect other type of
> > userdata.
> >
>
> Except the locking functions aren't public, are they? and since the
> interface uses carefd's I don't think it can be (easily) made public.
>
Correct.
> I guess what I'm saying is that if an app wants to lock accesses to its
> userdata then it has to do that itself, it can't be done internally to
> libxl.
>
Application locking is one thing, but we still need to serialise libxl
access to those files, don't we? Any access to userdata store via libxl
API should be serialised. The reason is stated in previous patch "libxl:
properly lock user data store".
> > During review last round we discussed how we should deal with "xl
> > config-udpate" command. The conclusion is that we still honour user
> > supplied config file and it has higher priority than libxl-json. We
> > would like to transform the config file supplied by user to libxl-json,
> > then remove that user supplied file, so that next time domain is
> > rebooted it always has the config tracked by libxl. Without this patch
> > xl has no way to unlink that file and it will still take effect during
> > next reboot, which is not what we want.
>
> OK, so this is about removing the existing xl config, not the
> libxl-json. I don't think this should take the libxl lock then -- that
> lock doesn't protect the xl cfg userdata in any meaningful way AFAICT.
>
libxl-lock won't lock that file on application level but it is used to
prevent libxl threads from messing things up.
Does my explanation make sense? And, what do you have in mind for
designing this API?
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-28 19:04 ` Wei Liu
@ 2014-08-28 19:31 ` Ian Campbell
2014-08-28 20:27 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-28 19:31 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 20:04 +0100, Wei Liu wrote:
> Application locking is one thing, but we still need to serialise libxl
> access to those files, don't we? Any access to userdata store via libxl
> API should be serialised. The reason is stated in previous patch "libxl:
> properly lock user data store".
I may be confused here, so please correct me if I'm wrong...
Any individual userdata store/retrieve is atomic insofar as afterwards
there will be a consistent copy of the thing there, i.e. if there is a
race you will get one or the other copy of the data, but never a
mixture. Locking within the store/retrieve function neither helps nor
hinders this (since to the loser of the race the result is
indistinguishable from someone coming along 1s later and updating).
The locking is there to protect against read-modify-write cycles (e.g.
updating the config), which necessarily implies taking the lock before
the read and releasing it after the write -- i.e. at the application
layer (the libxl-lock being a kind of special in-libxl "application"
layer). Without the lock two entities racing in the r-m-w cycle can
result in updates being lost.
>
> > > During review last round we discussed how we should deal with "xl
> > > config-udpate" command. The conclusion is that we still honour user
> > > supplied config file and it has higher priority than libxl-json. We
> > > would like to transform the config file supplied by user to libxl-json,
> > > then remove that user supplied file, so that next time domain is
> > > rebooted it always has the config tracked by libxl. Without this patch
> > > xl has no way to unlink that file and it will still take effect during
> > > next reboot, which is not what we want.
> >
> > OK, so this is about removing the existing xl config, not the
> > libxl-json. I don't think this should take the libxl lock then -- that
> > lock doesn't protect the xl cfg userdata in any meaningful way AFAICT.
> >
>
> libxl-lock won't lock that file on application level but it is used to
> prevent libxl threads from messing things up.
>
> Does my explanation make sense? And, what do you have in mind for
> designing this API?
I don't think we need or want this API, is my point, the libxl-lock
should be used inside libxl to protect its updates of the cfg data, it
should not be part of the core userdata primitives.
Perhaps Ian J disagrees though, or maybe I am confused.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-28 19:31 ` Ian Campbell
@ 2014-08-28 20:27 ` Wei Liu
2014-08-28 20:44 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-28 20:27 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Thu, Aug 28, 2014 at 08:31:39PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 20:04 +0100, Wei Liu wrote:
> > Application locking is one thing, but we still need to serialise libxl
> > access to those files, don't we? Any access to userdata store via libxl
> > API should be serialised. The reason is stated in previous patch "libxl:
> > properly lock user data store".
>
> I may be confused here, so please correct me if I'm wrong...
>
> Any individual userdata store/retrieve is atomic insofar as afterwards
> there will be a consistent copy of the thing there, i.e. if there is a
> race you will get one or the other copy of the data, but never a
> mixture. Locking within the store/retrieve function neither helps nor
> hinders this (since to the loser of the race the result is
> indistinguishable from someone coming along 1s later and updating).
>
> The locking is there to protect against read-modify-write cycles (e.g.
> updating the config), which necessarily implies taking the lock before
> the read and releasing it after the write -- i.e. at the application
> layer (the libxl-lock being a kind of special in-libxl "application"
> layer). Without the lock two entities racing in the r-m-w cycle can
> result in updates being lost.
You're right on the r-m-w analysis. But the lock does more than that. In
this specific API family that manipulates userdata store, it also
ensures files won't disappear until other thread that holds the lock
finishes its job. Userdata vanishes under our feet is one abnormal state
we would like to avoid, userdata reappears after we delete it is another
abnormal state we would like to avoid. If we don't hold this lock for
this unlink API, we now have the chance to come into those two abnormal
states. Does this make sense?
> >
> > > > During review last round we discussed how we should deal with "xl
> > > > config-udpate" command. The conclusion is that we still honour user
> > > > supplied config file and it has higher priority than libxl-json. We
> > > > would like to transform the config file supplied by user to libxl-json,
> > > > then remove that user supplied file, so that next time domain is
> > > > rebooted it always has the config tracked by libxl. Without this patch
> > > > xl has no way to unlink that file and it will still take effect during
> > > > next reboot, which is not what we want.
> > >
> > > OK, so this is about removing the existing xl config, not the
> > > libxl-json. I don't think this should take the libxl lock then -- that
> > > lock doesn't protect the xl cfg userdata in any meaningful way AFAICT.
> > >
> >
> > libxl-lock won't lock that file on application level but it is used to
> > prevent libxl threads from messing things up.
> >
> > Does my explanation make sense? And, what do you have in mind for
> > designing this API?
>
> I don't think we need or want this API, is my point, the libxl-lock
> should be used inside libxl to protect its updates of the cfg data, it
> should not be part of the core userdata primitives.
>
OK, TBH I don't quite like this API either. If we don't provide a way
for xl to delete xl cfg userdata, what should we do with xl cfg? What do
you suggest to achieve the said behavior of "xl config-update"?
Wei.
> Perhaps Ian J disagrees though, or maybe I am confused.
>
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-28 20:27 ` Wei Liu
@ 2014-08-28 20:44 ` Ian Campbell
2014-08-29 10:37 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-08-28 20:44 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2014-08-28 at 21:27 +0100, Wei Liu wrote:
> On Thu, Aug 28, 2014 at 08:31:39PM +0100, Ian Campbell wrote:
> > On Thu, 2014-08-28 at 20:04 +0100, Wei Liu wrote:
> > > Application locking is one thing, but we still need to serialise libxl
> > > access to those files, don't we? Any access to userdata store via libxl
> > > API should be serialised. The reason is stated in previous patch "libxl:
> > > properly lock user data store".
> >
> > I may be confused here, so please correct me if I'm wrong...
> >
> > Any individual userdata store/retrieve is atomic insofar as afterwards
> > there will be a consistent copy of the thing there, i.e. if there is a
> > race you will get one or the other copy of the data, but never a
> > mixture. Locking within the store/retrieve function neither helps nor
> > hinders this (since to the loser of the race the result is
> > indistinguishable from someone coming along 1s later and updating).
> >
> > The locking is there to protect against read-modify-write cycles (e.g.
> > updating the config), which necessarily implies taking the lock before
> > the read and releasing it after the write -- i.e. at the application
> > layer (the libxl-lock being a kind of special in-libxl "application"
> > layer). Without the lock two entities racing in the r-m-w cycle can
> > result in updates being lost.
>
> You're right on the r-m-w analysis. But the lock does more than that. In
> this specific API family that manipulates userdata store, it also
> ensures files won't disappear until other thread that holds the lock
> finishes its job. Userdata vanishes under our feet is one abnormal state
> we would like to avoid, userdata reappears after we delete it is another
> abnormal state we would like to avoid. If we don't hold this lock for
> this unlink API, we now have the chance to come into those two abnormal
> states. Does this make sense?
Yes, it makes sense to lock removal against any r-m-w in another thread.
But I don't think it follows the libxl_userdata_unlink should take any
particular lock, including the libxl lock, that would be the
responsibility of the caller.
For libxl-json userdata manipulation, you don't have this issue because
you aren't using unlink, I don't think. If libxl is using unlink
internally then it should arrange to hold the lock while calling unlink,
just like for r-m-w.
For xl cfg userdata the lock which you are making libxl_userdata_unlink
touch is not used by xl when doing r-m-w operations of the data (if it
even does such) so it doesn't protect you against anything.
If xl is doing r-m-w then it needs its own lock, and it should also
arrange to hold that lock over the unlink. This shouldn't be the
libxl-lock, it should be some lock which belongs to xl and it needs to
be taken at the application level.
> OK, TBH I don't quite like this API either. If we don't provide a way
> for xl to delete xl cfg userdata, what should we do with xl cfg? What do
> you suggest to achieve the said behavior of "xl config-update"?
I hope the above makes the clearer, but either xl needs a lock of its
own or it doesn't, but in no case is this libxl's business...
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-28 20:44 ` Ian Campbell
@ 2014-08-29 10:37 ` Wei Liu
2014-09-03 12:12 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-08-29 10:37 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Thu, Aug 28, 2014 at 09:44:04PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 21:27 +0100, Wei Liu wrote:
> > On Thu, Aug 28, 2014 at 08:31:39PM +0100, Ian Campbell wrote:
> > > On Thu, 2014-08-28 at 20:04 +0100, Wei Liu wrote:
> > > > Application locking is one thing, but we still need to serialise libxl
> > > > access to those files, don't we? Any access to userdata store via libxl
> > > > API should be serialised. The reason is stated in previous patch "libxl:
> > > > properly lock user data store".
> > >
> > > I may be confused here, so please correct me if I'm wrong...
> > >
> > > Any individual userdata store/retrieve is atomic insofar as afterwards
> > > there will be a consistent copy of the thing there, i.e. if there is a
> > > race you will get one or the other copy of the data, but never a
> > > mixture. Locking within the store/retrieve function neither helps nor
> > > hinders this (since to the loser of the race the result is
> > > indistinguishable from someone coming along 1s later and updating).
> > >
> > > The locking is there to protect against read-modify-write cycles (e.g.
> > > updating the config), which necessarily implies taking the lock before
> > > the read and releasing it after the write -- i.e. at the application
> > > layer (the libxl-lock being a kind of special in-libxl "application"
> > > layer). Without the lock two entities racing in the r-m-w cycle can
> > > result in updates being lost.
> >
> > You're right on the r-m-w analysis. But the lock does more than that. In
> > this specific API family that manipulates userdata store, it also
> > ensures files won't disappear until other thread that holds the lock
> > finishes its job. Userdata vanishes under our feet is one abnormal state
> > we would like to avoid, userdata reappears after we delete it is another
> > abnormal state we would like to avoid. If we don't hold this lock for
> > this unlink API, we now have the chance to come into those two abnormal
> > states. Does this make sense?
>
I think I know where I got lost.
In my previous patch "libxl: properly lock user data store", I got your
ack. In that patch, public API like libxl_userdata_{store,retrieve} also
hold libxl-lock, while in this API you suggest not to hold this lock.
> Yes, it makes sense to lock removal against any r-m-w in another thread.
>
> But I don't think it follows the libxl_userdata_unlink should take any
> particular lock, including the libxl lock, that would be the
> responsibility of the caller.
>
Responsibility of the caller -- yes, this is true. One precondition is
that we have applications that only touches userdata that belongs to
itself, but in reality we don't. Applications might touch other userdata
files when they don't even know about it.
Libxl does unlink files. libxl__userdata_destroyall unlinks every
userdata files belonged to a particular domain. So consider if there's
no lock in these APIs:
Task 1 Task 2
Do random stuffs to domain Trying to shut down
reads "task1" userdata
observe domid
start domain destruction
delete all userdata
destroy domain
writes "task1" userdata
[ forbidden state: userdata leaked ]
Task1 and task2 need not to be the same application.
> For libxl-json userdata manipulation, you don't have this issue because
> you aren't using unlink, I don't think. If libxl is using unlink
> internally then it should arrange to hold the lock while calling unlink,
> just like for r-m-w.
>
> For xl cfg userdata the lock which you are making libxl_userdata_unlink
> touch is not used by xl when doing r-m-w operations of the data (if it
> even does such) so it doesn't protect you against anything.
>
Against file removal by other applications. Otherwise application might
try to unlink a file that doesn't exist.
> If xl is doing r-m-w then it needs its own lock, and it should also
> arrange to hold that lock over the unlink. This shouldn't be the
> libxl-lock, it should be some lock which belongs to xl and it needs to
> be taken at the application level.
>
>
> > OK, TBH I don't quite like this API either. If we don't provide a way
> > for xl to delete xl cfg userdata, what should we do with xl cfg? What do
> > you suggest to achieve the said behavior of "xl config-update"?
>
> I hope the above makes the clearer, but either xl needs a lock of its
> own or it doesn't, but in no case is this libxl's business...
>
Yes, I understand what your said. Application should lock file when it sees
fit. But this only works when other applications guarantee not to step
outside its own realm...
I should have mentioned libxl does unlink files earlier, sorry.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-08-29 10:37 ` Wei Liu
@ 2014-09-03 12:12 ` Ian Campbell
2014-09-03 14:10 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 12:12 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Fri, 2014-08-29 at 11:37 +0100, Wei Liu wrote:
> On Thu, Aug 28, 2014 at 09:44:04PM +0100, Ian Campbell wrote:
> > On Thu, 2014-08-28 at 21:27 +0100, Wei Liu wrote:
> > > On Thu, Aug 28, 2014 at 08:31:39PM +0100, Ian Campbell wrote:
> > > > On Thu, 2014-08-28 at 20:04 +0100, Wei Liu wrote:
> > > > > Application locking is one thing, but we still need to serialise libxl
> > > > > access to those files, don't we? Any access to userdata store via libxl
> > > > > API should be serialised. The reason is stated in previous patch "libxl:
> > > > > properly lock user data store".
> > > >
> > > > I may be confused here, so please correct me if I'm wrong...
> > > >
> > > > Any individual userdata store/retrieve is atomic insofar as afterwards
> > > > there will be a consistent copy of the thing there, i.e. if there is a
> > > > race you will get one or the other copy of the data, but never a
> > > > mixture. Locking within the store/retrieve function neither helps nor
> > > > hinders this (since to the loser of the race the result is
> > > > indistinguishable from someone coming along 1s later and updating).
> > > >
> > > > The locking is there to protect against read-modify-write cycles (e.g.
> > > > updating the config), which necessarily implies taking the lock before
> > > > the read and releasing it after the write -- i.e. at the application
> > > > layer (the libxl-lock being a kind of special in-libxl "application"
> > > > layer). Without the lock two entities racing in the r-m-w cycle can
> > > > result in updates being lost.
> > >
> > > You're right on the r-m-w analysis. But the lock does more than that. In
> > > this specific API family that manipulates userdata store, it also
> > > ensures files won't disappear until other thread that holds the lock
> > > finishes its job. Userdata vanishes under our feet is one abnormal state
> > > we would like to avoid, userdata reappears after we delete it is another
> > > abnormal state we would like to avoid. If we don't hold this lock for
> > > this unlink API, we now have the chance to come into those two abnormal
> > > states. Does this make sense?
> >
>
> I think I know where I got lost.
>
> In my previous patch "libxl: properly lock user data store", I got your
> ack. In that patch, public API like libxl_userdata_{store,retrieve} also
> hold libxl-lock, while in this API you suggest not to hold this lock.
>
> > Yes, it makes sense to lock removal against any r-m-w in another thread.
> >
> > But I don't think it follows the libxl_userdata_unlink should take any
> > particular lock, including the libxl lock, that would be the
> > responsibility of the caller.
> >
>
> Responsibility of the caller -- yes, this is true. One precondition is
> that we have applications that only touches userdata that belongs to
> itself, but in reality we don't. Applications might touch other userdata
> files when they don't even know about it.
Can they? How? To do this an application would have to pass some other
entities userid to a libxl_userdata function, which they would surely
know about.
I think it is up to the entity which defines the userdata "userid" to
also define the required locking when accessing that particular
userdata. By necessity I think that locking would need to be implemented
at the level of that entity, not at a lower level (e.g. libxl), since
the userdata API does not include a read-modify-write interface. A third
party which fiddles with someone else's userdata without following the
defined protocol would be buggy.
In the case of the new libxl-json userid libxl defines that you must
hold the libxl-lock, but that should be entirely internal to libxl and
not exposed via general userdata API functions, I think. It's somewhat
confusing in this case because libxl is both "the entity which defines
the userdata userid" and the thing which provides the generic userdata
accessor methods.
BTW I'm not sure why we need libxl-json and libxl-lock (I'd failed to
grok that they differed), rather than locking a userdata path with a
different wh parameter, e.g. "l" as I think Ian J suggested before.
>
> Libxl does unlink files. libxl__userdata_destroyall unlinks every
> userdata files belonged to a particular domain. So consider if there's
> no lock in these APIs:
>
> Task 1 Task 2
> Do random stuffs to domain Trying to shut down
> reads "task1" userdata
> observe domid
> start domain destruction
> delete all userdata
> destroy domain
> writes "task1" userdata
> [ forbidden state: userdata leaked ]
>
> Task1 and task2 need not to be the same application.
>
> > For libxl-json userdata manipulation, you don't have this issue because
> > you aren't using unlink, I don't think. If libxl is using unlink
> > internally then it should arrange to hold the lock while calling unlink,
> > just like for r-m-w.
> >
> > For xl cfg userdata the lock which you are making libxl_userdata_unlink
> > touch is not used by xl when doing r-m-w operations of the data (if it
> > even does such) so it doesn't protect you against anything.
> >
>
> Against file removal by other applications. Otherwise application might
> try to unlink a file that doesn't exist.
For this to help you would need a lock which could be held from before
the applications check that the userdata is present until after the
unlink, which I don't think can be done inside libxl_userdata_unlink,
can it?
>
> > If xl is doing r-m-w then it needs its own lock, and it should also
> > arrange to hold that lock over the unlink. This shouldn't be the
> > libxl-lock, it should be some lock which belongs to xl and it needs to
> > be taken at the application level.
> >
> >
> > > OK, TBH I don't quite like this API either. If we don't provide a way
> > > for xl to delete xl cfg userdata, what should we do with xl cfg? What do
> > > you suggest to achieve the said behavior of "xl config-update"?
> >
> > I hope the above makes the clearer, but either xl needs a lock of its
> > own or it doesn't, but in no case is this libxl's business...
> >
>
> Yes, I understand what your said. Application should lock file when it sees
> fit. But this only works when other applications guarantee not to step
> outside its own realm...
>
> I should have mentioned libxl does unlink files earlier, sorry.
libxl__userdata_destroyall does complicate things somewhat but I'm not
convinced that holding some libxl internal lock actual solves the
problem you are hoping to solve.
libxl__userdata_destroyall is only called during domain destruction. By
this point the application has in some sense already relinquished
control of the domain (and by extension the userdata)
It might be useful if we discuss this face to face, perhaps with a
whiteboard?
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-09-03 12:12 ` Ian Campbell
@ 2014-09-03 14:10 ` Wei Liu
2014-09-03 14:16 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-09-03 14:10 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
For the record, here is summary of the face-2-face discussion Ian and I
had.
The lock proposed in patch 2 should be renamed to "domain-data-lock" to
avoid making the false impression that it's related to libxl-json data.
For any specific application, it should hold a lock to protect its own
userdata if it cares, to protect against r-m-w by other threads.
With the introduction of libxl-json data, libxl becomes an application
of libxl itself, in the sense that libxl now does r-m-w to libxl-json
data internally. Hence it needs a lock (LockA).
The said lock, proposed in patch 2, is used to lock all types of user
data related to a specified domain. It protects userdata against domain
destruction (LockB).
I reused the lock in patch 2 as the application data lock for libxl-json
data. The lock serves two purposes:
1. to protect against r-m-w to libxl-json data
2. to protect against domain destruction
This double-duty lock is what confuses most. In theory LockA and LockB
can be different locks but there isn't much benefit in doing so in terms
of protecting libxl-json data.
In general, locking hierarchy should be
application userdata lock (if any)
---- (inside libxl)
libxl domain data lock
...
libxl domain data unlock
----
application userdata unlock
In the case of protecting libxl-json data, the application userdata lock
is the same one as libxl domain data lock, so the locking hierarchy is
preserved.
Wei.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-09-03 14:10 ` Wei Liu
@ 2014-09-03 14:16 ` Ian Campbell
2014-09-03 14:17 ` Wei Liu
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 14:16 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-09-03 at 15:10 +0100, Wei Liu wrote:
> For the record, here is summary of the face-2-face discussion Ian and I
> had.
Thanks for summarising!
> The lock proposed in patch 2 should be renamed to "domain-data-lock" to
Better "domain-userdata-lock" I think, to make it completely clear..
> avoid making the false impression that it's related to libxl-json data.
>
> For any specific application, it should hold a lock to protect its own
> userdata if it cares, to protect against r-m-w by other threads.
>
> With the introduction of libxl-json data, libxl becomes an application
> of libxl itself, in the sense that libxl now does r-m-w to libxl-json
> data internally. Hence it needs a lock (LockA).
>
> The said lock, proposed in patch 2, is used to lock all types of user
> data related to a specified domain. It protects userdata against domain
> destruction (LockB).
>
> I reused the lock in patch 2 as the application data lock for libxl-json
> data. The lock serves two purposes:
> 1. to protect against r-m-w to libxl-json data
> 2. to protect against domain destruction
>
> This double-duty lock is what confuses most. In theory LockA and LockB
> can be different locks but there isn't much benefit in doing so in terms
> of protecting libxl-json data.
>
> In general, locking hierarchy should be
> application userdata lock (if any)
> ---- (inside libxl)
> libxl domain data lock
> ...
> libxl domain data unlock
> ----
> application userdata unlock
>
> In the case of protecting libxl-json data, the application userdata lock
> is the same one as libxl domain data lock, so the locking hierarchy is
> preserved.
>
> Wei.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink
2014-09-03 14:16 ` Ian Campbell
@ 2014-09-03 14:17 ` Wei Liu
0 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-09-03 14:17 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, Wei Liu, xen-devel
On Wed, Sep 03, 2014 at 03:16:01PM +0100, Ian Campbell wrote:
> On Wed, 2014-09-03 at 15:10 +0100, Wei Liu wrote:
> > For the record, here is summary of the face-2-face discussion Ian and I
> > had.
>
> Thanks for summarising!
>
> > The lock proposed in patch 2 should be renamed to "domain-data-lock" to
>
> Better "domain-userdata-lock" I think, to make it completely clear..
>
Oh sure.
Wei.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 17/18] xl: use libxl_retrieve_domain_configuration and JSON format
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (15 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-09-03 12:57 ` Ian Campbell
2014-07-30 18:23 ` [PATCH v2 18/18] xl: long output of "list" command now contains Dom0 information Wei Liu
2014-08-12 16:17 ` [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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>
---
docs/man/xl.pod.1 | 4 ++
tools/libxl/xl_cmdimpl.c | 124 ++++++++++++++++++++++++++++-----------------
tools/libxl/xl_cmdtable.c | 4 +-
3 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..23add95 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -187,6 +187,10 @@ 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.
+Libxl now 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 01bce2f..c9c45bf 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. */
@@ -1699,21 +1701,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,
@@ -1721,7 +1743,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)
{
@@ -1779,13 +1800,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:
@@ -1982,6 +2002,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;
@@ -2036,7 +2057,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",
@@ -2064,7 +2085,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;
@@ -2104,6 +2127,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"
@@ -2111,12 +2135,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) {
@@ -2184,14 +2214,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)
@@ -2248,9 +2270,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. */
@@ -2287,12 +2307,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.
@@ -3071,9 +3085,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;
@@ -3094,24 +3106,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;
}
@@ -3296,22 +3302,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 parse overridden config file\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,
@@ -3339,6 +3364,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 */
@@ -4348,6 +4375,9 @@ int main_config_update(int argc, char **argv)
exit(1);
}
+ fprintf(stderr, "WARN: libxl 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 4279b9f..9bc0bca 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."
+ "WARN: libxl 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] 70+ messages in thread* Re: [PATCH v2 17/18] xl: use libxl_retrieve_domain_configuration and JSON format
2014-07-30 18:23 ` [PATCH v2 17/18] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
@ 2014-09-03 12:57 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-09-03 12:57 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 30bd4bf..23add95 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -187,6 +187,10 @@ 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.
>
> +Libxl now has improved capabilities to handle dynamic domain configuration
How about "Since Xen 4.5 xl has improved ..."?
> + config_c = libxl_domain_config_to_json(ctx, &d_config);
> + if (!config_c) {
> + fprintf(stderr, "unable to parse overridden config file\n");
I don't think this message matches the failure, does it? It's a failure
to convert the config to JSON I think?
> @@ -4348,6 +4375,9 @@ int main_config_update(int argc, char **argv)
> exit(1);
> }
>
> + fprintf(stderr, "WARN: libxl now has better capability to manage domain configuration, "
> + "avoid using this command when possible\n");
I think you can say "xl" in this context, the user of xl doesn't care
that it happens to be due to better scaffolding in the library.
> +
> domid = find_domain(argv[1]);
> argc--; argv++;
>
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 4279b9f..9bc0bca 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."
> + "WARN: libxl now has better capability to manage domain configuration, "
> + "avoid using this command when possible",
Same.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 18/18] xl: long output of "list" command now contains Dom0 information
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (16 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 17/18] xl: use libxl_retrieve_domain_configuration and JSON format Wei Liu
@ 2014-07-30 18:23 ` Wei Liu
2014-09-03 12:50 ` Ian Campbell
2014-08-12 16:17 ` [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
18 siblings, 1 reply; 70+ messages in thread
From: Wei Liu @ 2014-07-30 18:23 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>
---
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 c9c45bf..a46f528 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3107,9 +3107,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] 70+ messages in thread* Re: [PATCH v2 00/18] libxl: synchronise domain configuration
2014-07-30 18:23 [PATCH v2 00/18] libxl: synchronise domain configuration Wei Liu
` (17 preceding siblings ...)
2014-07-30 18:23 ` [PATCH v2 18/18] xl: long output of "list" command now contains Dom0 information Wei Liu
@ 2014-08-12 16:17 ` Wei Liu
18 siblings, 0 replies; 70+ messages in thread
From: Wei Liu @ 2014-08-12 16:17 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, ian.jackson, ian.campbell
Ping?
^ permalink raw reply [flat|nested] 70+ messages in thread