thr3ads.net - Xen devel - Fix users of xc_domain

If this information is useful, please help other people find it:
Share via:

Andrew Cooper

2013-Aug-12 17:31 UTC

Fix users of xc_domain_getinfo()

xc_domain_getinfo() has an insane interface, so much so that most in-tree
callers get it wrong.  This series implements some fixes.

It is RFC particularly to do with the issues of deprecating an API which is
currently being used.  Thoughts/suggestions appreciated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

Andrew Cooper

2013-Aug-12 17:31 UTC

head link

[RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.

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

Andrew Cooper

2013-Aug-12 17:31 UTC

head link

[RFC Patch 2/2] tools/xc_domain_getinfo: Implement sane interface for single domain information.

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

Ian Campbell

2013-Aug-13 21:38 UTC

head link

Re: [RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.

On Mon, 2013-08-12 at 18:31 +0100, Andrew Cooper wrote:> 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.
libxc doesn''t have any API guarantees so we don''t need to do
this for
that reason. Of course renaming makes it easier to validate that you
have caught all the callsites.

FWIW libxc has a few existing functions which are called foo_exact
rather than foo_single.
> 
> 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

Ian Jackson

2013-Aug-19 14:43 UTC

head link

Re: [RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.

Andrew Cooper writes ("[Xen-devel] [RFC Patch 1/2] tools/xc_domain_getinfo:
Fix callsites looking for more than 1 domain."):> xc_domain_getinfo has an absolutely insane interface, leading to it being
> misused by accident at almost all callsites.
I looked, in these two patches, for what you did to libxl.  But I
didn''t find the relevant code.  Have I missed your intent ?  You seem
to me to be claiming to have adjusted every call site...

Ian.

Ian Jackson

2013-Aug-19 14:45 UTC

head link

Re: [RFC Patch 2/2] tools/xc_domain_getinfo: Implement sane interface for single domain information.

Andrew Cooper writes ("[Xen-devel] [RFC Patch 2/2] tools/xc_domain_getinfo:
Implement sane interface for single domain information."):
...> +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);
...> -        info->dying    =
!!(domctl.u.getdomaininfo.flags&XEN_DOMINF_dying);
> -        info->shutdown =
!!(domctl.u.getdomaininfo.flags&XEN_DOMINF_shutdown);
This code motion makes the patch a bit harder to review than
necessary.

Would it be possible to split it into a separate patch ("introduce
xc_domaininfo_to_dominfo" I guess), or provide a "git diff -b" ?

I see you are using git.  If you had provided a public git url for
your branch then I would have just looked t6here and asked git for
diff -b ...

Ian.

Xen devel - Aug 2013 - Fix users of xc_domain_getinfo()

Fix users of xc_domain_getinfo()

[RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.

[RFC Patch 2/2] tools/xc_domain_getinfo: Implement sane interface for single domain information.

Re: [RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.

Re: [RFC Patch 1/2] tools/xc_domain_getinfo: Fix callsites looking for more than 1 domain.

Re: [RFC Patch 2/2] tools/xc_domain_getinfo: Implement sane interface for single domain information.