* [RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.
2013-08-12 17:31 Fix users of xc_domain_getinfo() Andrew Cooper
@ 2013-08-12 17:31 ` Andrew Cooper
2013-08-13 21:38 ` Ian Campbell
2013-08-19 14:43 ` Ian Jackson
2013-08-12 17:31 ` [RFC Patch 2/2] tools/xc_domain_getinfo: Implement sane interface for single domain information Andrew Cooper
1 sibling, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2013-08-12 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell
xc_domain_getinfo has an absolutely insane interface, leading to it being
misused by accident at almost all callsites.
This patch renames the current xc_domain_getinfo() sideways to
xc_domain_getinfo_many(), and provides a #define for compilation
compatibility.
The callsites which are actually using the ability to find more than a
specific domain are updated to to the _many() variant, while all other
callsites use the compatibility #define for now.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/console/daemon/io.c | 2 +-
tools/libxc/xc_domain.c | 8 ++++----
tools/libxc/xenctrl.h | 13 ++++++++-----
tools/python/xen/lowlevel/xc/xc.c | 2 +-
tools/xenmon/xenbaked.c | 2 +-
5 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 250550a..ba9a2bd 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -758,7 +758,7 @@ static void enum_domains(void)
enum_pass++;
- while (xc_domain_getinfo(xc, domid, 1, &dominfo) == 1) {
+ while (xc_domain_getinfo_many(xc, domid, 1, &dominfo) == 1) {
dom = lookup_domain(dominfo.domid);
if (dominfo.dying) {
if (dom)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 3257e2a..e49dfc2 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -271,10 +271,10 @@ out:
}
-int xc_domain_getinfo(xc_interface *xch,
- uint32_t first_domid,
- unsigned int max_doms,
- xc_dominfo_t *info)
+int xc_domain_getinfo_many(xc_interface *xch,
+ uint32_t first_domid,
+ unsigned int max_doms,
+ xc_dominfo_t *info)
{
unsigned int nr_doms;
uint32_t next_domid = first_domid;
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 388a9c3..86abec7 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -576,11 +576,14 @@ int xc_vcpu_getaffinity(xc_interface *xch,
* the enumerated domains.
* @return the number of domains enumerated or -1 on error
*/
-int xc_domain_getinfo(xc_interface *xch,
- uint32_t first_domid,
- unsigned int max_doms,
- xc_dominfo_t *info);
-
+int xc_domain_getinfo_many(xc_interface *xch,
+ uint32_t first_domid,
+ unsigned int max_doms,
+ xc_dominfo_t *info);
+
+/* Compatability for the moment for out of tree users. External callers
+ * should move over to using an explicit _single() or _many() */
+#define xc_domain_getinfo(x, f, m, i) xc_domain_getinfo_many((x), (f), (m), (i))
/**
* This function will set the execution context for the specified vcpu.
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index e611b24..2d30890 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -326,7 +326,7 @@ static PyObject *pyxc_domain_getinfo(XcObject *self,
if (info == NULL)
return PyErr_NoMemory();
- nr_doms = xc_domain_getinfo(self->xc_handle, first_dom, max_doms, info);
+ nr_doms = xc_domain_getinfo_many(self->xc_handle, first_dom, max_doms, info);
if (nr_doms < 0)
{
diff --git a/tools/xenmon/xenbaked.c b/tools/xenmon/xenbaked.c
index 985f1dd..d9c9bbe 100644
--- a/tools/xenmon/xenbaked.c
+++ b/tools/xenmon/xenbaked.c
@@ -794,7 +794,7 @@ static int indexof(int domid)
// call domaininfo hypercall to try and garbage collect unused entries
xc_handle = xc_interface_open(0,0,0);
- ndomains = xc_domain_getinfo(xc_handle, 0, NDOMAINS, dominfo);
+ ndomains = xc_domain_getinfo_many(xc_handle, 0, NDOMAINS, dominfo);
xc_interface_close(xc_handle);
// for each domain in our data, look for it in the system dominfo structure
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [RFC Patch 2/2] tools/xc_domain_getinfo: Implement sane interface for single domain information.
2013-08-12 17:31 Fix users of xc_domain_getinfo() Andrew Cooper
2013-08-12 17:31 ` [RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain Andrew Cooper
@ 2013-08-12 17:31 ` Andrew Cooper
2013-08-19 14:45 ` Ian Jackson
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2013-08-12 17:31 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell
Now that xc_domain_getinfo_many() exists, implement a rather more sane
interface for xc_domain_getinfo(), under the name xc_domain_getinfo_single(),
which will only return success if the information is for the requested domid.
As can be seen from the patch, only 3 of the in-tree callers correctly check
whether the information they have refers to the correct domain.
There are out-of-tree callers of xc_domain_getinfo, so removing the API is a
little antisocial at this point.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/console/client/main.c | 3 +-
tools/console/daemon/io.c | 6 +-
tools/debugger/kdd/kdd-xen.c | 2 +-
tools/libxc/xc_core.c | 2 +-
tools/libxc/xc_cpuid_x86.c | 2 +-
tools/libxc/xc_domain.c | 96 +++++++++++++-------
tools/libxc/xc_domain_save.c | 4 +-
tools/libxc/xc_offline_page.c | 2 +-
tools/libxc/xc_pagetab.c | 3 +-
tools/libxc/xc_resume.c | 4 +-
tools/libxc/xenctrl.h | 12 +++
tools/misc/xen-hvmcrash.c | 4 +-
tools/misc/xen-lowmemd.c | 2 +-
.../python/xen/lowlevel/checkpoint/libcheckpoint.c | 5 +-
tools/xenstore/xenstored_domain.c | 5 +-
tools/xentrace/xenctx.c | 4 +-
16 files changed, 97 insertions(+), 59 deletions(-)
diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 523fc23..ec1211d 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -339,7 +339,8 @@ int main(int argc, char **argv)
xc_interface *xc_handle = xc_interface_open(0,0,0);
if (xc_handle == NULL)
err(errno, "Could not open xc interface");
- xc_domain_getinfo(xc_handle, domid, 1, &xcinfo);
+ if (xc_domain_getinfo_single(xc_handle, domid, &xcinfo) != 0)
+ err(errno, "Unable to get domain information");
/* default to pv console for pv guests and serial for hvm guests */
if (xcinfo.hvm)
type = CONSOLE_SERIAL;
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index ba9a2bd..0f37188 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -251,13 +251,9 @@ static void buffer_advance(struct buffer *buffer, size_t len)
static bool domain_is_valid(int domid)
{
- bool ret;
xc_dominfo_t info;
- ret = (xc_domain_getinfo(xc, domid, 1, &info) == 1 &&
- info.domid == domid);
-
- return ret;
+ return xc_domain_getinfo_single(xc, domid, &info) == 0;
}
static int create_hv_log(void)
diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index 4fbea7d..9ebc3f1 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -593,7 +593,7 @@ kdd_guest *kdd_guest_init(char *arg, FILE *log, int verbosity)
g->domid = domid;
/* Check that the domain exists and is HVM */
- if (xc_domain_getinfo(xch, domid, 1, &info) != 1 || !info.hvm)
+ if (xc_domain_getinfo_single(xch, domid, &info) != 0 || !info.hvm)
goto err;
snprintf(g->id, (sizeof g->id) - 1,
diff --git a/tools/libxc/xc_core.c b/tools/libxc/xc_core.c
index 4207eed..66f7313 100644
--- a/tools/libxc/xc_core.c
+++ b/tools/libxc/xc_core.c
@@ -491,7 +491,7 @@ xc_domain_dumpcore_via_callback(xc_interface *xch,
goto out;
}
- if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
+ if ( xc_domain_getinfo_single(xch, domid, &info) != 0 )
{
PERROR("Could not get info for domain");
goto out;
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index fa47787..d18f45e 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -557,7 +557,7 @@ static int xc_cpuid_policy(
{
xc_dominfo_t info;
- if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 )
+ if ( xc_domain_getinfo_single(xch, domid, &info) != 0 )
return -EINVAL;
if ( info.hvm )
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index e49dfc2..da65b2c 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -270,6 +270,69 @@ out:
return ret;
}
+static void xc_domaininfo_to_dominfo(const xc_domaininfo_t *src,
+ xc_dominfo_t *dst)
+{
+ dst->dying = !!(src->flags & XEN_DOMINF_dying);
+ dst->shutdown = !!(src->flags & XEN_DOMINF_shutdown);
+ dst->paused = !!(src->flags & XEN_DOMINF_paused);
+ dst->blocked = !!(src->flags & XEN_DOMINF_blocked);
+ dst->running = !!(src->flags & XEN_DOMINF_running);
+ dst->hvm = !!(src->flags & XEN_DOMINF_hvm_guest);
+ dst->debugged = !!(src->flags & XEN_DOMINF_debugged);
+
+ dst->shutdown_reason =
+ (src->flags>>XEN_DOMINF_shutdownshift) &
+ XEN_DOMINF_shutdownmask;
+
+ if ( dst->shutdown && (dst->shutdown_reason == SHUTDOWN_crash) )
+ {
+ dst->shutdown = 0;
+ dst->crashed = 1;
+ }
+
+ dst->ssidref = src->ssidref;
+ dst->nr_pages = src->tot_pages;
+ dst->nr_outstanding_pages = src->outstanding_pages;
+ dst->nr_shared_pages = src->shr_pages;
+ dst->nr_paged_pages = src->paged_pages;
+ dst->max_memkb = src->max_pages << (PAGE_SHIFT-10);
+ dst->shared_info_frame = src->shared_info_frame;
+ dst->cpu_time = src->cpu_time;
+ dst->nr_online_vcpus = src->nr_online_vcpus;
+ dst->max_vcpu_id = src->max_vcpu_id;
+ dst->cpupool = src->cpupool;
+
+ memcpy(dst->handle, src->handle,
+ sizeof(xen_domain_handle_t));
+}
+
+int xc_domain_getinfo_single(xc_interface *xch,
+ uint32_t domid,
+ xc_dominfo_t *info)
+{
+ DECLARE_DOMCTL;
+ int rc = 0;
+
+ domctl.cmd = XEN_DOMCTL_getdomaininfo;
+ domctl.domain = (domid_t)domid;
+
+ rc = do_domctl(xch, &domctl);
+ if ( rc )
+ return rc;
+
+ if ( domctl.domain != domid ) {
+ errno = ESRCH;
+ return -1;
+ }
+
+ memset(info, 0, sizeof *info);
+
+ info->domid = domctl.domain;
+ xc_domaininfo_to_dominfo(&domctl.u.getdomaininfo, info);
+
+ return 0;
+}
int xc_domain_getinfo_many(xc_interface *xch,
uint32_t first_domid,
@@ -291,38 +354,7 @@ int xc_domain_getinfo_many(xc_interface *xch,
break;
info->domid = (uint16_t)domctl.domain;
- info->dying = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_dying);
- info->shutdown = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_shutdown);
- info->paused = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_paused);
- info->blocked = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_blocked);
- info->running = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_running);
- info->hvm = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_hvm_guest);
- info->debugged = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_debugged);
-
- info->shutdown_reason =
- (domctl.u.getdomaininfo.flags>>XEN_DOMINF_shutdownshift) &
- XEN_DOMINF_shutdownmask;
-
- if ( info->shutdown && (info->shutdown_reason == SHUTDOWN_crash) )
- {
- info->shutdown = 0;
- info->crashed = 1;
- }
-
- info->ssidref = domctl.u.getdomaininfo.ssidref;
- info->nr_pages = domctl.u.getdomaininfo.tot_pages;
- info->nr_outstanding_pages = domctl.u.getdomaininfo.outstanding_pages;
- info->nr_shared_pages = domctl.u.getdomaininfo.shr_pages;
- info->nr_paged_pages = domctl.u.getdomaininfo.paged_pages;
- info->max_memkb = domctl.u.getdomaininfo.max_pages << (PAGE_SHIFT-10);
- info->shared_info_frame = domctl.u.getdomaininfo.shared_info_frame;
- info->cpu_time = domctl.u.getdomaininfo.cpu_time;
- info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus;
- info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id;
- info->cpupool = domctl.u.getdomaininfo.cpupool;
-
- memcpy(info->handle, domctl.u.getdomaininfo.handle,
- sizeof(xen_domain_handle_t));
+ xc_domaininfo_to_dominfo(&domctl.u.getdomaininfo, info);
next_domid = (uint16_t)domctl.domain + 1;
info++;
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index fbc15e9..b42c1c5 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -365,7 +365,7 @@ static int suspend_and_state(int (*suspend)(void*), void* data,
return -1;
}
- if ( (xc_domain_getinfo(xch, dom, 1, info) != 1) ||
+ if ( (xc_domain_getinfo_single(xch, dom, info) != 0) ||
!info->shutdown || (info->shutdown_reason != SHUTDOWN_suspend) )
{
ERROR("Domain not in suspended state");
@@ -919,7 +919,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
return 1;
}
- if ( xc_domain_getinfo(xch, dom, 1, &info) != 1 )
+ if ( xc_domain_getinfo_single(xch, dom, &info) != 0 )
{
PERROR("Could not get domain info");
return 1;
diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index 36b9812..0405a3f 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -557,7 +557,7 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
uint32_t status;
xen_pfn_t new_mfn, gpfn;
- if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
+ if ( xc_domain_getinfo_single(xch, domid, &info) != 0 )
{
ERROR("Could not get domain info");
return -EFAULT;
diff --git a/tools/libxc/xc_pagetab.c b/tools/libxc/xc_pagetab.c
index 27c4e9f..387139b 100644
--- a/tools/libxc/xc_pagetab.c
+++ b/tools/libxc/xc_pagetab.c
@@ -35,8 +35,7 @@ unsigned long xc_translate_foreign_address(xc_interface *xch, uint32_t dom,
int size, level, pt_levels = 2;
void *map;
- if (xc_domain_getinfo(xch, dom, 1, &dominfo) != 1
- || dominfo.domid != dom)
+ if (xc_domain_getinfo_single(xch, dom, &dominfo) != 0)
return 0;
/* What kind of paging are we dealing with? */
diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index 1c43ec6..30de9bc 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -46,7 +46,7 @@ static int modify_returncode(xc_interface *xch, uint32_t domid)
struct domain_info_context *dinfo = &_dinfo;
int rc;
- if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
+ if ( xc_domain_getinfo_single(xch, domid, &info) != 0 )
{
PERROR("Could not get domain info");
return -1;
@@ -131,7 +131,7 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t domid)
xen_pfn_t *p2m = NULL;
#endif
- if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
+ if ( xc_domain_getinfo_single(xch, domid, &info) != 0 )
{
PERROR("Could not get domain info");
return rc;
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 86abec7..6f795e1 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -562,6 +562,18 @@ int xc_vcpu_getaffinity(xc_interface *xch,
xc_cpumap_t cpumap);
/**
+ * This function will return information about a single domain.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid the domain to get information information from.
+ * @parm info an xc_dominfo_t structure to hold the information.
+ * @return 0 on success or -1 on error
+ */
+int xc_domain_getinfo_single(xc_interface *xch,
+ uint32_t domid,
+ xc_dominfo_t *info);
+
+/**
* This function will return information about one or more domains. It is
* designed to iterate over the list of domains. If a single domain is
* requested, this function will return the next domain in the list - if
diff --git a/tools/misc/xen-hvmcrash.c b/tools/misc/xen-hvmcrash.c
index 4f0dabc..bdd60fc 100644
--- a/tools/misc/xen-hvmcrash.c
+++ b/tools/misc/xen-hvmcrash.c
@@ -66,8 +66,8 @@ main(int argc, char **argv)
exit(1);
}
- ret = xc_domain_getinfo(xch, domid, 1, &dominfo);
- if (ret < 0) {
+ ret = xc_domain_getinfo_single(xch, domid, &dominfo);
+ if (ret != 0) {
perror("xc_domain_getinfo");
exit(1);
}
diff --git a/tools/misc/xen-lowmemd.c b/tools/misc/xen-lowmemd.c
index 82ffd75..d5b6eb0 100644
--- a/tools/misc/xen-lowmemd.c
+++ b/tools/misc/xen-lowmemd.c
@@ -57,7 +57,7 @@ void handle_low_mem(void)
return;
diff = THRESHOLD_PG - free_pages;
- if (xc_domain_getinfo(xch, 0, 1, &dom0_info) < 1)
+ if (xc_domain_getinfo_single(xch, 0, &dom0_info) != 0)
{
perror("Failed to get dom0 info");
return;
diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
index 01c0d47..cacdfee 100644
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
@@ -97,7 +97,7 @@ int checkpoint_open(checkpoint_state* s, unsigned int domid)
return -1;
}
- if (xc_domain_getinfo(s->xch, s->domid, 1, &dominfo) < 0) {
+ if (xc_domain_getinfo_single(s->xch, s->domid, &dominfo) != 0) {
checkpoint_close(s);
s->errstr = "could not get domain info";
@@ -428,8 +428,7 @@ static int check_shutdown(checkpoint_state* s) {
break;
}
- if (xc_domain_getinfo(s->xch, s->domid, 1, &info) != 1
- || info.domid != s->domid) {
+ if (xc_domain_getinfo_single(s->xch, s->domid, &info) != 0) {
snprintf(errbuf, sizeof(errbuf),
"error getting info for domain %u", s->domid);
s->errstr = errbuf;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index bf83d58..e626593 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -217,9 +217,8 @@ static void domain_cleanup(void)
int notify = 0;
list_for_each_entry_safe(domain, tmp, &domains, list) {
- if (xc_domain_getinfo(*xc_handle, domain->domid, 1,
- &dominfo) == 1 &&
- dominfo.domid == domain->domid) {
+ if (xc_domain_getinfo_single(*xc_handle, domain->domid,
+ &dominfo) == 0) {
if ((dominfo.crashed || dominfo.shutdown)
&& !domain->shutdown) {
domain->shutdown = 1;
diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 060e480..e1ca581 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -896,8 +896,8 @@ int main(int argc, char **argv)
exit(-1);
}
- ret = xc_domain_getinfo(xenctx.xc_handle, xenctx.domid, 1, &xenctx.dominfo);
- if (ret < 0) {
+ ret = xc_domain_getinfo_single(xenctx.xc_handle, xenctx.domid, &xenctx.dominfo);
+ if (ret != 0) {
perror("xc_domain_getinfo");
exit(-1);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread