Re: [libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

2013-08-06 Thread Stefan Bader 
On 05.08.2013 19:52, Jim Fehlig wrote:
 
 libvirt typically uses a '*Internal' naming pattern for these types of
 internal functions, e.g. xenUnifiedDomainGetVcpusFlagsInternal.  Also as
 we touch this code we should strive to use the libvirt pattern of
 putting each parameter after the first on a new line when the list of
 parameters exceeds 80 columns.  Finally, since you added a line after
 the xenUnifiedNodeGetInfo declaration, we should add one here too.
 
Ok, changed.

 
 I don't think this comment is necessary.  Instead, just send a follow-up
 patch :).

Oh well, it was a kind of reminder, but beside of the doing it correct part,
the current usage is ok as there is no special checking between public and
private usage which could lock up... :)

 @@ -1501,7 +1533,7 @@ xenUnifiedDomainGetXMLDesc(virDomainPtr dom, unsigned 
 int flags)
  } else {
  char *cpus;
  xenUnifiedLock(priv);
 -cpus = xenDomainUsedCpus(dom);
 +cpus = xenDomainUsedCpus(dom, def);
   
 
 This should be minidef right?  Otherwise def is NULL, resulting in a
 segfault further down the call chain.


Absolutely right. I missed to do that in the version I forward ported to HEAD
since I did the fix and testing in an older version. :/
Good you spotted that.

Ok, I updated the patch as suggested (attached).

-Stefan

From 47ce666f6a4d91832883341c56f0a55181698e76 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 15 Jul 2013 16:03:58 +0200
Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

Since commit 95e18efd most public interfaces (xenUnified...) obtain
a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
lock.
This is already taken before calling xenDomainUsedCpus(), so we get
a deadlock for active guests. Avoid this by splitting up
xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
public and private function calls (which get the virDomainDefPtr passed)
and use those in xenDomainUsedCpus().

xenDomainUsedCpus
  ...
  nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
return xenUnifiedDomainGetVcpusFlags(...)
  ...
  if (!(def = xenGetDomainDefForDom(dom)))
return xenGetDomainDefForUUID(dom-conn, dom-uuid);
  ...
  ret = xenHypervisorLookupDomainByUUID(conn, uuid);
...
xenUnifiedLock(priv);
name = xenStoreDomainGetName(conn, id);
xenUnifiedUnlock(priv);
  ...
  if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
...
if (!(def = xenGetDomainDefForDom(dom)))
  [again like above]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/xen/xen_driver.c | 100 +++
 src/xen/xen_driver.h |   2 +-
 2 files changed, 70 insertions(+), 32 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 4ae38d3..e1fcbcc 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -73,12 +73,18 @@
 
 static int
 xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
+
 static int
-xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
+xenUnifiedDomainGetVcpusFlagsInternal(virDomainPtr dom,
+  virDomainDefPtr def,
+  unsigned int flags);
 static int
-xenUnifiedDomainGetVcpus(virDomainPtr dom,
- virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen);
+xenUnifiedDomainGetVcpusInternal(virDomainPtr dom,
+ virDomainDefPtr def,
+ virVcpuInfoPtr info,
+ int maxinfo,
+ unsigned char *cpumaps,
+ int maplen);
 
 
 static bool is_privileged = false;
@@ -173,6 +179,7 @@ xenNumaInit(virConnectPtr conn) {
 /**
  * xenDomainUsedCpus:
  * @dom: the domain
+ * @def: the domain definition
  *
  * Analyze which set of CPUs are used by the domain and
  * return a string providing the ranges.
@@ -181,7 +188,7 @@ xenNumaInit(virConnectPtr conn) {
  * NULL if the domain uses all CPU or in case of error.
  */
 char *
-xenDomainUsedCpus(virDomainPtr dom)
+xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def)
 {
 char *res = NULL;
 int ncpus;
@@ -202,7 +209,9 @@ xenDomainUsedCpus(virDomainPtr dom)
 
 if (priv-nbNodeCpus = 0)
 return NULL;
-nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
+nb_vcpu = xenUnifiedDomainGetVcpusFlagsInternal(dom, def,
+(VIR_DOMAIN_VCPU_LIVE |
+ VIR_DOMAIN_VCPU_MAXIMUM));
 if (nb_vcpu = 0)
 return NULL;
 if (xenUnifiedNodeGetInfo(dom-conn, nodeinfo)  0)
@@ -217,8 +226,8 @@ xenDomainUsedCpus(virDomainPtr dom)
 VIR_ALLOC_N(cpumap, nb_vcpu *

Re: [libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

2013-08-06 Thread Jim Fehlig 
Stefan Bader wrote:
 On 05.08.2013 19:52, Jim Fehlig wrote:
   
 libvirt typically uses a '*Internal' naming pattern for these types of
 internal functions, e.g. xenUnifiedDomainGetVcpusFlagsInternal.  Also as
 we touch this code we should strive to use the libvirt pattern of
 putting each parameter after the first on a new line when the list of
 parameters exceeds 80 columns.  Finally, since you added a line after
 the xenUnifiedNodeGetInfo declaration, we should add one here too.

 
 Ok, changed.

   
 I don't think this comment is necessary.  Instead, just send a follow-up
 patch :).
 

 Oh well, it was a kind of reminder, but beside of the doing it correct part,
 the current usage is ok as there is no special checking between public and
 private usage which could lock up... :)
   

Nod.

[...]

 From 47ce666f6a4d91832883341c56f0a55181698e76 Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Mon, 15 Jul 2013 16:03:58 +0200
 Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

 Since commit 95e18efd most public interfaces (xenUnified...) obtain
 a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
 lock.
 This is already taken before calling xenDomainUsedCpus(), so we get
 a deadlock for active guests. Avoid this by splitting up
 xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
 public and private function calls (which get the virDomainDefPtr passed)
 and use those in xenDomainUsedCpus().

 xenDomainUsedCpus
   ...
   nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
 return xenUnifiedDomainGetVcpusFlags(...)
   ...
   if (!(def = xenGetDomainDefForDom(dom)))
 return xenGetDomainDefForUUID(dom-conn, dom-uuid);
   ...
   ret = xenHypervisorLookupDomainByUUID(conn, uuid);
 ...
 xenUnifiedLock(priv);
 name = xenStoreDomainGetName(conn, id);
 xenUnifiedUnlock(priv);
   ...
   if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
 ...
 if (!(def = xenGetDomainDefForDom(dom)))
   [again like above]

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/xen/xen_driver.c | 100 
 +++
  src/xen/xen_driver.h |   2 +-
  2 files changed, 70 insertions(+), 32 deletions(-)

 diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
 index 4ae38d3..e1fcbcc 100644
 --- a/src/xen/xen_driver.c
 +++ b/src/xen/xen_driver.c
 @@ -73,12 +73,18 @@
  
  static int
  xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
 +
  static int
 -xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
 +xenUnifiedDomainGetVcpusFlagsInternal(virDomainPtr dom,
 +  virDomainDefPtr def,
 +  unsigned int flags);
   

Super nit: I added a line between these function declarations for
consistency.  ACK to the patch and now pushed.  Thanks Stefan!

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

2013-08-05 Thread Jim Fehlig 
Hi Stefan,

Sorry for missing this patch and thanks for the reminder poke...

Stefan Bader wrote:
 From 0df7a75cb777c2f616d5d0e16a6eb8b3db4b1f15 Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Mon, 15 Jul 2013 16:03:58 +0200
 Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

 Since commit 95e18efd most public interfaces (xenUnified...) obtain
 a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
 lock.
 This is already taken before calling xenDomainUsedCpus(), so we get
 a deadlock for active guests. Avoid this by splitting up
 xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
 public and private function calls (which get the virDomainDefPtr passed)
 and use those in xenDomainUsedCpus().

 xenDomainUsedCpus
   ...
   nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
 return xenUnifiedDomainGetVcpusFlags(...)
   ...
   if (!(def = xenGetDomainDefForDom(dom)))
 return xenGetDomainDefForUUID(dom-conn, dom-uuid);
   ...
   ret = xenHypervisorLookupDomainByUUID(conn, uuid);
 ...
 xenUnifiedLock(priv);
 name = xenStoreDomainGetName(conn, id);
 xenUnifiedUnlock(priv);
   ...
   if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
 ...
 if (!(def = xenGetDomainDefForDom(dom)))
   [again like above]

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/xen/xen_driver.c |   94 
 +-
  src/xen/xen_driver.h |2 +-
  2 files changed, 64 insertions(+), 32 deletions(-)

 diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
 index 39334b7..edd8447 100644
 --- a/src/xen/xen_driver.c
 +++ b/src/xen/xen_driver.c
 @@ -73,12 +73,14 @@
  
  static int
  xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
 +
  static int
 -xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
 +__xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, virDomainDefPtr def,
 +unsigned int flags);
   

libvirt typically uses a '*Internal' naming pattern for these types of
internal functions, e.g. xenUnifiedDomainGetVcpusFlagsInternal.  Also as
we touch this code we should strive to use the libvirt pattern of
putting each parameter after the first on a new line when the list of
parameters exceeds 80 columns.  Finally, since you added a line after
the xenUnifiedNodeGetInfo declaration, we should add one here too.

  static int
 -xenUnifiedDomainGetVcpus(virDomainPtr dom,
 - virVcpuInfoPtr info, int maxinfo,
 - unsigned char *cpumaps, int maplen);
 +__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
 +   virVcpuInfoPtr info, int maxinfo,
 +   unsigned char *cpumaps, int maplen);
   

Same comment here.

  
  
  static bool is_privileged = false;
 @@ -173,6 +175,7 @@ xenNumaInit(virConnectPtr conn) {
  /**
   * xenDomainUsedCpus:
   * @dom: the domain
 + * @def: the domain definition
   *
   * Analyze which set of CPUs are used by the domain and
   * return a string providing the ranges.
 @@ -181,7 +184,7 @@ xenNumaInit(virConnectPtr conn) {
   * NULL if the domain uses all CPU or in case of error.
   */
  char *
 -xenDomainUsedCpus(virDomainPtr dom)
 +xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def)
  {
  char *res = NULL;
  int ncpus;
 @@ -202,9 +205,14 @@ xenDomainUsedCpus(virDomainPtr dom)
  
  if (priv-nbNodeCpus = 0)
  return NULL;
 -nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
 +nb_vcpu = __xenUnifiedDomainGetVcpusFlags(dom, def,
 +  (VIR_DOMAIN_VCPU_LIVE |
 +   VIR_DOMAIN_VCPU_MAXIMUM));
  if (nb_vcpu = 0)
  return NULL;
 +/* FIXME: To be consistent this should map to an internal interface, too.
 + * Currently it actually does map straight to xenDaemonNodeGetInfo().
 + */
   

I don't think this comment is necessary.  Instead, just send a follow-up
patch :).

  if (xenUnifiedNodeGetInfo(dom-conn, nodeinfo)  0)
  return NULL;
  
 @@ -217,8 +225,8 @@ xenDomainUsedCpus(virDomainPtr dom)
  VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen)  0)
  goto done;
  
 -if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
 -  cpumap, cpumaplen)) = 0) {
 +if ((ncpus = __xenUnifiedDomainGetVcpus(dom, def, cpuinfo, nb_vcpu,
 +cpumap, cpumaplen)) = 0) {
  for (n = 0; n  ncpus; n++) {
  for (m = 0; m  priv-nbNodeCpus; m++) {
  bool used;
 @@ -1410,54 +1418,57 @@ cleanup:
  }
  
  static int
 -xenUnifiedDomainGetVcpus(virDomainPtr dom,
 - virVcpuInfoPtr info, int maxinfo,
 -

[libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

2013-07-16 Thread Stefan Bader 
Based on Daniel's feedback I did a split for public/private functions
for those that cause the lockup when getting XML. Maybe not complete
but at least seems to allow basic usage again (through virt-manager).

-Stefan

---

From f406c6891fb92a45dc5d5a4d794c5d667965d096 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 15 Jul 2013 16:03:58 +0200
Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

Since commit 95e18efd most public interfaces (xenUnified...) obtain
a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
lock.
This is already taken before calling xenDomainUsedCpus(), so we get
a deadlock for active guests. Avoid this by splitting up
xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
public and private function calls (which get the virDomainDefPtr passed)
and use those in xenDomainUsedCpus().

xenDomainUsedCpus
  ...
  nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
return xenUnifiedDomainGetVcpusFlags(...)
  ...
  if (!(def = xenGetDomainDefForDom(dom)))
return xenGetDomainDefForUUID(dom-conn, dom-uuid);
  ...
  ret = xenHypervisorLookupDomainByUUID(conn, uuid);
...
xenUnifiedLock(priv);
name = xenStoreDomainGetName(conn, id);
xenUnifiedUnlock(priv);
  ...
  if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
...
if (!(def = xenGetDomainDefForDom(dom)))
  [again like above]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/xen/xen_driver.c |   94 +-
 src/xen/xen_driver.h |2 +-
 2 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 39334b7..9ae728e 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -73,12 +73,14 @@
 
 static int
 xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
+
 static int
-xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
+__xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, virDomainDefPtr def,
+   unsigned int flags);
 static int
-xenUnifiedDomainGetVcpus(virDomainPtr dom,
- virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen);
+__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
+  virVcpuInfoPtr info, int maxinfo,
+  unsigned char *cpumaps, int maplen);
 
 
 static bool is_privileged = false;
@@ -173,6 +175,7 @@ xenNumaInit(virConnectPtr conn) {
 /**
  * xenDomainUsedCpus:
  * @dom: the domain
+ * @def: the domain definition
  *
  * Analyze which set of CPUs are used by the domain and
  * return a string providing the ranges.
@@ -181,7 +184,7 @@ xenNumaInit(virConnectPtr conn) {
  * NULL if the domain uses all CPU or in case of error.
  */
 char *
-xenDomainUsedCpus(virDomainPtr dom)
+xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def)
 {
 char *res = NULL;
 int ncpus;
@@ -202,9 +205,14 @@ xenDomainUsedCpus(virDomainPtr dom)
 
 if (priv-nbNodeCpus = 0)
 return NULL;
-nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
+nb_vcpu = __xenUnifiedDomainGetVcpusFlags(dom, def,
+ (VIR_DOMAIN_VCPU_LIVE |
+  VIR_DOMAIN_VCPU_MAXIMUM));
 if (nb_vcpu = 0)
 return NULL;
+/* FIXME: To be consistent this should map to an internal interface, too.
+ * Currently it actually does map straight to xenDaemonNodeGetInfo().
+ */
 if (xenUnifiedNodeGetInfo(dom-conn, nodeinfo)  0)
 return NULL;
 
@@ -217,8 +225,8 @@ xenDomainUsedCpus(virDomainPtr dom)
 VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen)  0)
 goto done;
 
-if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
-  cpumap, cpumaplen)) = 0) {
+if ((ncpus = __xenUnifiedDomainGetVcpus(dom, def, cpuinfo, nb_vcpu,
+   cpumap, cpumaplen)) = 0) {
 for (n = 0; n  ncpus; n++) {
 for (m = 0; m  priv-nbNodeCpus; m++) {
 bool used;
@@ -1410,54 +1418,57 @@ cleanup:
 }
 
 static int
-xenUnifiedDomainGetVcpus(virDomainPtr dom,
- virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen)
+__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
+  virVcpuInfoPtr info, int maxinfo,
+  unsigned char *cpumaps, int maplen)
 {
 xenUnifiedPrivatePtr priv = dom-conn-privateData;
-virDomainDefPtr def = NULL;
 int ret = -1;
 
-if (!(def = xenGetDomainDefForDom(dom)))
-goto cleanup;
-
-if (virDomainGetVcpusEnsureACL(dom-conn, def)  0)
-goto cleanup;
-
 if (dom-id  0) {