* [Xen-devel] [PATCH v5 1/7] libxl: add definition of INVALID_DOMID to the API
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 2/7] libxl_create: make 'soft reset' explicit Paul Durrant
` (6 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
Currently both xl and libxl have internal definitions of INVALID_DOMID
which happen to be identical. However, for the purposes of describing the
behaviour of libxl_domain_create_new/restore() it is useful to have a
specified invalid value for a domain id.
This patch therefore moves the libxl definition from libxl_internal.h to
libxl.h and removes the internal definition from xl_utils.h. The hardcoded
'-1' passed back via domcreate_complete() is then updated to INVALID_DOMID
and comment above libxl_domain_create_new/restore() is accordingly
modified.
NOTE: The value of INVALID_DOMID (~0) is distinct from the hypervisor's
DOMID_INVALID. This patch preserves that value.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
v5:
- Expand commit comment
---
tools/libxl/libxl.h | 4 +++-
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_internal.h | 1 -
tools/xl/xl_utils.h | 2 --
4 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 54abb9db1f..18c1a2d6bf 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1527,9 +1527,11 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
/* domain related functions */
+#define INVALID_DOMID ~0
+
/* If the result is ERROR_ABORTED, the domain may or may not exist
* (in a half-created state). *domid will be valid and will be the
- * domain id, or -1, as appropriate */
+ * domain id, or INVALID_DOMID, as appropriate */
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
uint32_t *domid,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 32d45dcef0..bc425fee32 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1773,7 +1773,7 @@ static void domcreate_complete(libxl__egc *egc,
libxl__domain_destroy(egc, &dcs->dds);
return;
}
- dcs->guest_domid = -1;
+ dcs->guest_domid = INVALID_DOMID;
}
dcs->callback(egc, dcs, rc, dcs->guest_domid);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2555aa4575..72290c6f28 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -121,7 +121,6 @@
#define STUBDOM_SPECIAL_CONSOLES 3
#define TAP_DEVICE_SUFFIX "-emu"
#define DOMID_XS_PATH "domid"
-#define INVALID_DOMID ~0
#define PVSHIM_BASENAME "xen-shim"
#define PVSHIM_CMDLINE "pv-shim console=xen,pv"
diff --git a/tools/xl/xl_utils.h b/tools/xl/xl_utils.h
index 7b9ccca30a..d98b419f10 100644
--- a/tools/xl/xl_utils.h
+++ b/tools/xl/xl_utils.h
@@ -52,8 +52,6 @@
#define STR_SKIP_PREFIX( a, b ) \
( STR_HAS_PREFIX(a, b) ? ((a) += strlen(b), 1) : 0 )
-#define INVALID_DOMID ~0
-
#define LOG(_f, _a...) dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
/*
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Xen-devel] [PATCH v5 2/7] libxl_create: make 'soft reset' explicit
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 1/7] libxl: add definition of INVALID_DOMID to the API Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 3/7] libxl: generalise libxl__domain_userdata_lock() Paul Durrant
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
The 'soft reset' code path in libxl__domain_make() is currently taken if a
valid domid is passed into the function. A subsequent patch will enable
higher levels of the toolstack to determine the domid of newly created or
restored domains and therefore this criteria for choosing 'soft reset'
will no longer be usable.
This patch adds an extra boolean option to libxl__domain_make() to specify
whether it is being invoked in soft reset context and appropriately
modifies callers to choose the right value. To facilitate this, a new
'soft_reset' boolean field is added to struct libxl__domain_create_state
and the 'domid_soft_reset' field is renamed to 'domid' in anticipation of
its wider remit. For the moment do_domain_create() will always set
domid to INVALID_DOMID and hence we can add an assertion into
libxl__domain_create() that, if it is not called in soft reset context,
the passed in domid is exactly that value.
Whilst in the neighbourhood, some checks of 'restore_fd > -1' have been
replaced by 'restore_fd >= 0' to be more conventional and consistent with
checks of 'restore_fd < 0'.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
tools/libxl/libxl_create.c | 56 ++++++++++++++++++++++--------------
tools/libxl/libxl_dm.c | 2 +-
tools/libxl/libxl_internal.h | 5 ++--
3 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index bc425fee32..1835a5502c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -538,7 +538,7 @@ out:
int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl__domain_build_state *state,
- uint32_t *domid)
+ uint32_t *domid, bool soft_reset)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
int ret, rc, nb_vm;
@@ -555,14 +555,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl_domain_create_info *info = &d_config->c_info;
libxl_domain_build_info *b_info = &d_config->b_info;
+ assert(soft_reset || *domid == INVALID_DOMID);
+
uuid_string = libxl__uuid2string(gc, info->uuid);
if (!uuid_string) {
rc = ERROR_NOMEM;
goto out;
}
- /* Valid domid here means we're soft resetting. */
- if (!libxl_domid_valid_guest(*domid)) {
+ if (!soft_reset) {
struct xen_domctl_createdomain create = {
.ssidref = info->ssidref,
.max_vcpus = b_info->max_vcpus,
@@ -611,6 +612,14 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
goto out;
}
+ /*
+ * If soft_reset is set the the domid will have been valid on entry.
+ * If it was not set then xc_domain_create() should have assigned a
+ * valid value. Either way, if we reach this point, domid should be
+ * valid.
+ */
+ assert(libxl_domid_valid_guest(*domid));
+
ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
if (ret < 0) {
LOGED(ERROR, *domid, "domain move fail");
@@ -1091,13 +1100,14 @@ static void initiate_domain_create(libxl__egc *egc,
libxl_domain_config *const d_config = dcs->guest_config;
const int restore_fd = dcs->restore_fd;
- domid = dcs->domid_soft_reset;
+ domid = dcs->domid;
libxl__domain_build_state_init(&dcs->build_state);
ret = libxl__domain_config_setdefault(gc,d_config,domid);
if (ret) goto error_out;
- ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid);
+ ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid,
+ dcs->soft_reset);
if (ret) {
LOGD(ERROR, domid, "cannot make domain: %d", ret);
dcs->guest_domid = domid;
@@ -1141,7 +1151,7 @@ static void initiate_domain_create(libxl__egc *egc,
if (ret)
goto error_out;
- if (restore_fd >= 0 || dcs->domid_soft_reset != INVALID_DOMID) {
+ if (restore_fd >= 0 || dcs->soft_reset) {
LOGD(DEBUG, domid, "restoring, not running bootloader");
domcreate_bootloader_done(egc, &dcs->bl, 0);
} else {
@@ -1217,7 +1227,7 @@ static void domcreate_bootloader_done(libxl__egc *egc,
dcs->sdss.dm.callback = domcreate_devmodel_started;
dcs->sdss.callback = domcreate_devmodel_started;
- if (restore_fd < 0 && dcs->domid_soft_reset == INVALID_DOMID) {
+ if (restore_fd < 0 && !dcs->soft_reset) {
rc = libxl__domain_build(gc, d_config, domid, state);
domcreate_rebuild_done(egc, dcs, rc);
return;
@@ -1827,7 +1837,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
libxl_domain_config_copy(ctx, &cdcs->dcs.guest_config_saved, d_config);
cdcs->dcs.restore_fd = cdcs->dcs.libxc_fd = restore_fd;
cdcs->dcs.send_back_fd = send_back_fd;
- if (restore_fd > -1) {
+ if (restore_fd >= 0) {
cdcs->dcs.restore_params = *params;
rc = libxl__fd_flags_modify_save(gc, cdcs->dcs.restore_fd,
~(O_NONBLOCK|O_NDELAY), 0,
@@ -1835,7 +1845,8 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
if (rc < 0) goto out_err;
}
cdcs->dcs.callback = domain_create_cb;
- cdcs->dcs.domid_soft_reset = INVALID_DOMID;
+ cdcs->dcs.domid = INVALID_DOMID;
+ cdcs->dcs.soft_reset = false;
if (cdcs->dcs.restore_params.checkpointed_stream ==
LIBXL_CHECKPOINTED_STREAM_COLO) {
@@ -1905,7 +1916,7 @@ static void soft_reset_dm_suspended(libxl__egc *egc,
int rc);
static int do_domain_soft_reset(libxl_ctx *ctx,
libxl_domain_config *d_config,
- uint32_t domid_soft_reset,
+ uint32_t domid,
const libxl_asyncop_how *ao_how,
const libxl_asyncprogress_how
*aop_console_how)
@@ -1933,15 +1944,16 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
libxl_domain_config_copy(ctx, &srs->cdcs.dcs.guest_config_saved,
d_config);
cdcs->dcs.restore_fd = -1;
- cdcs->dcs.domid_soft_reset = domid_soft_reset;
+ cdcs->dcs.domid = domid;
+ cdcs->dcs.soft_reset = true;
cdcs->dcs.callback = domain_create_cb;
libxl__ao_progress_gethow(&srs->cdcs.dcs.aop_console_how,
aop_console_how);
cdcs->domid_out = &domid_out;
- dom_path = libxl__xs_get_dompath(gc, domid_soft_reset);
+ dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path) {
- LOGD(ERROR, domid_soft_reset, "failed to read domain path");
+ LOGD(ERROR, domid, "failed to read domain path");
rc = ERROR_FAIL;
goto out;
}
@@ -1950,7 +1962,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
GCSPRINTF("%s/store/ring-ref", dom_path),
&xs_store_mfn);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to read store/ring-ref.");
+ LOGD(ERROR, domid, "failed to read store/ring-ref.");
goto out;
}
state->store_mfn = xs_store_mfn ? atol(xs_store_mfn): 0;
@@ -1959,7 +1971,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
GCSPRINTF("%s/console/ring-ref", dom_path),
&xs_console_mfn);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to read console/ring-ref.");
+ LOGD(ERROR, domid, "failed to read console/ring-ref.");
goto out;
}
state->console_mfn = xs_console_mfn ? atol(xs_console_mfn): 0;
@@ -1968,20 +1980,20 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
GCSPRINTF("%s/console/tty", dom_path),
&console_tty);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to read console/tty.");
+ LOGD(ERROR, domid, "failed to read console/tty.");
goto out;
}
state->console_tty = libxl__strdup(gc, console_tty);
dss->ao = ao;
- dss->domid = dss->dsps.domid = domid_soft_reset;
+ dss->domid = dss->dsps.domid = domid;
dss->dsps.dm_savefile = GCSPRINTF(LIBXL_DEVICE_MODEL_SAVE_FILE".%d",
- domid_soft_reset);
+ domid);
rc = libxl__save_emulator_xenstore_data(dss, &srs->toolstack_buf,
&srs->toolstack_len);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to save toolstack record.");
+ LOGD(ERROR, domid, "failed to save toolstack record.");
goto out;
}
@@ -2010,10 +2022,10 @@ static void soft_reset_dm_suspended(libxl__egc *egc,
* xenstore again with probably different store/console/...
* channels.
*/
- xs_release_domain(CTX->xsh, cdcs->dcs.domid_soft_reset);
+ xs_release_domain(CTX->xsh, cdcs->dcs.domid);
srs->dds.ao = ao;
- srs->dds.domid = cdcs->dcs.domid_soft_reset;
+ srs->dds.domid = cdcs->dcs.domid;
srs->dds.callback = domain_soft_reset_cb;
srs->dds.soft_reset = true;
libxl__domain_destroy(egc, &srs->dds);
@@ -2029,7 +2041,7 @@ static void domain_create_cb(libxl__egc *egc,
*cdcs->domid_out = domid;
- if (dcs->restore_fd > -1) {
+ if (dcs->restore_fd >= 0) {
flrc = libxl__fd_flags_restore(gc,
dcs->restore_fd, dcs->restore_fdfl);
/*
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e92e412c1b..f758daf3b6 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2193,7 +2193,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
/* fixme: this function can leak the stubdom if it fails */
ret = libxl__domain_make(gc, dm_config, stubdom_state,
- &sdss->pvqemu.guest_domid);
+ &sdss->pvqemu.guest_domid, false);
if (ret)
goto out;
uint32_t dm_domid = sdss->pvqemu.guest_domid;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 72290c6f28..f2efdedfba 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1972,7 +1972,7 @@ _hidden void libxl__exec(libxl__gc *gc, int stdinfd, int stdoutfd,
_hidden int libxl__domain_make(libxl__gc *gc,
libxl_domain_config *d_config,
libxl__domain_build_state *state,
- uint32_t *domid);
+ uint32_t *domid, bool soft_reset);
_hidden int libxl__domain_build(libxl__gc *gc,
libxl_domain_config *d_config,
@@ -4158,7 +4158,8 @@ struct libxl__domain_create_state {
int restore_fdfl; /* original flags of restore_fd */
int send_back_fd;
libxl_domain_restore_params restore_params;
- uint32_t domid_soft_reset;
+ uint32_t domid;
+ bool soft_reset;
libxl__domain_create_cb *callback;
libxl_asyncprogress_how aop_console_how;
/* private to domain_create */
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Xen-devel] [PATCH v5 3/7] libxl: generalise libxl__domain_userdata_lock()
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 1/7] libxl: add definition of INVALID_DOMID to the API Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 2/7] libxl_create: make 'soft reset' explicit Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids Paul Durrant
` (4 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
This function implements a file-based lock with a file name generated
from a domid.
This patch splits it into two, generalising the core of the locking code
into a new libxl__lock_file() function which operates on a specified file,
leaving just the file name generation in libxl__domain_userdata_lock().
This patch also generalises libxl__unlock_domain_userdata() to
libxl__unlock_file() and modifies all call-sites.
Suggested-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
v4:
- New in v4.
---
tools/libxl/libxl_create.c | 4 +--
tools/libxl/libxl_device.c | 4 +--
tools/libxl/libxl_disk.c | 12 ++++----
tools/libxl/libxl_dom.c | 12 ++++----
tools/libxl/libxl_domain.c | 14 ++++-----
tools/libxl/libxl_internal.c | 55 +++++++++++++++++++++---------------
tools/libxl/libxl_internal.h | 10 ++++---
tools/libxl/libxl_mem.c | 8 +++---
tools/libxl/libxl_pci.c | 4 +--
tools/libxl/libxl_usb.c | 8 +++---
10 files changed, 72 insertions(+), 59 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1835a5502c..3a7364e2ac 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1755,7 +1755,7 @@ static void domcreate_complete(libxl__egc *egc,
bool retain_domain = !rc || rc == ERROR_ABORTED;
if (retain_domain) {
- libxl__domain_userdata_lock *lock;
+ libxl__flock *lock;
/* Note that we hold CTX lock at this point so only need to
* take data store lock
@@ -1769,7 +1769,7 @@ static void domcreate_complete(libxl__egc *egc,
(gc, dcs->guest_domid, d_config_saved);
if (!rc)
rc = cfg_rc;
- libxl__unlock_domain_userdata(lock);
+ libxl__unlock_file(lock);
}
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 9d05d2fd13..0381c5d509 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1850,7 +1850,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
xs_transaction_t t = XBT_NULL;
libxl_domain_config d_config;
void *type_saved;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
int rc;
libxl_domain_config_init(&d_config);
@@ -1946,7 +1946,7 @@ void libxl__device_add_async(libxl__egc *egc, uint32_t domid,
out:
libxl__xs_transaction_abort(gc, &t);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
dt->dispose(type_saved);
libxl_domain_config_dispose(&d_config);
aodev->rc = rc;
diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index a463334130..ddc1eec176 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -245,7 +245,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
xs_transaction_t t = XBT_NULL;
libxl_domain_config d_config;
libxl_device_disk disk_saved;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
libxl_domain_config_init(&d_config);
libxl_device_disk_init(&disk_saved);
@@ -436,7 +436,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
out:
libxl__xs_transaction_abort(gc, &t);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
libxl_device_disk_dispose(&disk_saved);
libxl_domain_config_dispose(&d_config);
aodev->rc = rc;
@@ -794,7 +794,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
{
EGC_GC;
libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
- libxl__domain_userdata_lock *data_lock = NULL;
+ libxl__flock *data_lock = NULL;
libxl__device device;
const char *be_path, *libxl_path;
flexarray_t *empty = NULL;
@@ -896,7 +896,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
out:
libxl__xs_transaction_abort(gc, &t);
libxl_domain_config_dispose(&d_config);
- if (data_lock) libxl__unlock_domain_userdata(data_lock);
+ if (data_lock) libxl__unlock_file(data_lock);
if (rc) {
cdrom_insert_done(egc, cis, rc); /* must be last */
} else if (!has_callback) {
@@ -951,7 +951,7 @@ static void cdrom_insert_inserted(libxl__egc *egc,
{
EGC_GC;
libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
- libxl__domain_userdata_lock *data_lock = NULL;
+ libxl__flock *data_lock = NULL;
libxl_domain_config d_config;
flexarray_t *insert = NULL;
xs_transaction_t t = XBT_NULL;
@@ -1029,7 +1029,7 @@ static void cdrom_insert_inserted(libxl__egc *egc,
out:
libxl__xs_transaction_abort(gc, &t);
libxl_domain_config_dispose(&d_config);
- if (data_lock) libxl__unlock_domain_userdata(data_lock);
+ if (data_lock) libxl__unlock_file(data_lock);
cdrom_insert_done(egc, cis, rc); /* must be last */
}
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 573c63692b..d9ada8a422 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1347,7 +1347,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
{
GC_INIT(ctx);
int rc;
- libxl__domain_userdata_lock *lock;
+ libxl__flock *lock;
CTX_LOCK;
lock = libxl__lock_domain_userdata(gc, domid);
@@ -1359,7 +1359,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
rc = libxl__userdata_store(gc, domid, userdata_userid,
data, datalen);
- libxl__unlock_domain_userdata(lock);
+ libxl__unlock_file(lock);
out:
CTX_UNLOCK;
@@ -1408,7 +1408,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
{
GC_INIT(ctx);
int rc;
- libxl__domain_userdata_lock *lock;
+ libxl__flock *lock;
CTX_LOCK;
lock = libxl__lock_domain_userdata(gc, domid);
@@ -1421,7 +1421,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
data_r, datalen_r);
- libxl__unlock_domain_userdata(lock);
+ libxl__unlock_file(lock);
out:
CTX_UNLOCK;
GC_FREE;
@@ -1435,7 +1435,7 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
CTX_LOCK;
int rc;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
const char *filename;
lock = libxl__lock_domain_userdata(gc, domid);
@@ -1458,7 +1458,7 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
rc = 0;
out:
if (lock)
- libxl__unlock_domain_userdata(lock);
+ libxl__unlock_file(lock);
CTX_UNLOCK;
GC_FREE;
return rc;
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index b59cc65750..973fc1434d 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1278,7 +1278,7 @@ static void devices_destroy_cb(libxl__egc *egc,
uint32_t domid = dis->domid;
char *dom_path;
char *vm_path;
- libxl__domain_userdata_lock *lock;
+ libxl__flock *lock;
dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path) {
@@ -1308,7 +1308,7 @@ static void devices_destroy_cb(libxl__egc *egc,
}
libxl__userdata_destroyall(gc, domid);
- libxl__unlock_domain_userdata(lock);
+ libxl__unlock_file(lock);
/* Clean up qemu-save and qemu-resume files. They are
* intermediate files created by libxc. Unfortunately they
@@ -1917,7 +1917,7 @@ static void retrieve_domain_configuration_lock_acquired(
retrieve_domain_configuration_state *rdcs =
CONTAINER_OF(devlock, *rdcs, devlock);
STATE_AO_GC(rdcs->qmp.ao);
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
bool has_callback = false;
/* Convenience aliases */
@@ -1939,7 +1939,7 @@ static void retrieve_domain_configuration_lock_acquired(
goto out;
}
- libxl__unlock_domain_userdata(lock);
+ libxl__unlock_file(lock);
lock = NULL;
/* We start by querying QEMU, if it is running, for its cpumap as this
@@ -1964,7 +1964,7 @@ static void retrieve_domain_configuration_lock_acquired(
}
out:
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
if (!has_callback)
retrieve_domain_configuration_end(egc, rdcs, rc);
}
@@ -1998,7 +1998,7 @@ static void retrieve_domain_configuration_end(libxl__egc *egc,
retrieve_domain_configuration_state *rdcs, int rc)
{
STATE_AO_GC(rdcs->qmp.ao);
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
/* Convenience aliases */
libxl_domain_config *const d_config = rdcs->d_config;
@@ -2205,7 +2205,7 @@ static void retrieve_domain_configuration_end(libxl__egc *egc,
out:
libxl__ev_slowlock_unlock(gc, &rdcs->devlock);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
libxl_bitmap_dispose(&rdcs->qemuu_cpus);
libxl__ev_qmp_dispose(gc, &rdcs->qmp);
libxl__ev_time_deregister(gc, &rdcs->timeout);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ba5637358e..211236dc99 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -400,26 +400,22 @@ int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
/* Portability note: this lock utilises flock(2) so a proper implementation of
* flock(2) is required.
*/
-libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
- uint32_t domid)
+libxl__flock *libxl__lock_file(libxl__gc *gc, const char *lockfile)
{
- libxl__domain_userdata_lock *lock = NULL;
- const char *lockfile;
+ libxl__flock *lock;
int fd;
struct stat stab, fstab;
- lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
- if (!lockfile) goto out;
-
- lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
+ lock = libxl__zalloc(NOGC, sizeof(libxl__flock));
lock->path = libxl__strdup(NOGC, lockfile);
while (true) {
libxl__carefd_begin();
fd = open(lockfile, O_RDWR|O_CREAT, 0666);
if (fd < 0)
- LOGED(ERROR, domid,
- "cannot open lockfile %s, errno=%d", lockfile, errno);
+ LOGE(ERROR,
+ "cannot open lockfile %s, errno=%d",
+ lockfile, errno);
lock->carefd = libxl__carefd_opened(CTX, fd);
if (fd < 0) goto out;
@@ -433,21 +429,21 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
continue;
default:
/* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
- LOGED(ERROR, domid,
- "unexpected error while trying to lock %s, fd=%d, errno=%d",
+ LOGE(ERROR,
+ "unexpected error while trying to lock %s, fd=%d, errno=%d",
lockfile, fd, errno);
goto out;
}
}
if (fstat(fd, &fstab)) {
- LOGED(ERROR, domid, "cannot fstat %s, fd=%d, errno=%d",
+ LOGE(ERROR, "cannot fstat %s, fd=%d, errno=%d",
lockfile, fd, errno);
goto out;
}
if (stat(lockfile, &stab)) {
if (errno != ENOENT) {
- LOGED(ERROR, domid, "cannot stat %s, errno=%d", lockfile, errno);
+ LOGE(ERROR, "cannot stat %s, errno=%d", lockfile, errno);
goto out;
}
} else {
@@ -458,20 +454,14 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
libxl__carefd_close(lock->carefd);
}
- /* Check the domain is still there, if not we should release the
- * lock and clean up.
- */
- if (libxl_domain_info(CTX, NULL, domid))
- goto out;
-
return lock;
out:
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
return NULL;
}
-void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+void libxl__unlock_file(libxl__flock *lock)
{
/* It's important to unlink the file before closing fd to avoid
* the following race (if close before unlink):
@@ -493,6 +483,27 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
free(lock);
}
+libxl__flock *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
+{
+ const char *lockfile;
+ libxl__flock *lock;
+
+ lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
+ if (!lockfile) return NULL;
+
+ lock = libxl__lock_file(gc, lockfile);
+
+ /* Check the domain is still there, if not we should release the
+ * lock and clean up.
+ */
+ if (libxl_domain_info(CTX, NULL, domid)) {
+ libxl__unlock_file(lock);
+ return NULL;
+ }
+
+ return lock;
+}
+
int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
libxl_domain_config *d_config)
{
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f2efdedfba..dd3c08bc14 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4651,11 +4651,13 @@ int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
typedef struct {
libxl__carefd *carefd;
char *path; /* path of the lock file itself */
-} libxl__domain_userdata_lock;
+} libxl__flock;
/* The CTX_LOCK must be held around uses of this lock */
-libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
- uint32_t domid);
-void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock);
+
+libxl__flock *libxl__lock_file(libxl__gc *gc, const char *filename);
+void libxl__unlock_file(libxl__flock *lock);
+
+libxl__flock *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid);
/*
* Retrieve / store domain configuration from / to libxl private
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 7c01fac7e5..bc7b95aa74 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -30,7 +30,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
uint64_t memorykb, size;
char *dompath = libxl__xs_get_dompath(gc, domid);
int rc = 1;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
libxl_domain_config d_config;
libxl_domain_config_init(&d_config);
@@ -85,7 +85,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
rc = 0;
out:
libxl_domain_config_dispose(&d_config);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
CTX_UNLOCK;
GC_FREE;
return rc;
@@ -184,7 +184,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
libxl_dominfo ptr;
char *uuid;
xs_transaction_t t;
- libxl__domain_userdata_lock *lock;
+ libxl__flock *lock;
libxl_domain_config d_config;
libxl_domain_config_init(&d_config);
@@ -338,7 +338,7 @@ out:
out_no_transaction:
libxl_domain_config_dispose(&d_config);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
CTX_UNLOCK;
GC_FREE;
return rc;
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 664d74c478..f91bce07ec 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -125,7 +125,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc,
xs_transaction_t t = XBT_NULL;
int rc;
libxl_domain_config d_config;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
/* Stubdomain doesn't have own config. */
@@ -195,7 +195,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc,
out:
libxl__xs_transaction_abort(gc, &t);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
if (!is_stubdomain)
libxl_domain_config_dispose(&d_config);
return rc;
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index da5e3708e6..171bb04439 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -202,7 +202,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
int i, rc;
libxl_domain_config d_config;
libxl_device_usbctrl usbctrl_saved;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
libxl_domain_config_init(&d_config);
libxl_device_usbctrl_init(&usbctrl_saved);
@@ -291,7 +291,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
out:
libxl__xs_transaction_abort(gc, &t);
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
libxl_device_usbctrl_dispose(&usbctrl_saved);
libxl_domain_config_dispose(&d_config);
return rc;
@@ -1266,7 +1266,7 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
xs_transaction_t t = XBT_NULL;
libxl_domain_config d_config;
libxl_device_usbdev usbdev_saved;
- libxl__domain_userdata_lock *lock = NULL;
+ libxl__flock *lock = NULL;
libxl_domain_config_init(&d_config);
libxl_device_usbdev_init(&usbdev_saved);
@@ -1323,7 +1323,7 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
rc = 0;
out:
- if (lock) libxl__unlock_domain_userdata(lock);
+ if (lock) libxl__unlock_file(lock);
libxl_device_usbdev_dispose(&usbdev_saved);
libxl_domain_config_dispose(&d_config);
return rc;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
` (2 preceding siblings ...)
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 3/7] libxl: generalise libxl__domain_userdata_lock() Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-02-17 17:42 ` Ian Jackson
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid Paul Durrant
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
A domid is considered recent if the domain it represents was destroyed
less than a specified number of seconds ago. For debugging and/or testing
purposes the number can be set using the environment variable
LIBXL_DOMID_REUSE_TIMEOUT. If the variable does not exist then a default
value of 60s is used.
Whenever a domain is destroyed, a time-stamped record will be written into
a history file (/var/run/xen/domid-history). To avoid the history file
growing too large, any records with time-stamps that indicate that the
age of a domid has exceeded the re-use timeout will also be purged.
A new utility function, libxl__is_recent_domid(), has been added. This
function reads the same history file checking whether a specified domid
has a record that does not exceed the re-use timeout. Since this utility
function does not write to the file, no records are actually purged by it.
NOTE: The history file is purged on boot to it is safe to use
CLOCK_MONOTONIC as a time source.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
v5:
- Re-work file manipulation some more
- Add more error checks
v4:
- Use new generalised libxl__flock
- Don't read and write the same file
- Use 'recent' rather than 'retired'
- Add code into xen-init-dom0 to delete an old history file at boot
v2:
- New in v2
---
tools/helpers/xen-init-dom0.c | 30 +++++
tools/libxl/libxl.h | 2 +
tools/libxl/libxl_domain.c | 204 ++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.c | 10 ++
tools/libxl/libxl_internal.h | 14 +++
5 files changed, 260 insertions(+)
diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
index a1e5729458..56f69ab66f 100644
--- a/tools/helpers/xen-init-dom0.c
+++ b/tools/helpers/xen-init-dom0.c
@@ -12,6 +12,32 @@
#define DOMNAME_PATH "/local/domain/0/name"
#define DOMID_PATH "/local/domain/0/domid"
+int clear_domid_history(void)
+{
+ int rc = 1;
+ xentoollog_logger_stdiostream *logger;
+ libxl_ctx *ctx;
+
+ logger = xtl_createlogger_stdiostream(stderr, XTL_ERROR, 0);
+ if (!logger)
+ return 1;
+
+ if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
+ (xentoollog_logger *)logger)) {
+ fprintf(stderr, "cannot init libxl context\n");
+ goto outlog;
+ }
+
+ if (!libxl_clear_domid_history(ctx))
+ rc = 0;
+
+ libxl_ctx_free(ctx);
+
+outlog:
+ xtl_logger_destroy((xentoollog_logger *)logger);
+ return rc;
+}
+
int main(int argc, char **argv)
{
int rc;
@@ -70,6 +96,10 @@ int main(int argc, char **argv)
if (rc)
goto out;
+ rc = clear_domid_history();
+ if (rc)
+ goto out;
+
/* Write xenstore entries. */
if (!xs_write(xsh, XBT_NULL, DOMID_PATH, "0", strlen("0"))) {
fprintf(stderr, "cannot set domid for Dom0\n");
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 18c1a2d6bf..1d235ecb1c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2657,6 +2657,8 @@ static inline int libxl_qemu_monitor_command_0x041200(libxl_ctx *ctx,
#include <libxl_event.h>
+int libxl_clear_domid_history(libxl_ctx *ctx);
+
#endif /* LIBXL_H */
/*
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 973fc1434d..5349defcf0 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1268,6 +1268,208 @@ static void dm_destroy_cb(libxl__egc *egc,
libxl__devices_destroy(egc, &dis->drs);
}
+static unsigned int libxl__get_domid_reuse_timeout(void)
+{
+ const char *env_timeout = getenv("LIBXL_DOMID_REUSE_TIMEOUT");
+
+ return env_timeout ? strtol(env_timeout, NULL, 0) :
+ LIBXL_DOMID_REUSE_TIMEOUT;
+}
+
+char *libxl__domid_history_path(libxl__gc *gc, const char *suffix)
+{
+ return GCSPRINTF("%s/domid-history%s", libxl__run_dir_path(),
+ suffix ?: "");
+}
+
+int libxl_clear_domid_history(libxl_ctx *ctx)
+{
+ GC_INIT(ctx);
+ char *path;
+ int rc = ERROR_FAIL;
+
+ path = libxl__domid_history_path(gc, NULL);
+ if (!path)
+ goto out;
+
+ if (unlink(path) < 0 && errno != ENOENT) {
+ LOGE(ERROR, "failed to remove '%s'\n", path);
+ goto out;
+ }
+
+ rc = 0;
+
+out:
+ GC_FREE;
+ return rc;
+}
+
+static bool libxl__read_recent(FILE *f, unsigned long *sec,
+ unsigned int *domid)
+{
+ int n;
+
+ assert(f);
+
+ n = fscanf(f, "%lu %u", sec, domid);
+ if (n == EOF)
+ return false;
+ else if (n != 2) /* malformed entry */
+ *domid = INVALID_DOMID;
+
+ return true;
+}
+
+static bool libxl__write_recent(FILE *f, unsigned long sec,
+ unsigned int domid)
+{
+ assert(f);
+ assert(libxl_domid_valid_guest(domid));
+
+ return fprintf(f, "%lu %u\n", sec, domid) > 0;
+}
+
+static int libxl__mark_domid_recent(libxl__gc *gc, uint32_t domid)
+{
+ long timeout = libxl__get_domid_reuse_timeout();
+ libxl__flock *lock;
+ char *old, *new;
+ FILE *of = NULL, *nf = NULL;
+ struct timespec ts;
+ int rc = ERROR_FAIL;
+
+ lock = libxl__lock_domid_history(gc);
+ if (!lock) {
+ LOGED(ERROR, domid, "failed to acquire lock");
+ goto out;
+ }
+
+ old = libxl__domid_history_path(gc, NULL);
+ of = fopen(old, "r");
+ if (!of && errno != ENOENT)
+ LOGED(WARN, domid, "failed to open '%s'", old);
+
+ new = libxl__domid_history_path(gc, ".new");
+ nf = fopen(new, "a");
+ if (!nf) {
+ LOGED(ERROR, domid, "failed to open '%s'", new);
+ goto out;
+ }
+
+ if (clock_gettime(CLOCK_MONOTONIC, &ts)) {
+ LOGED(ERROR, domid, "failed to get time");
+ goto out;
+ }
+
+ if (of) {
+ unsigned long sec;
+ unsigned int val;
+
+ while (libxl__read_recent(of, &sec, &val)) {
+ if (!libxl_domid_valid_guest(val))
+ continue; /* Ignore invalid entries */
+
+ if (ts.tv_sec - sec > timeout)
+ continue; /* Ignore expired entries */
+
+ if (!libxl__write_recent(nf, sec, val)) {
+ LOGED(ERROR, domid, "failed to write to '%s'", new);
+ goto out;
+ }
+ }
+ if (ferror(of)) {
+ LOGED(ERROR, domid, "failed to read from '%s'", old);
+ goto out;
+ }
+ }
+
+ if (!libxl__write_recent(nf, ts.tv_sec, domid)) {
+ LOGED(ERROR, domid, "failed to write to '%s'", new);
+ goto out;
+ }
+
+ if (fclose(nf) == EOF) {
+ LOGED(ERROR, domid, "failed to close '%s'", new);
+ nf = NULL;
+ goto out;
+ }
+ nf = NULL;
+
+ if (of && fclose(of) == EOF) {
+ LOGED(ERROR, domid, "failed to close '%s'", old);
+ of = NULL;
+ goto out;
+ }
+ of = NULL;
+
+ if (rename(new, old) < 0) {
+ LOGED(ERROR, domid, "failed to rename '%s' -> '%s'", old, new);
+ goto out;
+ }
+ rc = 0;
+
+out:
+ if (nf) fclose(nf);
+ if (of) fclose(of);
+ if (lock) libxl__unlock_file(lock);
+
+ return rc;
+}
+
+int libxl__is_domid_recent(libxl__gc *gc, uint32_t domid, bool *recent)
+{
+ long timeout = libxl__get_domid_reuse_timeout();
+ const char *name;
+ FILE *f;
+ struct timespec ts;
+ unsigned long sec;
+ unsigned int val;
+ int rc = ERROR_FAIL;
+
+ name = GCSPRINTF("%s/domid-history", libxl__run_dir_path());
+ f = fopen(name, "r");
+ if (!f) {
+ if (errno != ENOENT)
+ LOGED(WARN, domid, "failed to open %s", name);
+ else
+ rc = 0;
+
+ goto out;
+ }
+
+ if (clock_gettime(CLOCK_MONOTONIC, &ts)) {
+ LOGED(ERROR, domid, "failed to get time");
+ goto out;
+ }
+
+ *recent = false;
+ while (libxl__read_recent(f, &sec, &val)) {
+ if (!libxl_domid_valid_guest(val))
+ continue; /* Ignore invalid entries */
+
+ if (val == domid && ts.tv_sec - sec <= timeout) {
+ *recent = true;
+ break;
+ }
+ }
+ if (ferror(f)) {
+ LOGED(ERROR, domid, "failed to read from '%s'", name);
+ goto out;
+ }
+
+ if (fclose(f) == EOF) {
+ LOGED(ERROR, domid, "failed to close '%s'", name);
+ f = NULL;
+ goto out;
+ }
+ f = NULL;
+ rc = 0;
+
+out:
+ if (f) fclose(f);
+ return rc;
+}
+
static void devices_destroy_cb(libxl__egc *egc,
libxl__devices_remove_state *drs,
int rc)
@@ -1331,6 +1533,8 @@ static void devices_destroy_cb(libxl__egc *egc,
if (!ctx->xch) goto badchild;
if (!dis->soft_reset) {
+ rc = libxl__mark_domid_recent(gc, domid);
+ if (rc) goto badchild;
rc = xc_domain_destroy(ctx->xch, domid);
} else {
rc = xc_domain_pause(ctx->xch, domid);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 211236dc99..bbd4c6cba9 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -504,6 +504,16 @@ libxl__flock *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
return lock;
}
+libxl__flock *libxl__lock_domid_history(libxl__gc *gc)
+{
+ const char *lockfile;
+
+ lockfile = libxl__domid_history_path(gc, ".lock");
+ if (!lockfile) return NULL;
+
+ return libxl__lock_file(gc, lockfile);
+}
+
int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
libxl_domain_config *d_config)
{
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index dd3c08bc14..39de2d5910 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4260,6 +4260,8 @@ _hidden void libxl__remus_teardown(libxl__egc *egc,
_hidden void libxl__remus_restore_setup(libxl__egc *egc,
libxl__domain_create_state *dcs);
+_hidden char *libxl__domid_history_path(libxl__gc *gc,
+ const char *suffix);
/*
* Convenience macros.
@@ -4658,6 +4660,7 @@ libxl__flock *libxl__lock_file(libxl__gc *gc, const char *filename);
void libxl__unlock_file(libxl__flock *lock);
libxl__flock *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid);
+libxl__flock *libxl__lock_domid_history(libxl__gc *gc);
/*
* Retrieve / store domain configuration from / to libxl private
@@ -4796,6 +4799,17 @@ _hidden int libxl__domain_pvcontrol(libxl__egc *egc,
libxl__xswait_state *pvcontrol,
domid_t domid, const char *cmd);
+/*
+ * Maximum number of seconds after desctruction then a domid remains
+ * 'recent'. Recent domids are not allowed to be re-used. This can be
+ * overidden, for debugging purposes, by the environment variable of the
+ * same name.
+ */
+#define LIBXL_DOMID_REUSE_TIMEOUT 60
+
+/* Check whether a domid is recent */
+int libxl__is_domid_recent(libxl__gc *gc, uint32_t domid, bool *recent);
+
#endif
/*
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids Paul Durrant
@ 2020-02-17 17:42 ` Ian Jackson
2020-02-18 9:24 ` Durrant, Paul
0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2020-02-17 17:42 UTC (permalink / raw)
To: Paul Durrant; +Cc: Anthony Perard, xen-devel, Wei Liu
Paul Durrant writes ("[PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids"):
> A domid is considered recent if the domain it represents was destroyed
> less than a specified number of seconds ago. For debugging and/or testing
> purposes the number can be set using the environment variable
> LIBXL_DOMID_REUSE_TIMEOUT. If the variable does not exist then a default
> value of 60s is used.
>
> Whenever a domain is destroyed, a time-stamped record will be written into
> a history file (/var/run/xen/domid-history). To avoid the history file
> growing too large, any records with time-stamps that indicate that the
> age of a domid has exceeded the re-use timeout will also be purged.
>
> A new utility function, libxl__is_recent_domid(), has been added. This
> function reads the same history file checking whether a specified domid
> has a record that does not exceed the re-use timeout. Since this utility
> function does not write to the file, no records are actually purged by it.
Thanks for this. Sorry for the delay in reviewing it.
I'm afraid I still have some comments about error handling etc.
> +int libxl_clear_domid_history(libxl_ctx *ctx);
I think this needs a clear doc comment saying it is for use in host
initialisation only. If it is run with any domains running, or
concurrent libxl processes, things may malfunction.
> +static bool libxl__read_recent(FILE *f, unsigned long *sec,
> + unsigned int *domid)
> +{
> + int n;
> +
> + assert(f);
> +
> + n = fscanf(f, "%lu %u", sec, domid);
> + if (n == EOF)
> + return false;
Missing error handling in case of read error.
> + else if (n != 2) /* malformed entry */
> + *domid = INVALID_DOMID;
Both call sites for this function have open-coded checks for this
return case, where they just go round again. I think
libxl__read_recent should handle this itself, factoring the common
code into this function and avoiding that special case.
> + return true;
I think this function should return an rc. It could signal EOF by
setting *domid to INVALID_DOMID maybe, and errors by returning
ERROR_FAIL.
> +static bool libxl__write_recent(FILE *f, unsigned long sec,
> + unsigned int domid)
> +{
> + assert(f);
This is rather pointless. Please drop it.
> + assert(libxl_domid_valid_guest(domid));
I doubt this is really needed but I don't mind it if you must.
> + return fprintf(f, "%lu %u\n", sec, domid) > 0;
Wrong error handling. This function should return rc. fprintf
doesn't return a boolean. Something should log errno (with LOGE
probably) if fprintf fails.
> +static int libxl__mark_domid_recent(libxl__gc *gc, uint32_t domid)
> +{
> + long timeout = libxl__get_domid_reuse_timeout();
> + libxl__flock *lock;
Please initialise lock = NULL so that it is easy to see that the out
block is correct.
(See tools/libxl/CODING_STYLE where this is discussed.)
> + char *old, *new;
> + FILE *of = NULL, *nf = NULL;
> + struct timespec ts;
> + int rc = ERROR_FAIL;
Please do not set rc to ERROR_FAIL like this. Leave it undefined.
Set it on each exit path. (If you are calling a function that returns
an rc, you can put it in rc, and then test rc and goto out without
assignment.)
(Again, see tools/libxl/CODING_STYLE where this is discussed.)
> + lock = libxl__lock_domid_history(gc);
> + if (!lock) {
> + LOGED(ERROR, domid, "failed to acquire lock");
> + goto out;
> + }
> +
> + old = libxl__domid_history_path(gc, NULL);
> + of = fopen(old, "r");
> + if (!of && errno != ENOENT)
> + LOGED(WARN, domid, "failed to open '%s'", old);
This fopen code and its error handling is still duplicated between
libxl__mark_domid_recent and libxl__is_domid_recent. I meant for you
to factor it out. Likewise the other duplicated code in these two
functions. I want there to be nothing duplicated that can be written
once.
Also failure to open the file should be an error, resulting failure of
this function and the whole surrounding operation, not simply produce
a warning in some logfile where it will be ignored.
> + while (libxl__read_recent(of, &sec, &val)) {
> + if (!libxl_domid_valid_guest(val))
> + continue; /* Ignore invalid entries */
> +
> + if (ts.tv_sec - sec > timeout)
> + continue; /* Ignore expired entries */
> +
> + if (!libxl__write_recent(nf, sec, val)) {
> + LOGED(ERROR, domid, "failed to write to '%s'", new);
> + goto out;
> + }
> + }
> + if (ferror(of)) {
> + LOGED(ERROR, domid, "failed to read from '%s'", old);
> + goto out;
> + }
Oh, wait, here is one of the missing pieces of error handling ?
Please put it where it belongs, next to the corresponding call.
> + if (of && fclose(of) == EOF) {
> + LOGED(ERROR, domid, "failed to close '%s'", old);
I don't see how of would be NULL here.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids
2020-02-17 17:42 ` Ian Jackson
@ 2020-02-18 9:24 ` Durrant, Paul
2020-02-18 11:38 ` Ian Jackson
0 siblings, 1 reply; 21+ messages in thread
From: Durrant, Paul @ 2020-02-18 9:24 UTC (permalink / raw)
To: Ian Jackson; +Cc: Anthony Perard, xen-devel, Wei Liu
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 17 February 2020 17:43
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>
> Subject: Re: [PATCH v5 4/7] libxl: add infrastructure to track and query
> 'recent' domids
>
> Paul Durrant writes ("[PATCH v5 4/7] libxl: add infrastructure to track
> and query 'recent' domids"):
> > A domid is considered recent if the domain it represents was destroyed
> > less than a specified number of seconds ago. For debugging and/or
> testing
> > purposes the number can be set using the environment variable
> > LIBXL_DOMID_REUSE_TIMEOUT. If the variable does not exist then a default
> > value of 60s is used.
> >
> > Whenever a domain is destroyed, a time-stamped record will be written
> into
> > a history file (/var/run/xen/domid-history). To avoid the history file
> > growing too large, any records with time-stamps that indicate that the
> > age of a domid has exceeded the re-use timeout will also be purged.
> >
> > A new utility function, libxl__is_recent_domid(), has been added. This
> > function reads the same history file checking whether a specified domid
> > has a record that does not exceed the re-use timeout. Since this utility
> > function does not write to the file, no records are actually purged by
> it.
>
> Thanks for this. Sorry for the delay in reviewing it.
>
> I'm afraid I still have some comments about error handling etc.
>
> > +int libxl_clear_domid_history(libxl_ctx *ctx);
>
> I think this needs a clear doc comment saying it is for use in host
> initialisation only. If it is run with any domains running, or
> concurrent libxl processes, things may malfunction.
Ok. Not sure precisely what you mean by 'doc comment'... Do mean a comment in the header just above this declaration or elsewhere?
>
> > +static bool libxl__read_recent(FILE *f, unsigned long *sec,
> > + unsigned int *domid)
> > +{
> > + int n;
> > +
> > + assert(f);
> > +
> > + n = fscanf(f, "%lu %u", sec, domid);
> > + if (n == EOF)
> > + return false;
>
> Missing error handling in case of read error.
'man fscanf' tells me:
"The value EOF is returned if the end of input is reached before either the first suc‐
cessful conversion or a matching failure occurs. EOF is also returned if a read error
occurs, in which case the error indicator for the stream (see ferror(3)) is set, and
errno is set to indicate the error."
So EOF is set in all error cases. What am I missing?
>
> > + else if (n != 2) /* malformed entry */
> > + *domid = INVALID_DOMID;
>
> Both call sites for this function have open-coded checks for this
> return case, where they just go round again. I think
> libxl__read_recent should handle this itself, factoring the common
> code into this function and avoiding that special case.
Ok. I thought it was more intuitive to have the function only ever read a single entry from the file, but I can easily add the retry loop if you prefer.
>
> > + return true;
>
> I think this function should return an rc. It could signal EOF by
> setting *domid to INVALID_DOMID maybe, and errors by returning
> ERROR_FAIL.
Ok. I thought it was slightly pointless to do that.
>
> > +static bool libxl__write_recent(FILE *f, unsigned long sec,
> > + unsigned int domid)
> > +{
> > + assert(f);
>
> This is rather pointless. Please drop it.
>
If you think so, ok.
> > + assert(libxl_domid_valid_guest(domid));
>
> I doubt this is really needed but I don't mind it if you must.
>
> > + return fprintf(f, "%lu %u\n", sec, domid) > 0;
>
> Wrong error handling. This function should return rc. fprintf
> doesn't return a boolean.
And nor does this code expect it to (since it tests for '> 0').
> Something should log errno (with LOGE
> probably) if fprintf fails.
I can see you dislike boolean functions; I'll return an error as you desire.
>
> > +static int libxl__mark_domid_recent(libxl__gc *gc, uint32_t domid)
> > +{
> > + long timeout = libxl__get_domid_reuse_timeout();
> > + libxl__flock *lock;
>
> Please initialise lock = NULL so that it is easy to see that the out
> block is correct.
>
> (See tools/libxl/CODING_STYLE where this is discussed.)
>
Ok. Xen style generally avoids initializers where not strictly necessary.
> > + char *old, *new;
> > + FILE *of = NULL, *nf = NULL;
> > + struct timespec ts;
> > + int rc = ERROR_FAIL;
>
> Please do not set rc to ERROR_FAIL like this. Leave it undefined.
> Set it on each exit path. (If you are calling a function that returns
> an rc, you can put it in rc, and then test rc and goto out without
> assignment.)
>
> (Again, see tools/libxl/CODING_STYLE where this is discussed.)
>
Ok.
> > + lock = libxl__lock_domid_history(gc);
> > + if (!lock) {
> > + LOGED(ERROR, domid, "failed to acquire lock");
> > + goto out;
> > + }
> > +
> > + old = libxl__domid_history_path(gc, NULL);
> > + of = fopen(old, "r");
> > + if (!of && errno != ENOENT)
> > + LOGED(WARN, domid, "failed to open '%s'", old);
>
> This fopen code and its error handling is still duplicated between
> libxl__mark_domid_recent and libxl__is_domid_recent.
That's not quite true. The error semantics are different; the former does not tolerate a failure to open the file, the latter does.
> I meant for you
> to factor it out. Likewise the other duplicated code in these two
> functions. I want there to be nothing duplicated that can be written
> once.
I'm not sure how you want me to combine them, given the differing semantics.
>
> Also failure to open the file should be an error, resulting failure of
> this function and the whole surrounding operation, not simply produce
> a warning in some logfile where it will be ignored.
But that will cause a failure when trying to create the first domain after boot, since the file won't exist.
>
> > + while (libxl__read_recent(of, &sec, &val)) {
> > + if (!libxl_domid_valid_guest(val))
> > + continue; /* Ignore invalid entries */
> > +
> > + if (ts.tv_sec - sec > timeout)
> > + continue; /* Ignore expired entries */
> > +
> > + if (!libxl__write_recent(nf, sec, val)) {
> > + LOGED(ERROR, domid, "failed to write to '%s'", new);
> > + goto out;
> > + }
> > + }
> > + if (ferror(of)) {
> > + LOGED(ERROR, domid, "failed to read from '%s'", old);
> > + goto out;
> > + }
>
> Oh, wait, here is one of the missing pieces of error handling ?
> Please put it where it belongs, next to the corresponding call.
>
> > + if (of && fclose(of) == EOF) {
> > + LOGED(ERROR, domid, "failed to close '%s'", old);
>
> I don't see how of would be NULL here.
>
It will be NULL if the file did not exist, which will be the case until the first domain destruction occurs.
Paul
> Thanks,
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids
2020-02-18 9:24 ` Durrant, Paul
@ 2020-02-18 11:38 ` Ian Jackson
0 siblings, 0 replies; 21+ messages in thread
From: Ian Jackson @ 2020-02-18 11:38 UTC (permalink / raw)
To: Durrant, Paul; +Cc: Anthony Perard, xen-devel, Wei Liu
Durrant, Paul writes ("RE: [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids"):
> Ian Jackson <ian.jackson@citrix.com>:
> > Paul Durrant writes ("[PATCH v5 4/7] libxl: add infrastructure to track
> > > +int libxl_clear_domid_history(libxl_ctx *ctx);
> >
> > I think this needs a clear doc comment saying it is for use in host
> > initialisation only. If it is run with any domains running, or
> > concurrent libxl processes, things may malfunction.
>
> Ok. Not sure precisely what you mean by 'doc comment'... Do mean a
> comment in the header just above this declaration [...] ?
Yes, precisely that. Thanks.
> > > +static bool libxl__read_recent(FILE *f, unsigned long *sec,
> > > + unsigned int *domid)
> > > +{
> > > + int n;
> > > +
> > > + assert(f);
> > > +
> > > + n = fscanf(f, "%lu %u", sec, domid);
> > > + if (n == EOF)
> > > + return false;
> >
> > Missing error handling in case of read error.
>
> 'man fscanf' tells me:
>
> "The value EOF is returned if the end of input is reached before
> either the first suc‐ cessful conversion or a matching failure
> occurs. EOF is also returned if a read error occurs, in which case
> the error indicator for the stream (see ferror(3)) is set, and errno
> is set to indicate the error."
>
> So EOF is set in all error cases. What am I missing?
I thought it treats read error the same as EOF. But of course
actually I discovered a ferror() (duplicated) later...
> > > + else if (n != 2) /* malformed entry */
> > > + *domid = INVALID_DOMID;
> >
> > Both call sites for this function have open-coded checks for this
> > return case, where they just go round again. I think
> > libxl__read_recent should handle this itself, factoring the common
> > code into this function and avoiding that special case.
>
> Ok. I thought it was more intuitive to have the function only ever
> read a single entry from the file, but I can easily add the retry
> loop if you prefer.
I think the purpose of this function is to contain all the code that
can be shared between the two call sites.
> > > + return true;
> >
> > I think this function should return an rc. It could signal EOF by
> > setting *domid to INVALID_DOMID maybe, and errors by returning
> > ERROR_FAIL.
>
> Ok. I thought it was slightly pointless to do that.
I don't have a 100% fixed opinion about the precise calling
convention. But this function needs to be able to report three
distinct conditions, not two:
- here is the entry you asked for
- EOF, we have established that there are no more entries
- failure to read the file, abandon all hope
Elsewhere in libxl the convention is usually to use an rc return value
to signal errors, and signal "no error, but no such thing" by writing
a sentinel rather than a value to an out parameter.
Returning an rc means that in the future if we want better control of
errors (i) this internal api is more like other internal apis (ii) the
exact error code is specified at the point in the code where the error
is recognised.
> > I doubt this is really needed but I don't mind it if you must.
> >
> > > + return fprintf(f, "%lu %u\n", sec, domid) > 0;
> >
> > Wrong error handling. This function should return rc. fprintf
> > doesn't return a boolean.
>
> And nor does this code expect it to (since it tests for '> 0').
Oh. I didn't spot that. This is contrary to libxl/CODING_STYLE.
* Function calls which might fail (ie most function calls) are
handled by putting the return/status value into a variable, and
then checking it in a separate statement:
char *dompath = libxl__xs_get_dompath(gc, bl->domid);
if (!dompath) { rc = ERROR_FAIL; goto out; }
For precisely this kind of reason.
> > Something should log errno (with LOGE
> > probably) if fprintf fails.
>
> I can see you dislike boolean functions; I'll return an error as you desire.
See above about error handling. Certainly a boolean cannot be used
for a function which might return "yes" or "no" or "argh, can't say".
For a function which might return "ok" or "argh", rc and ERROR_* is
clearly better since you get to invent the error code.
> > > +static int libxl__mark_domid_recent(libxl__gc *gc, uint32_t domid)
> > > +{
> > > + long timeout = libxl__get_domid_reuse_timeout();
> > > + libxl__flock *lock;
> >
> > Please initialise lock = NULL so that it is easy to see that the out
> > block is correct.
> >
> > (See tools/libxl/CODING_STYLE where this is discussed.)
>
> Ok. Xen style generally avoids initializers where not strictly necessary.
libxl does not use "Xen style".
If you want to challenge the contents of libxl/CODING_STYLE, that's
fair enough of course, but maybe in the middle of this patch review is
not ideal ?
> > > + lock = libxl__lock_domid_history(gc);
> > > + if (!lock) {
> > > + LOGED(ERROR, domid, "failed to acquire lock");
> > > + goto out;
> > > + }
> > > +
> > > + old = libxl__domid_history_path(gc, NULL);
> > > + of = fopen(old, "r");
> > > + if (!of && errno != ENOENT)
> > > + LOGED(WARN, domid, "failed to open '%s'", old);
> >
> > This fopen code and its error handling is still duplicated between
> > libxl__mark_domid_recent and libxl__is_domid_recent.
>
> That's not quite true. The error semantics are different; the former does not tolerate a failure to open the file, the latter does.
What is the reason for this difference in semantics ? It seems to me
that either:
(i) absence of the file means there are no recent domids (eg,
after boot) and therefore both functions should tolerate it; or
(ii) absence of the file means a system configuration error
and therefore neither function should tolerate it.
> > Also failure to open the file should be an error, resulting failure of
> > this function and the whole surrounding operation, not simply produce
> > a warning in some logfile where it will be ignored.
>
> But that will cause a failure when trying to create the first domain
> after boot, since the file won't exist.
I meant that failure to open *other than ENOENT*.
ISTM that of the two options above, (i) is to be preferred and
therefore that ENOENT should always be tolerated. But maybe you can
explain to me why that isn't right.
> > > + if (of && fclose(of) == EOF) {
> > > + LOGED(ERROR, domid, "failed to close '%s'", old);
> >
> > I don't see how of would be NULL here.
>
> It will be NULL if the file did not exist, which will be the case until the first domain destruction occurs.
Oh yes. I am confused because I keep reading `of' as `output file'.
In which case, please see CODING_STYLE about putting the return value
in a separate statement. This will also avoid duplicating the
`of=NULL' since it can go right after fclose.
Maybe the closing could be done by libxl__read_recent, if it took a
FILE** ? That would remove some duplication and leave only an
error-check-free if (of) fclose(of); in each out block.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
` (3 preceding siblings ...)
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 4/7] libxl: add infrastructure to track and query 'recent' domids Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-01-31 17:22 ` Jason Andryuk
2020-02-17 17:51 ` Ian Jackson
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 6/7] xl.conf: introduce 'domid_policy' Paul Durrant
` (2 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Paul Durrant, Jason Andryuk,
Ian Jackson, Anthony PERARD
This patch adds a 'domid' field to libxl_domain_create_info and then
modifies libxl__domain_make() to have Xen use that value if it is valid.
If the domid value is invalid then Xen will choose the domid, as before,
unless the value is the new special RANDOM_DOMID value added to the API.
This value instructs libxl__domain_make() to choose a random domid value
for Xen to use.
If Xen determines that a domid specified to or chosen by
libxl__domain_make() co-incides with an existing domain then the create
operation will fail. In this case, if RANDOM_DOMID was specified to
libxl__domain_make() then a new random value will be chosen and the create
operation will be re-tried, otherwise libxl__domain_make() will fail.
After Xen has successfully created a new domain, libxl__domain_make() will
check whether its domid matches any recently used domid values. If it does
then the domain will be destroyed. If the domid used in creation was
specified to libxl__domain_make() then it will fail at this point,
otherwise the create operation will be re-tried with either a new random
or Xen-selected domid value.
NOTE: libxl__logv() is also modified to only log valid domid values in
messages rather than any domid, valid or otherwise, that is not
INVALID_DOMID.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jason Andryuk <jandryuk@gmail.com>
v5:
- Flattened nested loops
v4:
- Not added Jason's R-b because of substantial change
- Check for recent domid *after* creation
- Re-worked commit comment
v3:
- Added DOMID_MASK definition used to mask randomized values
- Use stack variable to avoid assuming endianness
v2:
- Re-worked to use a value from libxl_domain_create_info
---
tools/libxl/libxl.h | 9 +++++
tools/libxl/libxl_create.c | 67 ++++++++++++++++++++++++++++++++----
tools/libxl/libxl_internal.c | 2 +-
tools/libxl/libxl_types.idl | 1 +
xen/include/public/xen.h | 3 ++
5 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1d235ecb1c..31c6f4b11a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1268,6 +1268,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
*/
#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
+/*
+ * LIBXL_HAVE_CREATEINFO_DOMID
+ *
+ * libxl_domain_create_new() and libxl_domain_create_restore() will use
+ * a domid specified in libxl_domain_create_info().
+ */
+#define LIBXL_HAVE_CREATEINFO_DOMID
+
typedef char **libxl_string_list;
void libxl_string_list_dispose(libxl_string_list *sl);
int libxl_string_list_length(const libxl_string_list *sl);
@@ -1528,6 +1536,7 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
/* domain related functions */
#define INVALID_DOMID ~0
+#define RANDOM_DOMID (INVALID_DOMID - 1)
/* If the result is ERROR_ABORTED, the domain may or may not exist
* (in a half-created state). *domid will be valid and will be the
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 3a7364e2ac..7fd4d713e7 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl_domain_create_info *info = &d_config->c_info;
libxl_domain_build_info *b_info = &d_config->b_info;
- assert(soft_reset || *domid == INVALID_DOMID);
-
uuid_string = libxl__uuid2string(gc, info->uuid);
if (!uuid_string) {
rc = ERROR_NOMEM;
@@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
goto out;
}
- ret = xc_domain_create(ctx->xch, domid, &create);
- if (ret < 0) {
- LOGED(ERROR, *domid, "domain creation fail");
- rc = ERROR_FAIL;
- goto out;
+ for (;;) {
+ bool recent;
+
+ if (info->domid == RANDOM_DOMID) {
+ uint16_t v;
+
+ ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
+ if (ret < 0)
+ break;
+
+ v &= DOMID_MASK;
+ if (!libxl_domid_valid_guest(v))
+ continue;
+
+ *domid = v;
+ } else
+ *domid = info->domid;
+
+ ret = xc_domain_create(ctx->xch, domid, &create);
+ if (ret < 0) {
+ /*
+ * If we generated a random domid and creation failed
+ * because that domid already exists then simply try
+ * again.
+ */
+ if (errno == EEXIST && info->domid == RANDOM_DOMID)
+ continue;
+
+ LOGED(ERROR, *domid, "domain creation fail");
+ *domid = INVALID_DOMID;
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__is_domid_recent(gc, *domid, &recent);
+ if (rc)
+ goto out;
+
+ /* The domid is not recent, so we're done */
+ if (!recent)
+ break;
+
+ /*
+ * If the domid was specified then there's no point in
+ * trying again.
+ */
+ if (libxl_domid_valid_guest(info->domid)) {
+ LOGED(ERROR, *domid, "domain id recently used");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Try to destroy the domain again as we can't use it */
+ ret = xc_domain_destroy(ctx->xch, *domid);
+ if (ret < 0) {
+ LOGED(ERROR, *domid, "domain destroy fail");
+ *domid = INVALID_DOMID;
+ rc = ERROR_FAIL;
+ goto out;
+ }
}
rc = libxl__arch_domain_save_config(gc, d_config, state, &create);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index bbd4c6cba9..d93a75533f 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -234,7 +234,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
fileline[sizeof(fileline)-1] = 0;
domain[0] = 0;
- if (domid != INVALID_DOMID)
+ if (libxl_domid_valid_guest(domid))
snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid);
x:
xtl_log(ctx->lg, msglevel, errnoval, "libxl",
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 7921950f6a..d0d431614f 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -409,6 +409,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
("ssidref", uint32),
("ssid_label", string),
("name", string),
+ ("domid", libxl_domid),
("uuid", libxl_uuid),
("xsdata", libxl_key_value_list),
("platformdata", libxl_key_value_list),
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d2198dffad..75b1619d0d 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
/* Idle domain. */
#define DOMID_IDLE xen_mk_uint(0x7FFF)
+/* Mask for valid domain id values */
+#define DOMID_MASK xen_mk_uint(0x7FFF)
+
#ifndef __ASSEMBLY__
typedef uint16_t domid_t;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid Paul Durrant
@ 2020-01-31 17:22 ` Jason Andryuk
2020-02-03 7:50 ` Durrant, Paul
2020-02-17 17:51 ` Ian Jackson
1 sibling, 1 reply; 21+ messages in thread
From: Jason Andryuk @ 2020-01-31 17:22 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Anthony PERARD,
xen-devel
On Fri, Jan 31, 2020 at 10:02 AM Paul Durrant <pdurrant@amazon.com> wrote:
>
> This patch adds a 'domid' field to libxl_domain_create_info and then
> modifies libxl__domain_make() to have Xen use that value if it is valid.
> If the domid value is invalid then Xen will choose the domid, as before,
> unless the value is the new special RANDOM_DOMID value added to the API.
> This value instructs libxl__domain_make() to choose a random domid value
> for Xen to use.
>
> If Xen determines that a domid specified to or chosen by
> libxl__domain_make() co-incides with an existing domain then the create
> operation will fail. In this case, if RANDOM_DOMID was specified to
> libxl__domain_make() then a new random value will be chosen and the create
> operation will be re-tried, otherwise libxl__domain_make() will fail.
>
> After Xen has successfully created a new domain, libxl__domain_make() will
> check whether its domid matches any recently used domid values. If it does
> then the domain will be destroyed. If the domid used in creation was
> specified to libxl__domain_make() then it will fail at this point,
> otherwise the create operation will be re-tried with either a new random
> or Xen-selected domid value.
>
> NOTE: libxl__logv() is also modified to only log valid domid values in
> messages rather than any domid, valid or otherwise, that is not
> INVALID_DOMID.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Looks good, with one suggestion below.
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
<snip>
> +
> + /* Try to destroy the domain again as we can't use it */
> + ret = xc_domain_destroy(ctx->xch, *domid);
> + if (ret < 0) {
> + LOGED(ERROR, *domid, "domain destroy fail");
Maybe "destroy recently used domain id failed"?
> + *domid = INVALID_DOMID;
> + rc = ERROR_FAIL;
> + goto out;
> + }
> }
>
> rc = libxl__arch_domain_save_config(gc, d_config, state, &create);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-01-31 17:22 ` Jason Andryuk
@ 2020-02-03 7:50 ` Durrant, Paul
0 siblings, 0 replies; 21+ messages in thread
From: Durrant, Paul @ 2020-02-03 7:50 UTC (permalink / raw)
To: jandryuk
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Anthony PERARD,
xen-devel
> -----Original Message-----
> From: jandryuk@gmail.com <jandryuk@gmail.com>
> Sent: 31 January 2020 17:23
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel <xen-devel@lists.xenproject.org>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Anthony PERARD
> <anthony.perard@citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <George.Dunlap@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v5 5/7] libxl: allow creation of domains with a
> specified or random domid
>
> On Fri, Jan 31, 2020 at 10:02 AM Paul Durrant <pdurrant@amazon.com> wrote:
> >
> > This patch adds a 'domid' field to libxl_domain_create_info and then
> > modifies libxl__domain_make() to have Xen use that value if it is valid.
> > If the domid value is invalid then Xen will choose the domid, as before,
> > unless the value is the new special RANDOM_DOMID value added to the API.
> > This value instructs libxl__domain_make() to choose a random domid value
> > for Xen to use.
> >
> > If Xen determines that a domid specified to or chosen by
> > libxl__domain_make() co-incides with an existing domain then the create
> > operation will fail. In this case, if RANDOM_DOMID was specified to
> > libxl__domain_make() then a new random value will be chosen and the
> create
> > operation will be re-tried, otherwise libxl__domain_make() will fail.
> >
> > After Xen has successfully created a new domain, libxl__domain_make()
> will
> > check whether its domid matches any recently used domid values. If it
> does
> > then the domain will be destroyed. If the domid used in creation was
> > specified to libxl__domain_make() then it will fail at this point,
> > otherwise the create operation will be re-tried with either a new random
> > or Xen-selected domid value.
> >
> > NOTE: libxl__logv() is also modified to only log valid domid values in
> > messages rather than any domid, valid or otherwise, that is not
> > INVALID_DOMID.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>
> Looks good, with one suggestion below.
>
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
>
Thanks.
> <snip>
>
> > +
> > + /* Try to destroy the domain again as we can't use it */
> > + ret = xc_domain_destroy(ctx->xch, *domid);
> > + if (ret < 0) {
> > + LOGED(ERROR, *domid, "domain destroy fail");
>
> Maybe "destroy recently used domain id failed"?
>
Probably doesn't actually matter. A destroy failure during domain creation would be an unusual circumstance, but if I have to post a v6 I'll change the message while I'm at it.
Paul
> > + *domid = INVALID_DOMID;
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > }
> >
> > rc = libxl__arch_domain_save_config(gc, d_config, state,
> &create);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid Paul Durrant
2020-01-31 17:22 ` Jason Andryuk
@ 2020-02-17 17:51 ` Ian Jackson
2020-02-18 9:31 ` Durrant, Paul
1 sibling, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2020-02-17 17:51 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Jason Andryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Paul Durrant writes ("[PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> This patch adds a 'domid' field to libxl_domain_create_info and then
> modifies libxl__domain_make() to have Xen use that value if it is valid.
> If the domid value is invalid then Xen will choose the domid, as before,
> unless the value is the new special RANDOM_DOMID value added to the API.
> This value instructs libxl__domain_make() to choose a random domid value
> for Xen to use.
>
> If Xen determines that a domid specified to or chosen by
> libxl__domain_make() co-incides with an existing domain then the create
> operation will fail. In this case, if RANDOM_DOMID was specified to
> libxl__domain_make() then a new random value will be chosen and the create
> operation will be re-tried, otherwise libxl__domain_make() will fail.
>
> After Xen has successfully created a new domain, libxl__domain_make() will
> check whether its domid matches any recently used domid values. If it does
> then the domain will be destroyed. If the domid used in creation was
> specified to libxl__domain_make() then it will fail at this point,
> otherwise the create operation will be re-tried with either a new random
> or Xen-selected domid value.
>
> NOTE: libxl__logv() is also modified to only log valid domid values in
> messages rather than any domid, valid or otherwise, that is not
> INVALID_DOMID.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Jason Andryuk <jandryuk@gmail.com>
>
> v5:
> - Flattened nested loops
>
> v4:
> - Not added Jason's R-b because of substantial change
> - Check for recent domid *after* creation
> - Re-worked commit comment
>
> v3:
> - Added DOMID_MASK definition used to mask randomized values
> - Use stack variable to avoid assuming endianness
>
> v2:
> - Re-worked to use a value from libxl_domain_create_info
> ---
> tools/libxl/libxl.h | 9 +++++
> tools/libxl/libxl_create.c | 67 ++++++++++++++++++++++++++++++++----
> tools/libxl/libxl_internal.c | 2 +-
> tools/libxl/libxl_types.idl | 1 +
> xen/include/public/xen.h | 3 ++
> 5 files changed, 74 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 1d235ecb1c..31c6f4b11a 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1268,6 +1268,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
> */
> #define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
>
> +/*
> + * LIBXL_HAVE_CREATEINFO_DOMID
> + *
> + * libxl_domain_create_new() and libxl_domain_create_restore() will use
> + * a domid specified in libxl_domain_create_info().
> + */
> +#define LIBXL_HAVE_CREATEINFO_DOMID
> +
> typedef char **libxl_string_list;
> void libxl_string_list_dispose(libxl_string_list *sl);
> int libxl_string_list_length(const libxl_string_list *sl);
> @@ -1528,6 +1536,7 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
> /* domain related functions */
>
> #define INVALID_DOMID ~0
> +#define RANDOM_DOMID (INVALID_DOMID - 1)
>
> /* If the result is ERROR_ABORTED, the domain may or may not exist
> * (in a half-created state). *domid will be valid and will be the
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 3a7364e2ac..7fd4d713e7 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> libxl_domain_create_info *info = &d_config->c_info;
> libxl_domain_build_info *b_info = &d_config->b_info;
>
> - assert(soft_reset || *domid == INVALID_DOMID);
> -
> uuid_string = libxl__uuid2string(gc, info->uuid);
> if (!uuid_string) {
> rc = ERROR_NOMEM;
> @@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> goto out;
> }
>
> - ret = xc_domain_create(ctx->xch, domid, &create);
> - if (ret < 0) {
> - LOGED(ERROR, *domid, "domain creation fail");
> - rc = ERROR_FAIL;
> - goto out;
> + for (;;) {
> + bool recent;
> +
> + if (info->domid == RANDOM_DOMID) {
> + uint16_t v;
> +
> + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> + if (ret < 0)
> + break;
> +
> + v &= DOMID_MASK;
> + if (!libxl_domid_valid_guest(v))
> + continue;
> +
> + *domid = v;
> + } else
> + *domid = info->domid;
Style: { } on all or none of the same `if' series. (CODING_STYLE)
> + /* The domid is not recent, so we're done */
> + if (!recent)
> + break;
> +
> + /*
> + * If the domid was specified then there's no point in
> + * trying again.
> + */
> + if (libxl_domid_valid_guest(info->domid)) {
> + LOGED(ERROR, *domid, "domain id recently used");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + /* Try to destroy the domain again as we can't use it */
> + ret = xc_domain_destroy(ctx->xch, *domid);
> + if (ret < 0) {
> + LOGED(ERROR, *domid, "domain destroy fail");
> + *domid = INVALID_DOMID;
> + rc = ERROR_FAIL;
> + goto out;
> + }
These two seem to be in the wrong order. Also if
libxl__is_domid_recent fails, you leak the domain.
This is sort of a result of you not treating `domid' as a `local
[variable] referring to resources which might need cleaning up'.
According to a strict reading of CODING_STYLE you should initialise it
to -1 and the xc_domain_destroy out should be in the out block, but
that would duplicate the call to destroy.
I don't mind exactly how you fix this, but please make sure not to
leak the newly-created domain even in the error cases.
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index bbd4c6cba9..d93a75533f 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -234,7 +234,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
> fileline[sizeof(fileline)-1] = 0;
>
> domain[0] = 0;
> - if (domid != INVALID_DOMID)
> + if (libxl_domid_valid_guest(domid))
> snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid);
> x:
> xtl_log(ctx->lg, msglevel, errnoval, "libxl",
This wants to be a separate patch.
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index d2198dffad..75b1619d0d 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> /* Idle domain. */
> #define DOMID_IDLE xen_mk_uint(0x7FFF)
>
> +/* Mask for valid domain id values */
> +#define DOMID_MASK xen_mk_uint(0x7FFF)
This needs a hypervisor maintainer ack.
Please split it into its own patch, with a rationale, etc.
Thanks,
ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-02-17 17:51 ` Ian Jackson
@ 2020-02-18 9:31 ` Durrant, Paul
2020-02-18 11:17 ` Ian Jackson
0 siblings, 1 reply; 21+ messages in thread
From: Durrant, Paul @ 2020-02-18 9:31 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Jason Andryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 17 February 2020 17:52
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Jason Andryuk <jandryuk@gmail.com>
> Subject: Re: [PATCH v5 5/7] libxl: allow creation of domains with a
> specified or random domid
>
> Paul Durrant writes ("[PATCH v5 5/7] libxl: allow creation of domains with
> a specified or random domid"):
> > This patch adds a 'domid' field to libxl_domain_create_info and then
> > modifies libxl__domain_make() to have Xen use that value if it is valid.
> > If the domid value is invalid then Xen will choose the domid, as before,
> > unless the value is the new special RANDOM_DOMID value added to the API.
> > This value instructs libxl__domain_make() to choose a random domid value
> > for Xen to use.
> >
> > If Xen determines that a domid specified to or chosen by
> > libxl__domain_make() co-incides with an existing domain then the create
> > operation will fail. In this case, if RANDOM_DOMID was specified to
> > libxl__domain_make() then a new random value will be chosen and the
> create
> > operation will be re-tried, otherwise libxl__domain_make() will fail.
> >
> > After Xen has successfully created a new domain, libxl__domain_make()
> will
> > check whether its domid matches any recently used domid values. If it
> does
> > then the domain will be destroyed. If the domid used in creation was
> > specified to libxl__domain_make() then it will fail at this point,
> > otherwise the create operation will be re-tried with either a new random
> > or Xen-selected domid value.
> >
> > NOTE: libxl__logv() is also modified to only log valid domid values in
> > messages rather than any domid, valid or otherwise, that is not
> > INVALID_DOMID.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Julien Grall <julien@xen.org>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Jason Andryuk <jandryuk@gmail.com>
> >
> > v5:
> > - Flattened nested loops
> >
> > v4:
> > - Not added Jason's R-b because of substantial change
> > - Check for recent domid *after* creation
> > - Re-worked commit comment
> >
> > v3:
> > - Added DOMID_MASK definition used to mask randomized values
> > - Use stack variable to avoid assuming endianness
> >
> > v2:
> > - Re-worked to use a value from libxl_domain_create_info
> > ---
> > tools/libxl/libxl.h | 9 +++++
> > tools/libxl/libxl_create.c | 67 ++++++++++++++++++++++++++++++++----
> > tools/libxl/libxl_internal.c | 2 +-
> > tools/libxl/libxl_types.idl | 1 +
> > xen/include/public/xen.h | 3 ++
> > 5 files changed, 74 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 1d235ecb1c..31c6f4b11a 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -1268,6 +1268,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac
> *dst, const libxl_mac *src);
> > */
> > #define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
> >
> > +/*
> > + * LIBXL_HAVE_CREATEINFO_DOMID
> > + *
> > + * libxl_domain_create_new() and libxl_domain_create_restore() will use
> > + * a domid specified in libxl_domain_create_info().
> > + */
> > +#define LIBXL_HAVE_CREATEINFO_DOMID
> > +
> > typedef char **libxl_string_list;
> > void libxl_string_list_dispose(libxl_string_list *sl);
> > int libxl_string_list_length(const libxl_string_list *sl);
> > @@ -1528,6 +1536,7 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
> > /* domain related functions */
> >
> > #define INVALID_DOMID ~0
> > +#define RANDOM_DOMID (INVALID_DOMID - 1)
> >
> > /* If the result is ERROR_ABORTED, the domain may or may not exist
> > * (in a half-created state). *domid will be valid and will be the
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 3a7364e2ac..7fd4d713e7 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc,
> libxl_domain_config *d_config,
> > libxl_domain_create_info *info = &d_config->c_info;
> > libxl_domain_build_info *b_info = &d_config->b_info;
> >
> > - assert(soft_reset || *domid == INVALID_DOMID);
> > -
> > uuid_string = libxl__uuid2string(gc, info->uuid);
> > if (!uuid_string) {
> > rc = ERROR_NOMEM;
> > @@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc,
> libxl_domain_config *d_config,
> > goto out;
> > }
> >
> > - ret = xc_domain_create(ctx->xch, domid, &create);
> > - if (ret < 0) {
> > - LOGED(ERROR, *domid, "domain creation fail");
> > - rc = ERROR_FAIL;
> > - goto out;
> > + for (;;) {
> > + bool recent;
> > +
> > + if (info->domid == RANDOM_DOMID) {
> > + uint16_t v;
> > +
> > + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> > + if (ret < 0)
> > + break;
> > +
> > + v &= DOMID_MASK;
> > + if (!libxl_domid_valid_guest(v))
> > + continue;
> > +
> > + *domid = v;
> > + } else
> > + *domid = info->domid;
>
> Style: { } on all or none of the same `if' series. (CODING_STYLE)
OK. Too used to Xen style.
>
> > + /* The domid is not recent, so we're done */
> > + if (!recent)
> > + break;
> > +
> > + /*
> > + * If the domid was specified then there's no point in
> > + * trying again.
> > + */
> > + if (libxl_domid_valid_guest(info->domid)) {
> > + LOGED(ERROR, *domid, "domain id recently used");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + /* Try to destroy the domain again as we can't use it */
> > + ret = xc_domain_destroy(ctx->xch, *domid);
> > + if (ret < 0) {
> > + LOGED(ERROR, *domid, "domain destroy fail");
> > + *domid = INVALID_DOMID;
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
>
> These two seem to be in the wrong order. Also if
> libxl__is_domid_recent fails, you leak the domain.
>
No, the domain will not be leaked. The existing failure handling in libxl will clean up if *domid != INVALID_DOMID.
> This is sort of a result of you not treating `domid' as a `local
> [variable] referring to resources which might need cleaning up'.
> According to a strict reading of CODING_STYLE you should initialise it
> to -1 and the xc_domain_destroy out should be in the out block, but
> that would duplicate the call to destroy.
>
> I don't mind exactly how you fix this, but please make sure not to
> leak the newly-created domain even in the error cases.
>
I think the error handling is good (but obscured by the way libxl works), and there is no way to avoid leaking the domain if xc_domain_destroy() fails.
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index bbd4c6cba9..d93a75533f 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -234,7 +234,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level
> msglevel, int errnoval,
> > fileline[sizeof(fileline)-1] = 0;
> >
> > domain[0] = 0;
> > - if (domid != INVALID_DOMID)
> > + if (libxl_domid_valid_guest(domid))
> > snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid);
> > x:
> > xtl_log(ctx->lg, msglevel, errnoval, "libxl",
>
> This wants to be a separate patch.
>
Ok.
> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index d2198dffad..75b1619d0d 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> > /* Idle domain. */
> > #define DOMID_IDLE xen_mk_uint(0x7FFF)
> >
> > +/* Mask for valid domain id values */
> > +#define DOMID_MASK xen_mk_uint(0x7FFF)
>
> This needs a hypervisor maintainer ack.
>
> Please split it into its own patch, with a rationale, etc.
>
Ok, but it has no rationale without the rest of this patch; I can only assert that it 'will be needed by a subsequent patch'.
Paul
> Thanks,
> ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-02-18 9:31 ` Durrant, Paul
@ 2020-02-18 11:17 ` Ian Jackson
2020-02-18 11:25 ` Durrant, Paul
0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2020-02-18 11:17 UTC (permalink / raw)
To: Durrant, Paul
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Jason Andryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> No, the domain will not be leaked. The existing failure handling in libxl will clean up if *domid != INVALID_DOMID.
Sorry if I was confused; I will read this again.
> > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > > index d2198dffad..75b1619d0d 100644
> > > --- a/xen/include/public/xen.h
> > > +++ b/xen/include/public/xen.h
> > > @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> > > /* Idle domain. */
> > > #define DOMID_IDLE xen_mk_uint(0x7FFF)
> > >
> > > +/* Mask for valid domain id values */
> > > +#define DOMID_MASK xen_mk_uint(0x7FFF)
> >
> > This needs a hypervisor maintainer ack.
> >
> > Please split it into its own patch, with a rationale, etc.
>
> Ok, but it has no rationale without the rest of this patch; I can only assert that it 'will be needed by a subsequent patch'.
What do the Xen maintainers, and my tools co-maintainers think ?
I was hoping for a rationale along the lines of:
This is useful for programs which need to [explanation],
including for example, libxl, which is going to want to
randomly generate domids.
Maybe it needs some explanation of why this belongs in the Xen public
headers rather than in some header available to libxc, libxl and other
tools stuff ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-02-18 11:17 ` Ian Jackson
@ 2020-02-18 11:25 ` Durrant, Paul
2020-02-18 11:47 ` Ian Jackson
0 siblings, 1 reply; 21+ messages in thread
From: Durrant, Paul @ 2020-02-18 11:25 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Jason Andryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 18 February 2020 11:17
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Jason Andryuk <jandryuk@gmail.com>
> Subject: RE: [PATCH v5 5/7] libxl: allow creation of domains with a
> specified or random domid
>
> Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains
> with a specified or random domid"):
> > No, the domain will not be leaked. The existing failure handling in
> libxl will clean up if *domid != INVALID_DOMID.
>
> Sorry if I was confused; I will read this again.
>
It is hard to follow the error paths. Early on in development I ended up with domains getting destroyed when I didn't want them to be (when xc_domain_create() failed due to a duplicate domid).
> > > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > > > index d2198dffad..75b1619d0d 100644
> > > > --- a/xen/include/public/xen.h
> > > > +++ b/xen/include/public/xen.h
> > > > @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> > > > /* Idle domain. */
> > > > #define DOMID_IDLE xen_mk_uint(0x7FFF)
> > > >
> > > > +/* Mask for valid domain id values */
> > > > +#define DOMID_MASK xen_mk_uint(0x7FFF)
> > >
> > > This needs a hypervisor maintainer ack.
> > >
> > > Please split it into its own patch, with a rationale, etc.
> >
> > Ok, but it has no rationale without the rest of this patch; I can only
> assert that it 'will be needed by a subsequent patch'.
>
> What do the Xen maintainers, and my tools co-maintainers think ?
>
> I was hoping for a rationale along the lines of:
>
> This is useful for programs which need to [explanation],
> including for example, libxl, which is going to want to
> randomly generate domids.
>
> Maybe it needs some explanation of why this belongs in the Xen public
> headers rather than in some header available to libxc, libxl and other
> tools stuff ?
Ok, I'll try words along those lines.
Paul
>
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-02-18 11:25 ` Durrant, Paul
@ 2020-02-18 11:47 ` Ian Jackson
2020-02-18 11:57 ` Durrant, Paul
0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2020-02-18 11:47 UTC (permalink / raw)
To: Durrant, Paul
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Jason Andryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> Ian Jackson <ian.jackson@citrix.com>
> > Sorry if I was confused; I will read this again.
>
> It is hard to follow the error paths. Early on in development I ended up with domains getting destroyed when I didn't want them to be (when xc_domain_create() failed due to a duplicate domid).
Having read the patch again, I suggest the following discipline (which
is along the lines contemplated by CODYING_STYLE):
The local variable `domid' contains only a domid we are trying to
create and does not constitute a "local [variable] referring to
resources which might need cleaning up" (in the words of
CODING_STYLE). Therefore it should never be passed to destroy.
Maybe it should be called `prospective_domid'.
The variable *domid _is_ a "local [variable] referring to resources
which might need cleaning up". Therefore it must only ever contain a
domain which actually exists. It should be set from prospective_domid
when xc_domain_create succeeds, and cleared (set back to INVALID) when
xc_domain_destroy succeeds in our retry loop.
That way any `goto out' anywhere will clear up a domain iff there is
one to clear up.
There is a hunk in this patch which I think is incompatible with this
discipline:
- assert(soft_reset || *domid == INVALID_DOMID);
-
I don't understand what this hunk is for. If we adopt the discipline
I suggest, can it go away ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
2020-02-18 11:47 ` Ian Jackson
@ 2020-02-18 11:57 ` Durrant, Paul
0 siblings, 0 replies; 21+ messages in thread
From: Durrant, Paul @ 2020-02-18 11:57 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Jason Andryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 18 February 2020 11:48
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Jason Andryuk <jandryuk@gmail.com>
> Subject: RE: [PATCH v5 5/7] libxl: allow creation of domains with a
> specified or random domid
>
> Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains
> with a specified or random domid"):
> > Ian Jackson <ian.jackson@citrix.com>
> > > Sorry if I was confused; I will read this again.
> >
> > It is hard to follow the error paths. Early on in development I ended up
> with domains getting destroyed when I didn't want them to be (when
> xc_domain_create() failed due to a duplicate domid).
>
> Having read the patch again, I suggest the following discipline (which
> is along the lines contemplated by CODYING_STYLE):
>
> The local variable `domid' contains only a domid we are trying to
> create and does not constitute a "local [variable] referring to
> resources which might need cleaning up" (in the words of
> CODING_STYLE). Therefore it should never be passed to destroy.
> Maybe it should be called `prospective_domid'.
>
> The variable *domid _is_ a "local [variable] referring to resources
> which might need cleaning up". Therefore it must only ever contain a
> domain which actually exists. It should be set from prospective_domid
> when xc_domain_create succeeds, and cleared (set back to INVALID) when
> xc_domain_destroy succeeds in our retry loop.
>
> That way any `goto out' anywhere will clear up a domain iff there is
> one to clear up.
>
> There is a hunk in this patch which I think is incompatible with this
> discipline:
>
> - assert(soft_reset || *domid == INVALID_DOMID);
> -
>
> I don't understand what this hunk is for. If we adopt the discipline
> I suggest, can it go away ?
Ok, I'll give that a try. It's possible things are sufficiently complex that a sub-function may be appropriate, which should also achieve the localization.
Paul
>
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Xen-devel] [PATCH v5 6/7] xl.conf: introduce 'domid_policy'
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
` (4 preceding siblings ...)
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 7/7] xl: allow domid to be preserved on save/restore or migrate Paul Durrant
2020-02-17 14:21 ` [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Durrant, Paul
7 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
This patch adds a new global 'domid_policy' configuration option to decide
how domain id values are allocated for new domains. It may be set to one of
two values:
"xen", the default value, will cause an invalid domid value to be passed
to do_domain_create() preserving the existing behaviour of having Xen
choose the domid value during domain_create().
"random" will cause the special RANDOM_DOMID value to be passed to
do_domain_create() such that libxl__domain_make() will select a random
domid value.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
v2:
- New in v2
---
docs/man/xl.conf.5.pod | 10 ++++++++++
tools/examples/xl.conf | 4 ++++
tools/xl/xl.c | 10 ++++++++++
tools/xl/xl.h | 1 +
tools/xl/xl_vmcontrol.c | 2 ++
5 files changed, 27 insertions(+)
diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
index 207ab3e77a..41ee428744 100644
--- a/docs/man/xl.conf.5.pod
+++ b/docs/man/xl.conf.5.pod
@@ -45,6 +45,16 @@ The semantics of each C<KEY> defines which form of C<VALUE> is required.
=over 4
+=item B<domid_policy="xen"|"random">
+
+Determines how domain-id is set when creating a new domain.
+
+If set to "xen" then the hypervisor will allocate new domain-id values on a sequential basis.
+
+If set to "random" then a random domain-id value will be chosen.
+
+Default: "xen"
+
=item B<autoballoon="off"|"on"|"auto">
If set to "on" then C<xl> will automatically reduce the amount of
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 0446deb304..95f2f442d3 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -1,5 +1,9 @@
## Global XL config file ##
+# Set domain-id policy. "xen" means that the hypervisor will choose the
+# id of a new domain. "random" means that a random value will be chosen.
+#domid_policy="xen"
+
# Control whether dom0 is ballooned down when xen doesn't have enough
# free memory to create a domain. "auto" means only balloon if dom0
# starts with all the host's memory.
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 3d4390a46d..2a5ddd4390 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -54,6 +54,7 @@ int claim_mode = 1;
bool progress_use_cr = 0;
int max_grant_frames = -1;
int max_maptrack_frames = -1;
+libxl_domid domid_policy = INVALID_DOMID;
xentoollog_level minmsglevel = minmsglevel_default;
@@ -228,6 +229,15 @@ static void parse_global_config(const char *configfile,
else
libxl_bitmap_set_any(&global_pv_affinity_mask);
+ if (!xlu_cfg_get_string (config, "domid_policy", &buf, 0)) {
+ if (!strcmp(buf, "xen"))
+ domid_policy = INVALID_DOMID;
+ else if (!strcmp(buf, "random"))
+ domid_policy = RANDOM_DOMID;
+ else
+ fprintf(stderr, "invalid domid_policy option");
+ }
+
xlu_cfg_destroy(config);
}
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 60bdad8ffb..2b4709efb2 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -283,6 +283,7 @@ extern int max_maptrack_frames;
extern libxl_bitmap global_vm_affinity_mask;
extern libxl_bitmap global_hvm_affinity_mask;
extern libxl_bitmap global_pv_affinity_mask;
+extern libxl_domid domid_policy;
enum output_format {
OUTPUT_FORMAT_JSON,
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index e520b1da79..39292acfe6 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -899,6 +899,8 @@ start:
autoconnect_console_how = 0;
}
+ d_config.c_info.domid = domid_policy;
+
if ( restoring ) {
libxl_domain_restore_params params;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Xen-devel] [PATCH v5 7/7] xl: allow domid to be preserved on save/restore or migrate
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
` (5 preceding siblings ...)
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 6/7] xl.conf: introduce 'domid_policy' Paul Durrant
@ 2020-01-31 15:01 ` Paul Durrant
2020-02-17 17:55 ` Ian Jackson
2020-02-17 14:21 ` [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Durrant, Paul
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-01-31 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
This patch adds a '-D' command line option to save and migrate to allow
the domain id to be incorporated into the saved domain configuration and
hence be preserved.
NOTE: Logically it may seem as though preservation of domid should be
dealt with by libxl, but the libxl migration stream has no record
in which to transfer domid and remote domain creation occurs before
the migration stream is parsed. Hence this patch modifies xl rather
then libxl.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
v5:
- Expand the commit comment to say why it is xl being patched rather
than libxl
v2:
- Heavily re-worked based on new libxl_domain_create_info
---
docs/man/xl.1.pod.in | 14 ++++++++++++++
tools/xl/xl.h | 1 +
tools/xl/xl_cmdtable.c | 6 ++++--
tools/xl/xl_migrate.c | 15 ++++++++++-----
tools/xl/xl_saverestore.c | 19 ++++++++++++++-----
tools/xl/xl_vmcontrol.c | 3 ++-
6 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 33ad2ebd71..09339282e6 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -490,6 +490,13 @@ Display huge (!) amount of debug information during the migration process.
Leave the domain on the receive side paused after migration.
+=item B<-D>
+
+Preserve the B<domain-id> in the domain coniguration that is transferred
+such that it will be identical on the destination host, unless that
+configuration is overridden using the B<-C> option. Note that it is not
+possible to use this option for a 'localhost' migration.
+
=back
=item B<remus> [I<OPTIONS>] I<domain-id> I<host>
@@ -692,6 +699,13 @@ Leave the domain running after creating the snapshot.
Leave the domain paused after creating the snapshot.
+=item B<-D>
+
+Preserve the B<domain-id> in the domain coniguration that is embedded in
+the state file such that it will be identical when the domain is restored,
+unless that configuration is overridden. (See the B<restore> operation
+above).
+
=back
=item B<sharing> [I<domain-id>]
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 2b4709efb2..06569c6c4a 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -99,6 +99,7 @@ struct save_file_header {
#define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
void save_domain_core_begin(uint32_t domid,
+ int preserve_domid,
const char *override_config_file,
uint8_t **config_data_r,
int *config_len_r);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 3b302b2f20..08335394e5 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -153,7 +153,8 @@ struct cmd_spec cmd_table[] = {
"[options] <Domain> <CheckpointFile> [<ConfigFile>]",
"-h Print this help.\n"
"-c Leave domain running after creating the snapshot.\n"
- "-p Leave domain paused after creating the snapshot."
+ "-p Leave domain paused after creating the snapshot.\n"
+ "-D Store the domain id in the configration."
},
{ "migrate",
&main_migrate, 0, 1,
@@ -167,7 +168,8 @@ struct cmd_spec cmd_table[] = {
"-e Do not wait in the background (on <host>) for the death\n"
" of the domain.\n"
"--debug Print huge (!) amount of debug during the migration process.\n"
- "-p Do not unpause domain after migrating it."
+ "-p Do not unpause domain after migrating it.\n"
+ "-D Preserve the domain id"
},
{ "restore",
&main_restore, 0, 1,
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index 22f0429b84..0813beb801 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -176,7 +176,8 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
}
-static void migrate_domain(uint32_t domid, const char *rune, int debug,
+static void migrate_domain(uint32_t domid, int preserve_domid,
+ const char *rune, int debug,
const char *override_config_file)
{
pid_t child = -1;
@@ -187,7 +188,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
uint8_t *config_data;
int config_len, flags = LIBXL_SUSPEND_LIVE;
- save_domain_core_begin(domid, override_config_file,
+ save_domain_core_begin(domid, preserve_domid, override_config_file,
&config_data, &config_len);
if (!config_len) {
@@ -537,13 +538,14 @@ int main_migrate(int argc, char **argv)
char *rune = NULL;
char *host;
int opt, daemonize = 1, monitor = 1, debug = 0, pause_after_migration = 0;
+ int preserve_domid = 0;
static struct option opts[] = {
{"debug", 0, 0, 0x100},
{"live", 0, 0, 0x200},
COMMON_LONG_OPTS
};
- SWITCH_FOREACH_OPT(opt, "FC:s:ep", opts, "migrate", 2) {
+ SWITCH_FOREACH_OPT(opt, "FC:s:epD", opts, "migrate", 2) {
case 'C':
config_filename = optarg;
break;
@@ -560,6 +562,9 @@ int main_migrate(int argc, char **argv)
case 'p':
pause_after_migration = 1;
break;
+ case 'D':
+ preserve_domid = 1;
+ break;
case 0x100: /* --debug */
debug = 1;
break;
@@ -596,7 +601,7 @@ int main_migrate(int argc, char **argv)
pause_after_migration ? " -p" : "");
}
- migrate_domain(domid, rune, debug, config_filename);
+ migrate_domain(domid, preserve_domid, rune, debug, config_filename);
return EXIT_SUCCESS;
}
@@ -716,7 +721,7 @@ int main_remus(int argc, char **argv)
}
}
- save_domain_core_begin(domid, NULL, &config_data, &config_len);
+ save_domain_core_begin(domid, 0, NULL, &config_data, &config_len);
if (!config_len) {
fprintf(stderr, "No config file stored for running domain and "
diff --git a/tools/xl/xl_saverestore.c b/tools/xl/xl_saverestore.c
index 9be033fe65..953d791d1a 100644
--- a/tools/xl/xl_saverestore.c
+++ b/tools/xl/xl_saverestore.c
@@ -32,6 +32,7 @@
#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
void save_domain_core_begin(uint32_t domid,
+ int preserve_domid,
const char *override_config_file,
uint8_t **config_data_r,
int *config_len_r)
@@ -62,6 +63,8 @@ void save_domain_core_begin(uint32_t domid,
fprintf(stderr, "unable to retrieve domain configuration\n");
exit(EXIT_FAILURE);
}
+
+ d_config.c_info.domid = preserve_domid ? domid : 0;
}
config_c = libxl_domain_config_to_json(ctx, &d_config);
@@ -120,14 +123,15 @@ void save_domain_core_writeconfig(int fd, const char *source,
hdr.optional_data_len);
}
-static int save_domain(uint32_t domid, const char *filename, int checkpoint,
- int leavepaused, const char *override_config_file)
+static int save_domain(uint32_t domid, int preserve_domid,
+ const char *filename, int checkpoint,
+ int leavepaused, const char *override_config_file)
{
int fd;
uint8_t *config_data;
int config_len;
- save_domain_core_begin(domid, override_config_file,
+ save_domain_core_begin(domid, preserve_domid, override_config_file,
&config_data, &config_len);
if (!config_len) {
@@ -236,15 +240,19 @@ int main_save(int argc, char **argv)
const char *config_filename = NULL;
int checkpoint = 0;
int leavepaused = 0;
+ int preserve_domid = 0;
int opt;
- SWITCH_FOREACH_OPT(opt, "cp", NULL, "save", 2) {
+ SWITCH_FOREACH_OPT(opt, "cpD", NULL, "save", 2) {
case 'c':
checkpoint = 1;
break;
case 'p':
leavepaused = 1;
break;
+ case 'D':
+ preserve_domid = 1;
+ break;
}
if (argc-optind > 3) {
@@ -257,7 +265,8 @@ int main_save(int argc, char **argv)
if ( argc - optind >= 3 )
config_filename = argv[optind + 2];
- save_domain(domid, filename, checkpoint, leavepaused, config_filename);
+ save_domain(domid, preserve_domid, filename, checkpoint, leavepaused,
+ config_filename);
return EXIT_SUCCESS;
}
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 39292acfe6..2e2d427492 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -899,7 +899,8 @@ start:
autoconnect_console_how = 0;
}
- d_config.c_info.domid = domid_policy;
+ if (!libxl_domid_valid_guest(d_config.c_info.domid))
+ d_config.c_info.domid = domid_policy;
if ( restoring ) {
libxl_domain_restore_params params;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v5 7/7] xl: allow domid to be preserved on save/restore or migrate
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 7/7] xl: allow domid to be preserved on save/restore or migrate Paul Durrant
@ 2020-02-17 17:55 ` Ian Jackson
0 siblings, 0 replies; 21+ messages in thread
From: Ian Jackson @ 2020-02-17 17:55 UTC (permalink / raw)
To: Paul Durrant; +Cc: Anthony Perard, xen-devel, Wei Liu
Paul Durrant writes ("[PATCH v5 7/7] xl: allow domid to be preserved on save/restore or migrate"):
> This patch adds a '-D' command line option to save and migrate to allow
> the domain id to be incorporated into the saved domain configuration and
> hence be preserved.
>
> NOTE: Logically it may seem as though preservation of domid should be
> dealt with by libxl, but the libxl migration stream has no record
> in which to transfer domid and remote domain creation occurs before
> the migration stream is parsed. Hence this patch modifies xl rather
> then libxl.
Thanks.
I think I am satisfied that this is the best we can do without
tremendous amounts of reorganisation.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Regards,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes
2020-01-31 15:01 [Xen-devel] [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes Paul Durrant
` (6 preceding siblings ...)
2020-01-31 15:01 ` [Xen-devel] [PATCH v5 7/7] xl: allow domid to be preserved on save/restore or migrate Paul Durrant
@ 2020-02-17 14:21 ` Durrant, Paul
7 siblings, 0 replies; 21+ messages in thread
From: Durrant, Paul @ 2020-02-17 14:21 UTC (permalink / raw)
To: Durrant, Paul, xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jason Andryuk,
George Dunlap, Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson,
Jan Beulich, Anthony PERARD
Ping?
> -----Original Message-----
> From: Paul Durrant <pdurrant@amazon.com>
> Sent: 31 January 2020 15:02
> To: xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@citrix.com>;
> George Dunlap <George.Dunlap@eu.citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Jason
> Andryuk <jandryuk@gmail.com>; Julien Grall <julien@xen.org>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>
> Subject: [PATCH v5 0/7] xl/libxl: domid allocation/preservation changes
>
> Paul Durrant (7):
> libxl: add definition of INVALID_DOMID to the API
> libxl_create: make 'soft reset' explicit
> libxl: generalise libxl__domain_userdata_lock()
> libxl: add infrastructure to track and query 'recent' domids
> libxl: allow creation of domains with a specified or random domid
> xl.conf: introduce 'domid_policy'
> xl: allow domid to be preserved on save/restore or migrate
>
> docs/man/xl.1.pod.in | 14 +++
> docs/man/xl.conf.5.pod | 10 ++
> tools/examples/xl.conf | 4 +
> tools/helpers/xen-init-dom0.c | 30 +++++
> tools/libxl/libxl.h | 15 ++-
> tools/libxl/libxl_create.c | 125 ++++++++++++++-----
> tools/libxl/libxl_device.c | 4 +-
> tools/libxl/libxl_disk.c | 12 +-
> tools/libxl/libxl_dm.c | 2 +-
> tools/libxl/libxl_dom.c | 12 +-
> tools/libxl/libxl_domain.c | 218 ++++++++++++++++++++++++++++++++--
> tools/libxl/libxl_internal.c | 67 +++++++----
> tools/libxl/libxl_internal.h | 30 +++--
> tools/libxl/libxl_mem.c | 8 +-
> tools/libxl/libxl_pci.c | 4 +-
> tools/libxl/libxl_types.idl | 1 +
> tools/libxl/libxl_usb.c | 8 +-
> tools/xl/xl.c | 10 ++
> tools/xl/xl.h | 2 +
> tools/xl/xl_cmdtable.c | 6 +-
> tools/xl/xl_migrate.c | 15 ++-
> tools/xl/xl_saverestore.c | 19 ++-
> tools/xl/xl_utils.h | 2 -
> tools/xl/xl_vmcontrol.c | 3 +
> xen/include/public/xen.h | 3 +
> 25 files changed, 517 insertions(+), 107 deletions(-)
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Jason Andryuk <jandryuk@gmail.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Wei Liu <wl@xen.org>
> --
> 2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread