[libvirt] [PATCH v2 1/4] cpu: define sub-leaf 0 for leaf 7 in cpu_map.xml

[Marek Marczykowski-Górecki]

CPUID leaf 7 is sub-leaf aware. Add missing attribute.

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes since v1:
 - format ecx_in='0x00'
---
 src/cpu/cpu_map.xml | 58 +++---
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index 29b5b59..8e7ac49 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -195,93 +195,93 @@
 
 
 
-  
+  
 
  
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
 
-  
+  
 
 
-  
+  
 
 
 
-- 
git-series 0.9.1

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

[libvirt] [PATCH v2 2/4] libxl: add support for CPUID features policy

[Marek Marczykowski-Górecki]

Convert CPU features policy into libxl cpuid policy settings. Use new
("libxl") syntax, which allow to enable/disable specific bits, using
host CPU as a base. For this reason, accept only "host-passthrough"
mode.
Libxl do not have distinction between "force" and "required" policy
(there is only "force") and also between "forbid" and "disable" (there
is only "disable"). So, merge them appropriately. If anything, "require"
and "forbid" should be enforced outside of specific driver.

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes since v1:
 - use new ("libxl") syntax to set only bits explicitly mentioned in
 domain XML
---
 src/libxl/libxl_conf.c | 77 ---
 src/libxl/libxl_conf.h |  1 +-
 2 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a0a53c2..0cf51a8 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -291,6 +291,44 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
 return 0;
 }
 
+static const char *libxlTranslateCPUFeature(const char *virCPUFeatureName)
+{
+/* libxl use different names for some CPUID bits */
+if (STREQ(virCPUFeatureName, "cx16"))
+return "cmpxchg16";
+if (STREQ(virCPUFeatureName, "cid"))
+return "cntxid";
+if (STREQ(virCPUFeatureName, "ds_cpl"))
+return "dscpl";
+if (STREQ(virCPUFeatureName, "pclmuldq"))
+return "pclmulqdq";
+if (STREQ(virCPUFeatureName, "pni"))
+return "sse3";
+if (STREQ(virCPUFeatureName, "ht"))
+return "htt";
+if (STREQ(virCPUFeatureName, "pn"))
+return "psn";
+if (STREQ(virCPUFeatureName, "clflush"))
+return "clfsh";
+if (STREQ(virCPUFeatureName, "sep"))
+return "sysenter";
+if (STREQ(virCPUFeatureName, "cx8"))
+return "cmpxchg8";
+if (STREQ(virCPUFeatureName, "nodeid_msr"))
+return "nodeid";
+if (STREQ(virCPUFeatureName, "cr8legacy"))
+return "altmovcr8";
+if (STREQ(virCPUFeatureName, "lahf_lm"))
+return "lahfsahf";
+if (STREQ(virCPUFeatureName, "cmp_legacy"))
+return "cmplegacy";
+if (STREQ(virCPUFeatureName, "fxsr_opt"))
+return "ffxsr";
+if (STREQ(virCPUFeatureName, "pdpe1gb"))
+return "page1gb";
+return virCPUFeatureName;
+}
+
 static int
 libxlMakeDomBuildInfo(virDomainDefPtr def,
   libxl_ctx *ctx,
@@ -376,10 +414,18 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
   def->features[VIR_DOMAIN_FEATURE_ACPI] ==
   VIR_TRISTATE_SWITCH_ON);
 
-if (caps &&
-def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
+if (caps && def->cpu) {
 bool hasHwVirt = false;
 bool svm = false, vmx = false;
+char xlCPU[32];
+
+if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported cpu mode '%s'"),
+   virCPUModeTypeToString(def->cpu->mode));
+return -1;
+}
+
 
 if (ARCH_IS_X86(def->os.arch)) {
 vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, 
"vmx");
@@ -394,20 +440,43 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
 
 case VIR_CPU_FEATURE_DISABLE:
 case VIR_CPU_FEATURE_FORBID:
+snprintf(xlCPU,
+sizeof(xlCPU),
+"%s=0",
+libxlTranslateCPUFeature(
+def->cpu->features[i].name));
+if (libxl_cpuid_parse_config(&b_info->cpuid, 
xlCPU)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_("unsupported cpu feature '%s'"),
+def->cpu->features[i].name);
+return -1;
+}
 if ((vmx && STREQ(def->cpu->features[i].name, 
"vmx")) ||
-(svm && STREQ(def->cpu->features[i].name, 
"svm")))
+(svm && STREQ(def->cpu->features[i].name, 
"svm")))
 hasHwVirt = false;
 break;
 
 case VIR_CPU_FEATURE_FORCE:
 case VIR_CPU_FEATURE_REQUIRE:
+snprintf(xlCPU,
+sizeof(xlCPU),
+"%s=1",
+libxlTranslateCPUFeature(
+def->cpu->features[i].name));
+if (libxl_cpuid_parse_config(&b_info->cpuid, 
xlCPU)) {
+   

[libvirt] [PATCH v2 4/4] tests: check CPU features handling in libxl driver

[Marek Marczykowski-Górecki]

Test enabling/disabling individual CPU features and also setting
nested HVM support, which is also controlled by CPU features node.

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes since v1:
 - rewritten to Jim's test suite for libxl_domain_config generator

Cc: Jim Fehlig 
---
 tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +-
 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml  | 37 ++-
 tests/libxlxml2domconfigtest.c   |  1 +-
 3 files changed, 102 insertions(+)
 create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json
 create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml

diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json 
b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
new file mode 100644
index 000..234e592
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
@@ -0,0 +1,64 @@
+{
+"c_info": {
+"type": "hvm",
+"name": "XenGuest2",
+"uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+},
+"b_info": {
+"max_vcpus": 1,
+"avail_vcpus": [
+0
+],
+"max_memkb": 592896,
+"target_memkb": 403456,
+"video_memkb": 8192,
+"shadow_memkb": 5656,
+"cpuid": [
+{
+"leaf": 1,
+"ecx": "xx00",
+"edx": "xxx1"
+}
+],
+"sched_params": {
+"weight": 1000
+},
+"type.hvm": {
+"pae": "True",
+"apic": "True",
+"acpi": "True",
+"vga": {
+
+},
+"nested_hvm": "False",
+"vnc": {
+"enable": "False"
+},
+"sdl": {
+"enable": "False"
+},
+"spice": {
+
+},
+"boot": "c",
+"rdm": {
+
+}
+},
+"arch_arm": {
+
+}
+},
+"disks": [
+{
+"pdev_path": "/dev/HostVG/XenGuest2",
+"vdev": "hda",
+"backend": "phy",
+"format": "raw",
+"removable": 1,
+"readwrite": 1
+}
+],
+"on_reboot": "restart",
+"on_crash": "restart"
+}
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml 
b/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
new file mode 100644
index 000..e9eba2e
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
@@ -0,0 +1,37 @@
+
+  XenGuest2
+  c7a5fdb2-cdaf-9455-926a-d65c16db1809
+  592896
+  403456
+  1
+  
+hvm
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+  destroy
+  restart
+  restart
+  
+
+  
+  
+  
+  
+
+
+
+
+  
+
+  
+
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index ecdb1fe..1627e9e 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -189,6 +189,7 @@ mymain(void)
 DO_TEST("basic-pv");
 DO_TEST("basic-hvm");
 DO_TEST("moredevs-hvm");
+DO_TEST("fullvirt-cpuid");
 
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
git-series 0.9.1

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

[libvirt] [PATCH v2 0/4] Add setting CPU features (CPUID) with libxenlight driver.

[Marek Marczykowski-Górecki]

Tests (patches 3 and 4) depends on libxl_domain_config test suite:
https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html

But first two patches can be applied independently.

Marek Marczykowski-Górecki (4):
  cpu: define sub-leaf 0 for leaf 7 in cpu_map.xml
  libxl: add support for CPUID features policy
  tests: switch libxlxml2domconfig test to use testXLInintCaps
  tests: check CPU features handling in libxl driver

 src/cpu/cpu_map.xml  | 58 ++---
 src/libxl/libxl_conf.c   | 77 -
 src/libxl/libxl_conf.h   |  1 +-
 tests/libxlxml2domconfigdata/basic-pv.xml|  2 +-
 tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 ++-
 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml  | 37 -
 tests/libxlxml2domconfigtest.c   |  3 +-
 7 files changed, 207 insertions(+), 35 deletions(-)
 create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json
 create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml

base-commit: fbcc08b245bb7d5502633d9cb4048281cc9d8532
-- 
git-series 0.9.1

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

[libvirt] [PATCH v2 3/4] tests: switch libxlxml2domconfig test to use testXLInintCaps

[Marek Marczykowski-Górecki]

As name suggests, it's a better choice for libxl test. Important
differences:
 - advertise x86_64 guests support
 - initialize host CPU caps

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes since v1:
 - new patch, applicable over Jim's test suite for libxl_domain_config
   generator

Cc: Jim Fehlig 
---
 tests/libxlxml2domconfigdata/basic-pv.xml | 2 +-
 tests/libxlxml2domconfigtest.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/libxlxml2domconfigdata/basic-pv.xml 
b/tests/libxlxml2domconfigdata/basic-pv.xml
index b3bc601..eba3cc6 100644
--- a/tests/libxlxml2domconfigdata/basic-pv.xml
+++ b/tests/libxlxml2domconfigdata/basic-pv.xml
@@ -6,7 +6,7 @@
   4
   pygrub
   
-linux
+linux
   
   
   destroy
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index d943cf2..ecdb1fe 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -173,7 +173,7 @@ mymain(void)
 return EXIT_FAILURE;
 }
 
-if ((xencaps = testXenCapsInit()) == NULL)
+if ((xencaps = testXLInitCaps()) == NULL)
 return EXIT_FAILURE;
 
 # define DO_TEST(name)  \
-- 
git-series 0.9.1

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

[libvirt] [PATCH v4 11/14] nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList

[John Ferlan]

Move the structures to withing virnodedeviceobj.c

Move the typedefs from node_device_conf to virnodedeviceobj.h

Signed-off-by: John Ferlan 
---
 src/conf/node_device_conf.h | 17 -
 src/conf/virnodedeviceobj.c | 11 +++
 src/conf/virnodedeviceobj.h |  6 ++
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 90c7e1f..d10683d 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -328,23 +328,6 @@ struct _virNodeDeviceDef {
 virNodeDevCapsDefPtr caps; /* optional device capabilities */
 };
 
-
-typedef struct _virNodeDeviceObj virNodeDeviceObj;
-typedef virNodeDeviceObj *virNodeDeviceObjPtr;
-struct _virNodeDeviceObj {
-virMutex lock;
-
-virNodeDeviceDefPtr def;   /* device definition */
-
-};
-
-typedef struct _virNodeDeviceObjList virNodeDeviceObjList;
-typedef virNodeDeviceObjList *virNodeDeviceObjListPtr;
-struct _virNodeDeviceObjList {
-size_t count;
-virNodeDeviceObjPtr *objs;
-};
-
 char *
 virNodeDeviceDefFormat(const virNodeDeviceDef *def);
 
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 7ebd4e8..04dba70 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -32,6 +32,17 @@
 
 VIR_LOG_INIT("conf.virnodedeviceobj");
 
+struct _virNodeDeviceObj {
+virMutex lock;
+
+virNodeDeviceDefPtr def;   /* device definition */
+};
+
+struct _virNodeDeviceObjList {
+size_t count;
+virNodeDeviceObjPtr *objs;
+};
+
 
 static virNodeDeviceObjPtr
 virNodeDeviceObjNew(virNodeDeviceDefPtr def)
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 6ec5ee7..1122b67 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -27,6 +27,12 @@
 # include "object_event.h"
 
 
+typedef struct _virNodeDeviceObj virNodeDeviceObj;
+typedef virNodeDeviceObj *virNodeDeviceObjPtr;
+
+typedef struct _virNodeDeviceObjList virNodeDeviceObjList;
+typedef virNodeDeviceObjList *virNodeDeviceObjListPtr;
+
 typedef struct _virNodeDeviceDriverState virNodeDeviceDriverState;
 typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr;
 struct _virNodeDeviceDriverState {
-- 
2.9.4

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

[libvirt] [PATCH v4 12/14] nodedev: Convert virNodeDeviceObj to use virObjectLockable

[John Ferlan]

Now that we have a bit more control, let's convert our object into
a lockable object and let that magic handle the create and lock/unlock.

This also involves creating a virNodeDeviceEndAPI in order to handle
the object cleaup for API's that use the Add or Find API's in order
to get a locked/reffed object. The EndAPI will unlock and unref the
object returning NULL to indicate to the caller to not use the obj.

For now this also involves a forward reference to the Unlock, but
that'll be removed shortly when the object is convert to use the
virObjectLockable shortly.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c  | 156 ++-
 src/conf/virnodedeviceobj.h  |  11 +--
 src/libvirt_private.syms |   4 +-
 src/node_device/node_device_driver.c |  18 ++--
 src/node_device/node_device_hal.c|   8 +-
 src/node_device/node_device_udev.c   |  12 +--
 src/test/test_driver.c   |  34 
 7 files changed, 119 insertions(+), 124 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 04dba70..3ce54fc 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -33,7 +33,7 @@
 VIR_LOG_INIT("conf.virnodedeviceobj");
 
 struct _virNodeDeviceObj {
-virMutex lock;
+virObjectLockable parent;
 
 virNodeDeviceDefPtr def;   /* device definition */
 };
@@ -44,27 +44,63 @@ struct _virNodeDeviceObjList {
 };
 
 
+static virClassPtr virNodeDeviceObjClass;
+static void virNodeDeviceObjDispose(void *opaque);
+
+static int
+virNodeDeviceObjOnceInit(void)
+{
+if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLockable(),
+  "virNodeDeviceObj",
+  sizeof(virNodeDeviceObj),
+  virNodeDeviceObjDispose)))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virNodeDeviceObj)
+
+
+static void
+virNodeDeviceObjDispose(void *opaque)
+{
+virNodeDeviceObjPtr obj = opaque;
+
+virNodeDeviceDefFree(obj->def);
+}
+
+
 static virNodeDeviceObjPtr
 virNodeDeviceObjNew(virNodeDeviceDefPtr def)
 {
 virNodeDeviceObjPtr obj;
 
-if (VIR_ALLOC(obj) < 0)
+if (virNodeDeviceObjInitialize() < 0)
 return NULL;
 
-if (virMutexInit(&obj->lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("cannot initialize mutex"));
-VIR_FREE(obj);
+if (!(obj = virObjectLockableNew(virNodeDeviceObjClass)))
 return NULL;
-}
-virNodeDeviceObjLock(obj);
+
+virObjectLock(obj);
 obj->def = def;
 
 return obj;
 }
 
 
+void
+virNodeDeviceObjEndAPI(virNodeDeviceObjPtr *obj)
+{
+if (!*obj)
+return;
+
+virObjectUnlock(*obj);
+virObjectUnref(*obj);
+*obj = NULL;
+}
+
+
 virNodeDeviceDefPtr
 virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
 {
@@ -186,13 +222,13 @@ 
virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
 virNodeDeviceObjPtr obj = devs->objs[i];
 virNodeDeviceDefPtr def;
 
-virNodeDeviceObjLock(obj);
+virObjectLock(obj);
 def = obj->def;
 if ((def->sysfs_path != NULL) &&
 (STREQ(def->sysfs_path, sysfs_path))) {
-return obj;
+return virObjectRef(obj);
 }
-virNodeDeviceObjUnlock(obj);
+virObjectUnlock(obj);
 }
 
 return NULL;
@@ -209,11 +245,11 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr 
devs,
 virNodeDeviceObjPtr obj = devs->objs[i];
 virNodeDeviceDefPtr def;
 
-virNodeDeviceObjLock(obj);
+virObjectLock(obj);
 def = obj->def;
 if (STREQ(def->name, name))
-return obj;
-virNodeDeviceObjUnlock(obj);
+return virObjectRef(obj);
+virObjectUnlock(obj);
 }
 
 return NULL;
@@ -231,13 +267,13 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr 
devs,
 virNodeDeviceObjPtr obj = devs->objs[i];
 virNodeDevCapsDefPtr cap;
 
-virNodeDeviceObjLock(obj);
+virObjectLock(obj);
 if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
 STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) &&
 STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) &&
 virNodeDeviceFindVPORTCapDef(obj))
-return obj;
-virNodeDeviceObjUnlock(obj);
+return virObjectRef(obj);
+virObjectUnlock(obj);
 }
 
 return NULL;
@@ -254,12 +290,12 @@ 
virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs,
 virNodeDeviceObjPtr obj = devs->objs[i];
 virNodeDevCapsDefPtr cap;
 
-virNodeDeviceObjLock(obj);
+virObjectLock(obj);
 if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
 STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) 
&&
 virNodeDeviceFindVPORTCapDe

[libvirt] [PATCH v4 13/14] nodedev: Convert virNodeDeviceObjListPtr to use hash tables

[John Ferlan]

Rather than use a forward linked list of elements, it'll be much more
efficient to use a hash table to reference the elements by unique name
and to perform hash searches.

This patch does all the heavy lifting of converting the list object to
use a self locking list that contains the hash table. Each of the FindBy
functions that do not involve finding the object by it's key (name) is
converted to use virHashSearch in order to find the specific object.
When searching for the key (name), it's possible to use virHashLookup.
For any of the list perusal functions that are required to evaluate
each object, the virHashForEach function is used.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 580 ++--
 1 file changed, 404 insertions(+), 176 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 3ce54fc..9def83f 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -25,6 +25,7 @@
 #include "viralloc.h"
 #include "virnodedeviceobj.h"
 #include "virerror.h"
+#include "virhash.h"
 #include "virlog.h"
 #include "virstring.h"
 
@@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
 };
 
 struct _virNodeDeviceObjList {
-size_t count;
-virNodeDeviceObjPtr *objs;
+virObjectLockable parent;
+
+/* name string -> virNodeDeviceObj  mapping
+ * for O(1), lockless lookup-by-uuid */
+virHashTable *objs;
+
 };
 
 
 static virClassPtr virNodeDeviceObjClass;
+static virClassPtr virNodeDeviceObjListClass;
 static void virNodeDeviceObjDispose(void *opaque);
+static void virNodeDeviceObjListDispose(void *opaque);
 
 static int
 virNodeDeviceObjOnceInit(void)
@@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
   virNodeDeviceObjDispose)))
 return -1;
 
+if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(),
+  "virNodeDeviceObjList",
+  sizeof(virNodeDeviceObjList),
+  
virNodeDeviceObjListDispose)))
+return -1;
+
 return 0;
 }
 
@@ -212,26 +225,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
 }
 
 
+static int
+virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
+const void *name ATTRIBUTE_UNUSED,
+const void *opaque)
+{
+virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+virNodeDeviceDefPtr def;
+const char *sysfs_path = opaque;
+int want = 0;
+
+virObjectLock(obj);
+def = obj->def;
+if (STREQ_NULLABLE(def->sysfs_path, sysfs_path))
+want = 1;
+virObjectUnlock(obj);
+return want;
+}
+
+
 virNodeDeviceObjPtr
 virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
 const char *sysfs_path)
 {
-size_t i;
+virNodeDeviceObjPtr obj;
 
-for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjPtr obj = devs->objs[i];
-virNodeDeviceDefPtr def;
+virObjectLock(devs);
+obj = virHashSearch(devs->objs, 
virNodeDeviceObjListFindBySysfsPathCallback,
+(void *)sysfs_path);
+virObjectRef(obj);
+virObjectUnlock(devs);
 
+if (obj)
 virObjectLock(obj);
-def = obj->def;
-if ((def->sysfs_path != NULL) &&
-(STREQ(def->sysfs_path, sysfs_path))) {
-return virObjectRef(obj);
-}
-virObjectUnlock(obj);
-}
 
-return NULL;
+return obj;
+}
+
+
+static virNodeDeviceObjPtr
+virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs,
+ const char *name)
+{
+return virObjectRef(virHashLookup(devs->objs, name));
 }
 
 
@@ -239,20 +275,42 @@ virNodeDeviceObjPtr
 virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
const char *name)
 {
-size_t i;
-
-for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjPtr obj = devs->objs[i];
-virNodeDeviceDefPtr def;
+virNodeDeviceObjPtr obj;
 
+virObjectLock(devs);
+obj = virNodeDeviceObjListFindByNameLocked(devs, name);
+virObjectUnlock(devs);
+if (obj)
 virObjectLock(obj);
-def = obj->def;
-if (STREQ(def->name, name))
-return virObjectRef(obj);
-virObjectUnlock(obj);
-}
 
-return NULL;
+return obj;
+}
+
+
+struct virNodeDeviceObjListFindByWWNsData {
+const char *parent_wwnn;
+const char *parent_wwpn;
+};
+
+static int
+virNodeDeviceObjListFindByWWNsCallback(const void *payload,
+   const void *name ATTRIBUTE_UNUSED,
+   const void *opaque)
+{
+virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+struct virNodeDeviceObjListFindByWWNsData *data =
+(

[libvirt] [PATCH v4 02/14] test: Adjust cleanup/error paths for nodedev test APIs

[John Ferlan]

 - In testDestroyVport rather than use a cleanup label, just return -1
   immediately since nothing else is needed.

 - In testStoragePoolDestroy, if !privpool, then just return -1 since
   nothing else will happen anyway.

 - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
   an @obj, just return directly.  This then allows the cleanup: label code
   to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
   This also simplifies some exit logic...

 - In testNodeDeviceObjFindByName use an error: label to handle the failure
   and don't do the ncaps++ within the VIR_STRDUP() source target index.
   Only increment ncaps after success. Easier on eyes at error label too.

 - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto

Signed-off-by: John Ferlan 
---
 src/test/test_driver.c | 78 +++---
 1 file changed, 30 insertions(+), 48 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 04887e0..2889ffa 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4509,7 +4509,6 @@ testDestroyVport(testDriverPtr privconn,
  const char *wwnn ATTRIBUTE_UNUSED,
  const char *wwpn ATTRIBUTE_UNUSED)
 {
-int ret = -1;
 virNodeDeviceObjPtr obj = NULL;
 virObjectEventPtr event = NULL;
 
@@ -4523,7 +4522,7 @@ testDestroyVport(testDriverPtr privconn,
 if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) {
 virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
_("no node device with matching name 'scsi_host12'"));
-goto cleanup;
+return -1;
 }
 
 event = virNodeDeviceEventLifecycleNew("scsi_host12",
@@ -4532,15 +4531,9 @@ testDestroyVport(testDriverPtr privconn,
 
 virNodeDeviceObjRemove(&privconn->devs, obj);
 virNodeDeviceObjFree(obj);
-obj = NULL;
 
-ret = 0;
-
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
 testObjectEventQueue(privconn, event);
-return ret;
+return 0;
 }
 
 
@@ -4553,7 +4546,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
 virObjectEventPtr event = NULL;
 
 if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name)))
-goto cleanup;
+return -1;
 
 if (!virStoragePoolObjIsActive(privpool)) {
 virReportError(VIR_ERR_OPERATION_INVALID,
@@ -5328,7 +5321,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char 
*name)
 virNodeDevicePtr ret = NULL;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, name)))
-goto cleanup;
+return NULL;
 def = virNodeDeviceObjGetDef(obj);
 
 if ((ret = virGetNodeDevice(conn, name))) {
@@ -5338,9 +5331,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char 
*name)
 }
 }
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -5355,13 +5346,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 virCheckFlags(0, NULL);
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return NULL;
 
 ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj));
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -5374,7 +5363,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
 char *ret = NULL;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return NULL;
 def = virNodeDeviceObjGetDef(obj);
 
 if (def->parent) {
@@ -5384,9 +5373,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
"%s", _("no parent for this device"));
 }
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -5399,20 +5386,16 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev)
 virNodeDeviceDefPtr def;
 virNodeDevCapsDefPtr caps;
 int ncaps = 0;
-int ret = -1;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return -1;
 def = virNodeDeviceObjGetDef(obj);
 
 for (caps = def->caps; caps; caps = caps->next)
 ++ncaps;
-ret = ncaps;
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
-return ret;
+virNodeDeviceObjUnlock(obj);
+return ncaps;
 }
 
 
@@ -5424,27 +5407,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char 
**const names, int maxnames)
 virNodeDeviceDefPtr def;
 virNodeDevCapsDefPtr caps;
 int ncaps = 0;
-int ret = -1;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return -1;
 def = virNodeDeviceObjGetDef(obj);
 
 for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) {
-if (VIR_STRDUP(names[ncaps++], 
virNodeDevCapTypeToString(caps->data.type)) < 0)
-goto cleanup;
+   

[libvirt] [PATCH v4 07/14] nodedev: Alter node device obj list function names

[John Ferlan]

Ensure that any function that walks the node device object list is prefixed
by virNodeDeviceObjList.

Also, modify the @filter param name for virNodeDeviceObjListExport to
be @aclfilter.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c  | 109 ++-
 src/conf/virnodedeviceobj.h  |  42 +++---
 src/libvirt_private.syms |  14 ++---
 src/node_device/node_device_driver.c |  20 +++
 src/node_device/node_device_hal.c|  12 ++--
 src/node_device/node_device_udev.c   |  14 ++---
 src/test/test_driver.c   |  28 -
 7 files changed, 121 insertions(+), 118 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 0c89790..f91f4a0 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -166,8 +166,8 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
 
 
 virNodeDeviceObjPtr
-virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs,
-const char *sysfs_path)
+virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
+const char *sysfs_path)
 {
 size_t i;
 
@@ -185,8 +185,8 @@ virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr 
devs,
 
 
 virNodeDeviceObjPtr
-virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs,
-   const char *name)
+virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
+   const char *name)
 {
 size_t i;
 
@@ -202,9 +202,9 @@ virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs,
 
 
 static virNodeDeviceObjPtr
-virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs,
-const char *parent_wwnn,
-const char *parent_wwpn)
+virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs,
+   const char *parent_wwnn,
+   const char *parent_wwpn)
 {
 size_t i;
 
@@ -224,8 +224,8 @@ virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs,
 
 
 static virNodeDeviceObjPtr
-virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs,
- const char *parent_fabric_wwn)
+virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs,
+const char *parent_fabric_wwn)
 {
 size_t i;
 
@@ -244,8 +244,8 @@ virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs,
 
 
 static virNodeDeviceObjPtr
-virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs,
-   const char *cap)
+virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs,
+  const char *cap)
 {
 size_t i;
 
@@ -297,12 +297,12 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
 
 
 virNodeDeviceObjPtr
-virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
-  virNodeDeviceDefPtr def)
+virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
+  virNodeDeviceDefPtr def)
 {
 virNodeDeviceObjPtr obj;
 
-if ((obj = virNodeDeviceObjFindByName(devs, def->name))) {
+if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) {
 virNodeDeviceDefFree(obj->def);
 obj->def = def;
 return obj;
@@ -323,8 +323,8 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 
 void
-virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-   virNodeDeviceObjPtr obj)
+virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
+   virNodeDeviceObjPtr obj)
 {
 size_t i;
 
@@ -373,14 +373,14 @@ virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj)
 
 
 static int
-virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs,
-   const char *dev_name,
-   const char *parent_name)
+virNodeDeviceObjListGetParentHostByParent(virNodeDeviceObjListPtr devs,
+  const char *dev_name,
+  const char *parent_name)
 {
 virNodeDeviceObjPtr obj = NULL;
 int ret;
 
-if (!(obj = virNodeDeviceObjFindByName(devs, parent_name))) {
+if (!(obj = virNodeDeviceObjListFindByName(devs, parent_name))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not find parent device for '%s'"),
dev_name);
@@ -396,15 +396,16 @@ 
virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs,
 
 
 static int
-virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs,
- const char *dev_name,
- const char *parent_wwnn,
- const char *parent_wwpn)
+virNodeDeviceObjListGetParentHostByWWNs(virNodeDeviceObjListPtr devs,
+const char *dev_name,
+const char *parent_ww

[libvirt] [PATCH v4 14/14] nodedev: Remove driver locks around object list mgmt code

[John Ferlan]

Since virnodedeviceobj now has a self-lockable hash table, there's no
need to lock the table from the driver for processing. Thus remove the
locks from the driver for NodeDeviceObjList mgmt.

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c | 61 +++-
 src/node_device/node_device_hal.c| 16 ++
 src/node_device/node_device_udev.c   | 12 +++
 3 files changed, 18 insertions(+), 71 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 0a19908..f3a6cfc 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -174,19 +174,13 @@ nodeNumOfDevices(virConnectPtr conn,
  const char *cap,
  unsigned int flags)
 {
-int ndevs = 0;
-
 if (virNodeNumOfDevicesEnsureACL(conn) < 0)
 return -1;
 
 virCheckFlags(0, -1);
 
-nodeDeviceLock();
-ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap,
- virNodeNumOfDevicesCheckACL);
-nodeDeviceUnlock();
-
-return ndevs;
+return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap,
+virNodeNumOfDevicesCheckACL);
 }
 
 
@@ -197,20 +191,14 @@ nodeListDevices(virConnectPtr conn,
 int maxnames,
 unsigned int flags)
 {
-int nnames;
-
 if (virNodeListDevicesEnsureACL(conn) < 0)
 return -1;
 
 virCheckFlags(0, -1);
 
-nodeDeviceLock();
-nnames = virNodeDeviceObjListGetNames(driver->devs, conn,
-  virNodeListDevicesCheckACL,
-  cap, names, maxnames);
-nodeDeviceUnlock();
-
-return nnames;
+return virNodeDeviceObjListGetNames(driver->devs, conn,
+virNodeListDevicesCheckACL,
+cap, names, maxnames);
 }
 
 
@@ -219,19 +207,14 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
   virNodeDevicePtr **devices,
   unsigned int flags)
 {
-int ret = -1;
-
 virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1);
 
 if (virConnectListAllNodeDevicesEnsureACL(conn) < 0)
 return -1;
 
-nodeDeviceLock();
-ret = virNodeDeviceObjListExport(conn, driver->devs, devices,
- virConnectListAllNodeDevicesCheckACL,
- flags);
-nodeDeviceUnlock();
-return ret;
+return virNodeDeviceObjListExport(conn, driver->devs, devices,
+  virConnectListAllNodeDevicesCheckACL,
+  flags);
 }
 
 
@@ -240,11 +223,7 @@ nodeDeviceObjFindByName(const char *name)
 {
 virNodeDeviceObjPtr obj;
 
-nodeDeviceLock();
-obj = virNodeDeviceObjListFindByName(driver->devs, name);
-nodeDeviceUnlock();
-
-if (!obj) {
+if (!(obj = virNodeDeviceObjListFindByName(driver->devs, name))) {
 virReportError(VIR_ERR_NO_NODE_DEVICE,
_("no node device with matching name '%s'"),
name);
@@ -294,11 +273,8 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 
 virCheckFlags(0, NULL);
 
-nodeDeviceLock();
-obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn);
-nodeDeviceUnlock();
-
-if (!obj)
+if (!(obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs,
+   wwnn, wwpn)))
 return NULL;
 
 def = virNodeDeviceObjGetDef(obj);
@@ -509,13 +485,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
 virNodeDevicePtr device = NULL;
 time_t start = 0, now = 0;
 
-/* The thread that creates the device takes the driver lock, so we
- * must release it in order to allow the device to be created.
- * We're not doing anything with the driver pointer at this point,
- * so it's safe to release it, assuming that the pointer itself
- * doesn't become invalid.  */
-nodeDeviceUnlock();
-
 nodeDeviceGetTime(&start);
 
 while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) {
@@ -532,8 +501,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
 break;
 }
 
-nodeDeviceLock();
-
 return device;
 }
 
@@ -552,8 +519,6 @@ nodeDeviceCreateXML(virConnectPtr conn,
 virCheckFlags(0, NULL);
 virt_type  = virConnectGetType(conn);
 
-nodeDeviceLock();
-
 if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, 
virt_type)))
 goto cleanup;
 
@@ -580,7 +545,6 @@ nodeDeviceCreateXML(virConnectPtr conn,
  "wwnn '%s' and wwpn '%s'"),
def->name, wwnn, wwpn);
  cleanup:
-nodeDeviceUnlock();
 virNodeDeviceDefFree(def);
 VIR_FREE(wwnn);
 VIR_FREE(wwpn);
@@ -601,8 +565,6 @@ nodeDevi

[libvirt] [PATCH v4 00/14] Make virNodeDeviceObj and virNodeDeviceObjList private

[John Ferlan]

v3 here:
https://www.redhat.com/archives/libvir-list/2017-June/msg00155.html

Most patches were ACK'd in v3 (some with conditions).  Changes this time
around are:

-> Patch1:
  Make the call to virNodeDeviceObjFree in dev_refresh() prior to dev_create
  Move the virNodeDeviceObjFree in device_removed() to after the Unlock as
   it avoids the need for { } else { } (besides the *ObjFree checks for !dev
   anyway).

-> Patch2:
  Change as requested to remove the unnecessary obj = NULL in testDestroyVport
  Change as requested to goto cleanup in testStoragePoolDestroy

-> Patch3:
  Fixed the comments to have the right variable name

-> Patch4:
  Use "if ((obj = ...))) {" type syntax as suggested.

-> Patch5-9:
  No change

-> Patch10:
  Fix commit description

-> Patch11:
  No change

-> Patch 12:
  Fix the testNodeDeviceDestroy logic

-> Patch 13-14
  New patches that take the next two logical steps - objectifying the
   virNodeDeviceObjList and using a HashTable for access (along with all
   those bells and whistles).  Finally remove the need for driver level
   locks when accessing the @devs ObjList. 

John Ferlan (14):
  nodedev: Alter virNodeDeviceObjRemove
  test: Adjust cleanup/error paths for nodedev test APIs
  nodedev: Use common naming for virnodedeviceobj
  nodedev: Use consistent names for driver variables
  nodedev: Introduce virNodeDeviceObjNew
  nodedev: Introduce virNodeDeviceObjListNew
  nodedev: Alter node device obj list function names
  nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
  nodedev: Introduce virNodeDeviceGetSCSIHostCaps
  nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs
  nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList
  nodedev: Convert virNodeDeviceObj to use virObjectLockable
  nodedev: Convert virNodeDeviceObjListPtr to use hash tables
  nodedev: Remove driver locks around object list mgmt code

 src/conf/node_device_conf.c   |  82 +++
 src/conf/node_device_conf.h   |  20 +-
 src/conf/virnodedeviceobj.c   | 846 --
 src/conf/virnodedeviceobj.h   |  67 +--
 src/libvirt_private.syms  |  20 +-
 src/node_device/node_device_driver.c  | 201 +++
 src/node_device/node_device_hal.c |  51 +-
 src/node_device/node_device_linux_sysfs.c |  77 +--
 src/node_device/node_device_udev.c|  63 +--
 src/test/test_driver.c| 133 +++--
 10 files changed, 895 insertions(+), 665 deletions(-)

-- 
2.9.4

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

[libvirt] [PATCH v4 10/14] nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs

[John Ferlan]

In an overall effort to privatize access to virNodeDeviceObj and
virNodeDeviceObjList into the virnodedeviceobj module, move the
object list parsing from node_device_driver and replace with a
call to a virnodedeviceobj helper. This follows other similar
APIs/helpers which peruse the object list looking for some specific
data in order to get/return an @device (virNodeDevice) object to
the caller.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c  | 33 +
 src/conf/virnodedeviceobj.h  |  5 
 src/libvirt_private.syms |  1 +
 src/node_device/node_device_driver.c | 56 +++-
 4 files changed, 55 insertions(+), 40 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 1dbaf83..7ebd4e8 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs,
 }
 
 
+virNodeDeviceObjPtr
+virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
+   const char *wwnn,
+   const char *wwpn)
+{
+size_t i;
+
+for (i = 0; i < devs->count; i++) {
+virNodeDeviceObjPtr obj = devs->objs[i];
+virNodeDevCapsDefPtr cap;
+
+virNodeDeviceObjLock(obj);
+cap = obj->def->caps;
+
+while (cap) {
+if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
+virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
+if (cap->data.scsi_host.flags &
+VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
+if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
+STREQ(cap->data.scsi_host.wwpn, wwpn))
+return obj;
+}
+}
+cap = cap->next;
+}
+virNodeDeviceObjUnlock(obj);
+}
+
+return NULL;
+}
+
+
 void
 virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
 {
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 6194c6c..6ec5ee7 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -53,6 +53,11 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr 
devs,
 ATTRIBUTE_NONNULL(2);
 
 virNodeDeviceObjPtr
+virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
+   const char *wwnn,
+   const char *wwpn);
+
+virNodeDeviceObjPtr
 virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
   virNodeDeviceDefPtr def);
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index acd123f..589e587 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -969,6 +969,7 @@ virNodeDeviceObjListAssignDef;
 virNodeDeviceObjListExport;
 virNodeDeviceObjListFindByName;
 virNodeDeviceObjListFindBySysfsPath;
+virNodeDeviceObjListFindSCSIHostByWWNs;
 virNodeDeviceObjListFree;
 virNodeDeviceObjListGetNames;
 virNodeDeviceObjListGetParentHost;
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 930f9b6..85a7c88 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
   const char *wwpn,
   unsigned int flags)
 {
-size_t i;
-virNodeDeviceObjListPtr devs = driver->devs;
-virNodeDevCapsDefPtr cap = NULL;
 virNodeDeviceObjPtr obj = NULL;
 virNodeDeviceDefPtr def;
 virNodeDevicePtr device = NULL;
@@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 virCheckFlags(0, NULL);
 
 nodeDeviceLock();
+obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn);
+nodeDeviceUnlock();
 
-for (i = 0; i < devs->count; i++) {
-obj = devs->objs[i];
-virNodeDeviceObjLock(obj);
-def = virNodeDeviceObjGetDef(obj);
-cap = def->caps;
-
-while (cap) {
-if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
-nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host);
-if (cap->data.scsi_host.flags &
-VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
-if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
-STREQ(cap->data.scsi_host.wwpn, wwpn)) {
-
-if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, 
def) < 0)
-goto error;
-
-if ((device = virGetNodeDevice(conn, def->name))) {
-if (VIR_STRDUP(device->parent, def->parent) < 0) {
-virObjectUnref(device);
-device = NULL;
-}
-}
-virNodeDevic

[libvirt] [PATCH v4 03/14] nodedev: Use common naming for virnodedeviceobj

[John Ferlan]

A virNodeDeviceObjPtr is an @obj

A virNodeDeviceObjListPtr is an @devs

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 130 ++--
 1 file changed, 65 insertions(+), 65 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index fa73de1..619c32d 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -41,10 +41,10 @@ virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
 
 
 static int
-virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
+virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
const char *cap)
 {
-virNodeDevCapsDefPtr caps = dev->def->caps;
+virNodeDevCapsDefPtr caps = obj->def->caps;
 const char *fc_host_cap =
 virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST);
 const char *vports_cap =
@@ -97,7 +97,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
 
 
 /* virNodeDeviceFindFCCapDef:
- * @dev: Pointer to current device
+ * @obj: Pointer to current device
  *
  * Search the device object 'caps' array for fc_host capability.
  *
@@ -105,9 +105,9 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
  * Pointer to the caps or NULL if not found
  */
 static virNodeDevCapsDefPtr
-virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev)
+virNodeDeviceFindFCCapDef(const virNodeDeviceObj *obj)
 {
-virNodeDevCapsDefPtr caps = dev->def->caps;
+virNodeDevCapsDefPtr caps = obj->def->caps;
 
 while (caps) {
 if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST &&
@@ -121,7 +121,7 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev)
 
 
 /* virNodeDeviceFindVPORTCapDef:
- * @dev: Pointer to current device
+ * @obj: Pointer to current device
  *
  * Search the device object 'caps' array for vport_ops capability.
  *
@@ -129,9 +129,9 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev)
  * Pointer to the caps or NULL if not found
  */
 static virNodeDevCapsDefPtr
-virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *dev)
+virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
 {
-virNodeDevCapsDefPtr caps = dev->def->caps;
+virNodeDevCapsDefPtr caps = obj->def->caps;
 
 while (caps) {
 if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST &&
@@ -240,16 +240,16 @@ virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs,
 
 
 void
-virNodeDeviceObjFree(virNodeDeviceObjPtr dev)
+virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
 {
-if (!dev)
+if (!obj)
 return;
 
-virNodeDeviceDefFree(dev->def);
+virNodeDeviceDefFree(obj->def);
 
-virMutexDestroy(&dev->lock);
+virMutexDestroy(&obj->lock);
 
-VIR_FREE(dev);
+VIR_FREE(obj);
 }
 
 
@@ -268,48 +268,48 @@ virNodeDeviceObjPtr
 virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
   virNodeDeviceDefPtr def)
 {
-virNodeDeviceObjPtr device;
+virNodeDeviceObjPtr obj;
 
-if ((device = virNodeDeviceObjFindByName(devs, def->name))) {
-virNodeDeviceDefFree(device->def);
-device->def = def;
-return device;
+if ((obj = virNodeDeviceObjFindByName(devs, def->name))) {
+virNodeDeviceDefFree(obj->def);
+obj->def = def;
+return obj;
 }
 
-if (VIR_ALLOC(device) < 0)
+if (VIR_ALLOC(obj) < 0)
 return NULL;
 
-if (virMutexInit(&device->lock) < 0) {
+if (virMutexInit(&obj->lock) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot initialize mutex"));
-VIR_FREE(device);
+VIR_FREE(obj);
 return NULL;
 }
-virNodeDeviceObjLock(device);
+virNodeDeviceObjLock(obj);
 
-if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, device) < 0) {
-virNodeDeviceObjUnlock(device);
-virNodeDeviceObjFree(device);
+if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) {
+virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjFree(obj);
 return NULL;
 }
-device->def = def;
+obj->def = def;
 
-return device;
+return obj;
 
 }
 
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-   virNodeDeviceObjPtr dev)
+   virNodeDeviceObjPtr obj)
 {
 size_t i;
 
-virNodeDeviceObjUnlock(dev);
+virNodeDeviceObjUnlock(obj);
 
 for (i = 0; i < devs->count; i++) {
 virNodeDeviceObjLock(devs->objs[i]);
-if (devs->objs[i] == dev) {
+if (devs->objs[i] == obj) {
 virNodeDeviceObjUnlock(devs->objs[i]);
 
 VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
@@ -324,7 +324,7 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
  * Return the NPIV dev's parent device name
  */
 /* virNodeDeviceFindFCParentHost:
- * @parent: Pointer to node device object
+ * @obj: Pointer to node device object
  *
  * Search the capabilities for the device to find the FC capabilities
  * in order to set the parent_host value.
@@ -333,15 +333,15 @@ vir

[libvirt] [PATCH v4 09/14] nodedev: Introduce virNodeDeviceGetSCSIHostCaps

[John Ferlan]

We're about to move the call to nodeDeviceSysfsGetSCSIHostCaps from
node_device_driver into virnodedeviceobj, so move the guts of the code
from the driver specific node_device_linux_sysfs into its own API
since virnodedeviceobj cannot callback into the driver.

Nothing in the code deals with sysfs anyway, as that's hidden by the
various virSCSIHost* and virVHBA* utility function calls.

Signed-off-by: John Ferlan 
---
 src/conf/node_device_conf.c   | 82 +++
 src/conf/node_device_conf.h   |  3 ++
 src/libvirt_private.syms  |  1 +
 src/node_device/node_device_linux_sysfs.c | 77 +
 4 files changed, 87 insertions(+), 76 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index e5947e6..503b129 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2483,3 +2483,85 @@ virNodeDeviceDeleteVport(virConnectPtr conn,
 VIR_FREE(scsi_host_name);
 return ret;
 }
+
+
+int
+virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host)
+{
+char *tmp = NULL;
+int ret = -1;
+
+if ((scsi_host->unique_id =
+ virSCSIHostGetUniqueId(NULL, scsi_host->host)) < 0) {
+VIR_DEBUG("Failed to read unique_id for host%d", scsi_host->host);
+scsi_host->unique_id = -1;
+}
+
+VIR_DEBUG("Checking if host%d is an FC HBA", scsi_host->host);
+
+if (virVHBAPathExists(NULL, scsi_host->host)) {
+scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
+
+if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "port_name"))) {
+VIR_WARN("Failed to read WWPN for host%d", scsi_host->host);
+goto cleanup;
+}
+VIR_FREE(scsi_host->wwpn);
+VIR_STEAL_PTR(scsi_host->wwpn, tmp);
+
+if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "node_name"))) {
+VIR_WARN("Failed to read WWNN for host%d", scsi_host->host);
+goto cleanup;
+}
+VIR_FREE(scsi_host->wwnn);
+VIR_STEAL_PTR(scsi_host->wwnn, tmp);
+
+if ((tmp = virVHBAGetConfig(NULL, scsi_host->host, "fabric_name"))) {
+VIR_FREE(scsi_host->fabric_wwn);
+VIR_STEAL_PTR(scsi_host->fabric_wwn, tmp);
+}
+}
+
+if (virVHBAIsVportCapable(NULL, scsi_host->host)) {
+scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS;
+
+if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
+ "max_npiv_vports"))) {
+VIR_WARN("Failed to read max_npiv_vports for host%d",
+ scsi_host->host);
+goto cleanup;
+}
+
+if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) {
+VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp);
+goto cleanup;
+}
+
+VIR_FREE(tmp);
+if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
+  "npiv_vports_inuse"))) {
+VIR_WARN("Failed to read npiv_vports_inuse for host%d",
+ scsi_host->host);
+goto cleanup;
+}
+
+if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) {
+VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp);
+goto cleanup;
+}
+}
+
+ret = 0;
+ cleanup:
+if (ret < 0) {
+/* Clear the two flags in case of producing confusing XML output */
+scsi_host->flags &= ~(VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST |
+  VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS);
+
+VIR_FREE(scsi_host->wwnn);
+VIR_FREE(scsi_host->wwpn);
+VIR_FREE(scsi_host->fabric_wwn);
+}
+VIR_FREE(tmp);
+return ret;
+}
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 0a5e731..90c7e1f 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -408,4 +408,7 @@ int
 virNodeDeviceDeleteVport(virConnectPtr conn,
  virStorageAdapterFCHostPtr fchost);
 
+int
+virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host);
+
 #endif /* __VIR_NODE_DEVICE_CONF_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f81a035..acd123f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -687,6 +687,7 @@ virNodeDeviceDefParseNode;
 virNodeDeviceDefParseString;
 virNodeDeviceDeleteVport;
 virNodeDeviceGetParentName;
+virNodeDeviceGetSCSIHostCaps;
 virNodeDeviceGetWWNs;
 
 
diff --git a/src/node_device/node_device_linux_sysfs.c 
b/src/node_device/node_device_linux_sysfs.c
index e02c384..6f438e5 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -48,82 +48,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs");
 int
 nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host)
 {
-char *tmp = NULL;
-int ret = -1;
-
-if ((scsi_

[libvirt] [PATCH v4 05/14] nodedev: Introduce virNodeDeviceObjNew

[John Ferlan]

Create an allocator for the virNodeDeviceObjPtr - include setting up
the mutex, saving the virNodeDeviceDefPtr, and locking the return object.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 34 +++---
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 619c32d..9a2b5de 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -33,6 +33,27 @@
 VIR_LOG_INIT("conf.virnodedeviceobj");
 
 
+static virNodeDeviceObjPtr
+virNodeDeviceObjNew(virNodeDeviceDefPtr def)
+{
+virNodeDeviceObjPtr obj;
+
+if (VIR_ALLOC(obj) < 0)
+return NULL;
+
+if (virMutexInit(&obj->lock) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("cannot initialize mutex"));
+VIR_FREE(obj);
+return NULL;
+}
+virNodeDeviceObjLock(obj);
+obj->def = def;
+
+return obj;
+}
+
+
 virNodeDeviceDefPtr
 virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
 {
@@ -276,26 +297,17 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 return obj;
 }
 
-if (VIR_ALLOC(obj) < 0)
+if (!(obj = virNodeDeviceObjNew(def)))
 return NULL;
 
-if (virMutexInit(&obj->lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("cannot initialize mutex"));
-VIR_FREE(obj);
-return NULL;
-}
-virNodeDeviceObjLock(obj);
-
 if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) {
+obj->def = NULL;
 virNodeDeviceObjUnlock(obj);
 virNodeDeviceObjFree(obj);
 return NULL;
 }
-obj->def = def;
 
 return obj;
-
 }
 
 
-- 
2.9.4

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

[libvirt] [PATCH v4 08/14] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs

[John Ferlan]

Create local @obj and @def for the API's rather than referencing the
devs->objs[i][->def->].  It'll make future patches easier to read.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 60 -
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index f91f4a0..1dbaf83 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -172,12 +172,16 @@ 
virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
 size_t i;
 
 for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjLock(devs->objs[i]);
-if ((devs->objs[i]->def->sysfs_path != NULL) &&
-(STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
-return devs->objs[i];
+virNodeDeviceObjPtr obj = devs->objs[i];
+virNodeDeviceDefPtr def;
+
+virNodeDeviceObjLock(obj);
+def = obj->def;
+if ((def->sysfs_path != NULL) &&
+(STREQ(def->sysfs_path, sysfs_path))) {
+return obj;
 }
-virNodeDeviceObjUnlock(devs->objs[i]);
+virNodeDeviceObjUnlock(obj);
 }
 
 return NULL;
@@ -191,10 +195,14 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr 
devs,
 size_t i;
 
 for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjLock(devs->objs[i]);
-if (STREQ(devs->objs[i]->def->name, name))
-return devs->objs[i];
-virNodeDeviceObjUnlock(devs->objs[i]);
+virNodeDeviceObjPtr obj = devs->objs[i];
+virNodeDeviceDefPtr def;
+
+virNodeDeviceObjLock(obj);
+def = obj->def;
+if (STREQ(def->name, name))
+return obj;
+virNodeDeviceObjUnlock(obj);
 }
 
 return NULL;
@@ -209,14 +217,16 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr 
devs,
 size_t i;
 
 for (i = 0; i < devs->count; i++) {
+virNodeDeviceObjPtr obj = devs->objs[i];
 virNodeDevCapsDefPtr cap;
-virNodeDeviceObjLock(devs->objs[i]);
-if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) &&
+
+virNodeDeviceObjLock(obj);
+if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
 STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) &&
 STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) &&
-virNodeDeviceFindVPORTCapDef(devs->objs[i]))
-return devs->objs[i];
-virNodeDeviceObjUnlock(devs->objs[i]);
+virNodeDeviceFindVPORTCapDef(obj))
+return obj;
+virNodeDeviceObjUnlock(obj);
 }
 
 return NULL;
@@ -230,13 +240,15 @@ 
virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs,
 size_t i;
 
 for (i = 0; i < devs->count; i++) {
+virNodeDeviceObjPtr obj = devs->objs[i];
 virNodeDevCapsDefPtr cap;
-virNodeDeviceObjLock(devs->objs[i]);
-if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) &&
+
+virNodeDeviceObjLock(obj);
+if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
 STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) 
&&
-virNodeDeviceFindVPORTCapDef(devs->objs[i]))
-return devs->objs[i];
-virNodeDeviceObjUnlock(devs->objs[i]);
+virNodeDeviceFindVPORTCapDef(obj))
+return obj;
+virNodeDeviceObjUnlock(obj);
 }
 
 return NULL;
@@ -250,10 +262,12 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr 
devs,
 size_t i;
 
 for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjLock(devs->objs[i]);
-if (virNodeDeviceObjHasCap(devs->objs[i], cap))
-return devs->objs[i];
-virNodeDeviceObjUnlock(devs->objs[i]);
+virNodeDeviceObjPtr obj = devs->objs[i];
+
+virNodeDeviceObjLock(obj);
+if (virNodeDeviceObjHasCap(obj, cap))
+return obj;
+virNodeDeviceObjUnlock(obj);
 }
 
 return NULL;
-- 
2.9.4

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

[libvirt] [PATCH v4 01/14] nodedev: Alter virNodeDeviceObjRemove

[John Ferlan]

Rather than passing the object to be removed by reference, pass by value
and then let the caller decide whether or not the object should be free'd
and how to handle the logic afterwards. This includes free'ing the object
and/or setting the local variable to NULL to prevent subsequent unexpected
usage (via something like virNodeDeviceObjRemove in testNodeDeviceDestroy).

For now this function will just handle the remove of the object from the
list for which it was placed during virNodeDeviceObjAssignDef.

This essentially reverts logic from commit id '61148074' that free'd the
device entry on list, set *dev = NULL and returned. Thus fixing a bug in
node_device_hal.c/dev_refresh() which would never call dev_create(udi)
since @dev would have been set to NULL.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c| 14 ++
 src/conf/virnodedeviceobj.h|  2 +-
 src/libvirt_private.syms   |  1 +
 src/node_device/node_device_hal.c  |  9 ++---
 src/node_device/node_device_udev.c |  3 ++-
 src/test/test_driver.c |  8 ++--
 6 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index e78f451..fa73de1 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -301,23 +301,21 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-   virNodeDeviceObjPtr *dev)
+   virNodeDeviceObjPtr dev)
 {
 size_t i;
 
-virNodeDeviceObjUnlock(*dev);
+virNodeDeviceObjUnlock(dev);
 
 for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjLock(*dev);
-if (devs->objs[i] == *dev) {
-virNodeDeviceObjUnlock(*dev);
-virNodeDeviceObjFree(devs->objs[i]);
-*dev = NULL;
+virNodeDeviceObjLock(devs->objs[i]);
+if (devs->objs[i] == dev) {
+virNodeDeviceObjUnlock(devs->objs[i]);
 
 VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
 break;
 }
-virNodeDeviceObjUnlock(*dev);
+virNodeDeviceObjUnlock(devs->objs[i]);
 }
 }
 
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 05a9d11..9bc02ee 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -58,7 +58,7 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-   virNodeDeviceObjPtr *dev);
+   virNodeDeviceObjPtr dev);
 
 int
 virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 888412a..e6ed981 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -965,6 +965,7 @@ virNetworkObjUpdateAssignDef;
 virNodeDeviceObjAssignDef;
 virNodeDeviceObjFindByName;
 virNodeDeviceObjFindBySysfsPath;
+virNodeDeviceObjFree;
 virNodeDeviceObjGetDef;
 virNodeDeviceObjGetNames;
 virNodeDeviceObjGetParentHost;
diff --git a/src/node_device/node_device_hal.c 
b/src/node_device/node_device_hal.c
index f468e42..dc9202b 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -514,14 +514,16 @@ dev_refresh(const char *udi)
 /* Simply "rediscover" device -- incrementally handling changes
  * to sub-capabilities (like net.80203) is nasty ... so avoid it.
  */
-virNodeDeviceObjRemove(&driver->devs, &dev);
+virNodeDeviceObjRemove(&driver->devs, dev);
 } else {
 VIR_DEBUG("no device named %s", name);
 }
 nodeDeviceUnlock();
 
-if (dev)
+if (dev) {
+virNodeDeviceObjFree(dev);
 dev_create(udi);
+}
 }
 
 static void
@@ -544,10 +546,11 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED,
 dev = virNodeDeviceObjFindByName(&driver->devs, name);
 VIR_DEBUG("%s", name);
 if (dev)
-virNodeDeviceObjRemove(&driver->devs, &dev);
+virNodeDeviceObjRemove(&driver->devs, dev);
 else
 VIR_DEBUG("no device named %s", name);
 nodeDeviceUnlock();
+virNodeDeviceObjFree(dev);
 }
 
 
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 174124a..819e4e7 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1333,7 +1333,8 @@ udevRemoveOneDevice(struct udev_device *device)
 
 VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
   def->name, name);
-virNodeDeviceObjRemove(&driver->devs, &dev);
+virNodeDeviceObjRemove(&driver->devs, dev);
+virNodeDeviceObjFree(dev);
 
 if (event)
 virObjectEventStateQueue(driver->nodeDeviceEventState, event);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 54e252c..04887e0 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4530,7 +4530,9 @@ testDestroyVport(testDriverPtr

[libvirt] [PATCH v4 06/14] nodedev: Introduce virNodeDeviceObjListNew

[John Ferlan]

In preparation to make things private, make the ->devs be pointers to a
virNodeDeviceObjList and then manage everything inside virnodedeviceobj

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c  | 13 -
 src/conf/virnodedeviceobj.h  |  5 -
 src/libvirt_private.syms |  1 +
 src/node_device/node_device_driver.c | 14 +++---
 src/node_device/node_device_hal.c| 19 +++
 src/node_device/node_device_udev.c   | 18 +++---
 src/test/test_driver.c   | 29 +++--
 7 files changed, 61 insertions(+), 38 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 9a2b5de..0c89790 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -274,6 +274,17 @@ virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
 }
 
 
+virNodeDeviceObjListPtr
+virNodeDeviceObjListNew(void)
+{
+virNodeDeviceObjListPtr devs;
+
+if (VIR_ALLOC(devs) < 0)
+return NULL;
+return devs;
+}
+
+
 void
 virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
 {
@@ -281,7 +292,7 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
 for (i = 0; i < devs->count; i++)
 virNodeDeviceObjFree(devs->objs[i]);
 VIR_FREE(devs->objs);
-devs->count = 0;
+VIR_FREE(devs);
 }
 
 
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 9bc02ee..77250a0 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -32,7 +32,7 @@ typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr;
 struct _virNodeDeviceDriverState {
 virMutex lock;
 
-virNodeDeviceObjList devs; /* currently-known devices */
+virNodeDeviceObjListPtr devs;  /* currently-known devices */
 void *privateData; /* driver-specific private data */
 
 /* Immutable pointer, self-locking APIs */
@@ -68,6 +68,9 @@ virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs,
 void
 virNodeDeviceObjFree(virNodeDeviceObjPtr dev);
 
+virNodeDeviceObjListPtr
+virNodeDeviceObjListNew(void);
+
 void
 virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs);
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e6ed981..c7140c8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -971,6 +971,7 @@ virNodeDeviceObjGetNames;
 virNodeDeviceObjGetParentHost;
 virNodeDeviceObjListExport;
 virNodeDeviceObjListFree;
+virNodeDeviceObjListNew;
 virNodeDeviceObjLock;
 virNodeDeviceObjNumOfDevices;
 virNodeDeviceObjRemove;
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 45eb3f2..1ad2557 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -182,7 +182,7 @@ nodeNumOfDevices(virConnectPtr conn,
 virCheckFlags(0, -1);
 
 nodeDeviceLock();
-ndevs = virNodeDeviceObjNumOfDevices(&driver->devs, conn, cap,
+ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap,
  virNodeNumOfDevicesCheckACL);
 nodeDeviceUnlock();
 
@@ -205,7 +205,7 @@ nodeListDevices(virConnectPtr conn,
 virCheckFlags(0, -1);
 
 nodeDeviceLock();
-nnames = virNodeDeviceObjGetNames(&driver->devs, conn,
+nnames = virNodeDeviceObjGetNames(driver->devs, conn,
   virNodeListDevicesCheckACL,
   cap, names, maxnames);
 nodeDeviceUnlock();
@@ -227,7 +227,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
 return -1;
 
 nodeDeviceLock();
-ret = virNodeDeviceObjListExport(conn, &driver->devs, devices,
+ret = virNodeDeviceObjListExport(conn, driver->devs, devices,
  virConnectListAllNodeDevicesCheckACL,
  flags);
 nodeDeviceUnlock();
@@ -241,7 +241,7 @@ nodeDeviceObjFindByName(const char *name)
 virNodeDeviceObjPtr obj;
 
 nodeDeviceLock();
-obj = virNodeDeviceObjFindByName(&driver->devs, name);
+obj = virNodeDeviceObjFindByName(driver->devs, name);
 nodeDeviceUnlock();
 
 if (!obj) {
@@ -289,7 +289,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
   unsigned int flags)
 {
 size_t i;
-virNodeDeviceObjListPtr devs = &driver->devs;
+virNodeDeviceObjListPtr devs = driver->devs;
 virNodeDevCapsDefPtr cap = NULL;
 virNodeDeviceObjPtr obj = NULL;
 virNodeDeviceDefPtr def;
@@ -587,7 +587,7 @@ nodeDeviceCreateXML(virConnectPtr conn,
 if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1)
 goto cleanup;
 
-if ((parent_host = virNodeDeviceObjGetParentHost(&driver->devs, def,
+if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def,
  CREATE_DEVICE)) < 0)
 goto cleanup;
 
@@ -639,7 +639,7 @@ nodeDeviceDestroy(virNodeDevicePtr device)
  * or par

[libvirt] [PATCH v4 04/14] nodedev: Use consistent names for driver variables

[John Ferlan]

A virNodeDeviceObjPtr is an @obj

A virNodeDeviceObjListPtr is a @devs

A virNodeDevicePtr is a @device

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c | 74 ++--
 src/node_device/node_device_hal.c| 39 +--
 src/node_device/node_device_udev.c   | 46 ++
 3 files changed, 76 insertions(+), 83 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 5bf202e..45eb3f2 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -260,7 +260,7 @@ nodeDeviceLookupByName(virConnectPtr conn,
 {
 virNodeDeviceObjPtr obj;
 virNodeDeviceDefPtr def;
-virNodeDevicePtr ret = NULL;
+virNodeDevicePtr device = NULL;
 
 if (!(obj = nodeDeviceObjFindByName(name)))
 return NULL;
@@ -269,16 +269,16 @@ nodeDeviceLookupByName(virConnectPtr conn,
 if (virNodeDeviceLookupByNameEnsureACL(conn, def) < 0)
 goto cleanup;
 
-if ((ret = virGetNodeDevice(conn, name))) {
-if (VIR_STRDUP(ret->parent, def->parent) < 0) {
-virObjectUnref(ret);
-ret = NULL;
+if ((device = virGetNodeDevice(conn, name))) {
+if (VIR_STRDUP(device->parent, def->parent) < 0) {
+virObjectUnref(device);
+device = NULL;
 }
 }
 
  cleanup:
 virNodeDeviceObjUnlock(obj);
-return ret;
+return device;
 }
 
 
@@ -293,7 +293,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 virNodeDevCapsDefPtr cap = NULL;
 virNodeDeviceObjPtr obj = NULL;
 virNodeDeviceDefPtr def;
-virNodeDevicePtr dev = NULL;
+virNodeDevicePtr device = NULL;
 
 virCheckFlags(0, NULL);
 
@@ -316,10 +316,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, 
def) < 0)
 goto error;
 
-if ((dev = virGetNodeDevice(conn, def->name))) {
-if (VIR_STRDUP(dev->parent, def->parent) < 0) {
-virObjectUnref(dev);
-dev = NULL;
+if ((device = virGetNodeDevice(conn, def->name))) {
+if (VIR_STRDUP(device->parent, def->parent) < 0) {
+virObjectUnref(device);
+device = NULL;
 }
 }
 virNodeDeviceObjUnlock(obj);
@@ -335,7 +335,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 
  out:
 nodeDeviceUnlock();
-return dev;
+return device;
 
  error:
 virNodeDeviceObjUnlock(obj);
@@ -344,7 +344,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 
 
 char *
-nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
+nodeDeviceGetXMLDesc(virNodeDevicePtr device,
  unsigned int flags)
 {
 virNodeDeviceObjPtr obj;
@@ -353,11 +353,11 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 
 virCheckFlags(0, NULL);
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return NULL;
 def = virNodeDeviceObjGetDef(obj);
 
-if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, def) < 0)
+if (virNodeDeviceGetXMLDescEnsureACL(device->conn, def) < 0)
 goto cleanup;
 
 if (nodeDeviceUpdateDriverName(def) < 0)
@@ -375,17 +375,17 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 
 
 char *
-nodeDeviceGetParent(virNodeDevicePtr dev)
+nodeDeviceGetParent(virNodeDevicePtr device)
 {
 virNodeDeviceObjPtr obj;
 virNodeDeviceDefPtr def;
 char *ret = NULL;
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return NULL;
 def = virNodeDeviceObjGetDef(obj);
 
-if (virNodeDeviceGetParentEnsureACL(dev->conn, def) < 0)
+if (virNodeDeviceGetParentEnsureACL(device->conn, def) < 0)
 goto cleanup;
 
 if (def->parent) {
@@ -403,7 +403,7 @@ nodeDeviceGetParent(virNodeDevicePtr dev)
 
 
 int
-nodeDeviceNumOfCaps(virNodeDevicePtr dev)
+nodeDeviceNumOfCaps(virNodeDevicePtr device)
 {
 virNodeDeviceObjPtr obj;
 virNodeDeviceDefPtr def;
@@ -411,11 +411,11 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev)
 int ncaps = 0;
 int ret = -1;
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return -1;
 def = virNodeDeviceObjGetDef(obj);
 
-if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, def) < 0)
+if (virNodeDeviceNumOfCapsEnsureACL(device->conn, def) < 0)
 goto cleanup;
 
 for (caps = def->caps; caps; caps = caps->next) {
@@ -442,7 +442,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev)
 
 
 int
-nodeDeviceListCaps(virNodeDevicePtr dev,
+nodeDeviceListCaps(virNodeDevicePtr device,
char **const names,

Re: [libvirt] [PATCH v3 10/12] nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs

[John Ferlan]

On 07/03/2017 09:12 AM, Erik Skultety wrote:
> On Sat, Jun 03, 2017 at 09:12:00AM -0400, John Ferlan wrote:
>> Alter the nodeDeviceLookupSCSIHostByWWN to use the new API in order
>> to find what it's looking for.
>>
> 
> Would you mind enhancing the commit message a bit? I assume the new API is
> virNodeDeviceGetSCSIHostCaps but a random reader would lack the context. It
> misses the main motivation to move nodeDeviceLookupSCSIHostByWWN out of the
> driver simply because of the NodeDeviceObj privatization. Also, this got me
> thinking, whether 9/10 and 10/10 is critical for this series, so I tried to
> rewrite it without it and somewhere down the road I decided that it was an
> ugly, pointless waste of time and I like it this way much more.
> 
> ACK if you enhance the commit message a bit.
> 
> Erik
> 

Sure I'll add some details...

My first hack at 9/12 wasn't well received either since I moved too
much, see:

https://www.redhat.com/archives/libvir-list/2017-May/msg00722.html

and followups to that.  So I took the move and call path of less
resistance. The 10/12 change just follows other Lookup functions. It'll
get even more interesting soon when you see patch 13.

John

>> Signed-off-by: John Ferlan 
>> ---
>>  src/conf/virnodedeviceobj.c  | 33 +
>>  src/conf/virnodedeviceobj.h  |  5 
>>  src/libvirt_private.syms |  1 +
>>  src/node_device/node_device_driver.c | 56 
>> +++-
>>  4 files changed, 55 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
>> index a9187be..fa61a2d 100644
>> --- a/src/conf/virnodedeviceobj.c
>> +++ b/src/conf/virnodedeviceobj.c
>> @@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr 
>> devs,
>>  }
>>
>>
>> +virNodeDeviceObjPtr
>> +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
>> +   const char *wwnn,
>> +   const char *wwpn)
>> +{
>> +size_t i;
>> +
>> +for (i = 0; i < devs->count; i++) {
>> +virNodeDeviceObjPtr obj = devs->objs[i];
>> +virNodeDevCapsDefPtr cap;
>> +
>> +virNodeDeviceObjLock(obj);
>> +cap = obj->def->caps;
>> +
>> +while (cap) {
>> +if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
>> +virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
>> +if (cap->data.scsi_host.flags &
>> +VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
>> +if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
>> +STREQ(cap->data.scsi_host.wwpn, wwpn))
>> +return obj;
>> +}
>> +}
>> +cap = cap->next;
>> +}
>> +virNodeDeviceObjUnlock(obj);
>> +}
>> +
>> +return NULL;
>> +}
>> +
>> +
>>  void
>>  virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
>>  {
>> diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
>> index 6194c6c..6ec5ee7 100644
>> --- a/src/conf/virnodedeviceobj.h
>> +++ b/src/conf/virnodedeviceobj.h
>> @@ -53,6 +53,11 @@ 
>> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>>  ATTRIBUTE_NONNULL(2);
>>
>>  virNodeDeviceObjPtr
>> +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
>> +   const char *wwnn,
>> +   const char *wwpn);
>> +
>> +virNodeDeviceObjPtr
>>  virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
>>virNodeDeviceDefPtr def);
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 33fc9fc..d415888 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -963,6 +963,7 @@ virNodeDeviceObjListAssignDef;
>>  virNodeDeviceObjListExport;
>>  virNodeDeviceObjListFindByName;
>>  virNodeDeviceObjListFindBySysfsPath;
>> +virNodeDeviceObjListFindSCSIHostByWWNs;
>>  virNodeDeviceObjListFree;
>>  virNodeDeviceObjListGetNames;
>>  virNodeDeviceObjListGetParentHost;
>> diff --git a/src/node_device/node_device_driver.c 
>> b/src/node_device/node_device_driver.c
>> index 348539f..4a5f168 100644
>> --- a/src/node_device/node_device_driver.c
>> +++ b/src/node_device/node_device_driver.c
>> @@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>>const char *wwpn,
>>unsigned int flags)
>>  {
>> -size_t i;
>> -virNodeDeviceObjListPtr devs = driver->devs;
>> -virNodeDevCapsDefPtr cap = NULL;
>>  virNodeDeviceObjPtr obj = NULL;
>>  virNodeDeviceDefPtr def;
>>  virNodeDevicePtr device = NULL;
>> @@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>>  virCheckFlags(0, NULL);
>>
>>  nodeDeviceLock();
>> +obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn);

Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove

[John Ferlan]

[...]

>>
>> Doh...  true - of course I have in mind the end goal which is making
>> this whole process awfully difficult. Going step by step hasn't really
>> helped as much as it should have /-(.  Adjustments to later patches will
>> be necessary to either Unref or EndAPI.
> 
> I'm sorry, I don't follow. What changes to later patches are we talking about
> exactly? I agree with the original approach you took because IMHO it is the 
> most
> transparent in terms of gradual changes - replacing frees with unrefs or 
> EndAPIs
> for that matter, and it also keeps things consistent.
> 

Pay no attention to the old man talking to himself ;-)

I've gone back to the original 1/12 and just added a
virNodeDeviceObjFree within the if (dev) prior to dev_create(udi). Since
it doesn't matter if the node device is unlocked it's a safe place to
put it.

[...]

>>
>> I'm not sure I follow... Like noted previously trying not to disturb the
>> logic here - just applying a different band-aid
>>
>> The purpose of the Remove AIUI is to allow the code to rediscover some
>> device because no one wanted to handle the messy situation of some
>> capability being removed or some property being modified. Instead it was
>> deemed a better option to just Remove the device and allow dev_create
>> reinstantiate.
> 
> It's been a long time since 2008 and there wasn't a exhausting description why
> it was a problem back in 2008 - locks maybe?. Commits d48717054c7 and 
> 6244c173b
> don't really relate to what I'm suggesting, do they? You wouldn't mess around
> with locks, so you wouldn't regress with deadlock. I'm still trying to figure
> out what I'm missing here, since you're holding all the necessary locks anyway
> so what could possibly be the benefit of calling remove on the device prior to
> creating-refreshing it next.
> 
> Erik
> 

I was trying to figure out why you were advocating removing the Find and
Remove and just call dev_create directly.

AIUI, the code is removing the device and allowing dev_create recreate
it because it was easier than deal with def->caps replacement. Sure
maybe that's easier today, but it's been way too long for me to recall
much about hal other than something to do with 2001: A Space Odyssey
("I'm sorry Dave, I'm afraid I can't do that")...

John

v4 will be posted soon

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

[libvirt] [PATCH 0/2] Fix a couple issues found w/ vHBA logic

[John Ferlan]

Patch 1 fixes something seen whilst working through patch 2. Long
description in patch 2 describes the problem.

John Ferlan (2):
  storage: Alter check for default managed setting
  conf: Fix vHBA checkParent logic for pool creation

 src/conf/node_device_conf.c| 50 --
 src/storage/storage_backend_scsi.c |  6 ++---
 2 files changed, 45 insertions(+), 11 deletions(-)

-- 
2.9.4

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

[libvirt] [PATCH 2/2] conf: Fix vHBA checkParent logic for pool creation

[John Ferlan]

https://bugzilla.redhat.com/show_bug.cgi?id=1458708

When originally designed in commit id '42a021c1', providing the 'parent'
attribute to the 
was checked to make sure that the "parent" of the wwnn/wwpn matched that
of the provided parent "just in case" someone created the node device first,
then defined the storage pool using that node device afterwards. The result
is to not perform the vport_create call when the scsi_host represented by
the wwnn/wwpn already exists since it would fail.

Eventually someone came up with the brilliant idea to provide wwnn/wwpn of
an HBA instead of a vHBA, e.g. . This is the same as creating a storage pool backed to
the HBA that just happens to also be vport (vHBA) capable. The logic to
bypass the vport_create call was the same as the vHBA code since the wwn's
already exist. Once that was determined to work, adding in the 'parent'
attribute caused a problem for the DeleteVport code, which was fixed by
commit id '2c8e30ee7e'.

The next test tried was providing a valid pair of wwns that would find
the scsi_host HBA, but providing the wrong name for the 'parent' attribute.
This caused a different failure because at DeleteVport time if a parent
is provided it was assumed valid especially since the CreateVport code
would check that by calling virVHBAPathExists.

So alter the checkParent code to first ensure that the provided parent_name
is a valid HBA/vHBA, then check if the found scsi_host is an HBA or a vHBA
and make the appropriate check similar to the check made during DestroyVport.
This restores some checks that were "lost" during various refactorings such
as commit id '79ab0935' which altered the return value logic and commit id
'9fdc8c426' which moved the parent host name validity check, but neglected
to add a similar check for this odd HBA configuration. As it turns out prior
to this patch, the checkParent code would fail for an HBA, but that was
masked by the changed return value checking logic.

Signed-off-by: John Ferlan 
---
 src/conf/node_device_conf.c | 50 +
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index e5947e6..d4075b5 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2268,6 +2268,7 @@ checkParent(virConnectPtr conn,
 const char *name,
 const char *parent_name)
 {
+unsigned int host_num;
 char *scsi_host_name = NULL;
 char *vhba_parent = NULL;
 bool retval = false;
@@ -2278,20 +2279,53 @@ checkParent(virConnectPtr conn,
 if (!conn)
 return true;
 
-if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("parent '%s' is not properly formatted"), name);
 goto cleanup;
+}
 
-if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+if (!virVHBAPathExists(NULL, host_num)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("parent '%s' is not a vHBA/HBA"), parent_name);
 goto cleanup;
+}
 
-if (STRNEQ(parent_name, vhba_parent)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("Parent attribute '%s' does not match parent '%s' "
- "determined for the '%s' wwnn/wwpn lookup."),
-   parent_name, vhba_parent, name);
+if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+goto cleanup;
+
+if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("host name '%s' is not properly formatted"), name);
 goto cleanup;
 }
 
+/* If scsi_host_name is vport capable, then it's an HBA, thus compare
+ * only against the parent_name; otherwise, as long as the scsi_host_name
+ * path exists, then the scsi_host_name is a vHBA in which case we need
+ * to compare against it's parent. */
+if (virVHBAIsVportCapable(NULL, host_num)) {
+if (STRNEQ(parent_name, scsi_host_name)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("parent HBA '%s' doesn't match the wwnn/wwpn "
+ "scsi_host '%s'"),
+   parent_name, scsi_host_name);
+goto cleanup;
+}
+} else {
+if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+goto cleanup;
+
+if (STRNEQ(parent_name, vhba_parent)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("parent vHBA '%s' doesn't match the wwnn/wwpn "
+ "scsi_host '%s' parent '%s'"),
+   parent_name, scsi_host_name, vhba_parent);
+goto cleanup;
+}
+}
+
+
 retval = true;
 
  cleanup:
@@ -2333,7 +2367,7 @@ virNodeDeviceCreateVport(virConnectP

[libvirt] [PATCH 1/2] storage: Alter check for default managed setting

[John Ferlan]

Only alter the managed setting if it wasn't provided. If someone provided
'no', then honor that rather than overwriting.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend_scsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index f7378d3..f3e62fb 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -227,12 +227,12 @@ createVport(virConnectPtr conn,
   fchost->wwnn, fchost->wwpn);
 
 
-/* Since we're creating the vHBA, then we need to manage removing it
+/* Since we're creating the vHBA, then we may need to manage removing it
  * as well. Since we need this setting to "live" through a libvirtd
  * restart, we need to save the persistent configuration. So if not
- * already defined as YES, then force the issue.
+ * already defined as YES or NO, then force the issue.
  */
-if (fchost->managed != VIR_TRISTATE_BOOL_YES) {
+if (fchost->managed == VIR_TRISTATE_BOOL_ABSENT) {
 fchost->managed = VIR_TRISTATE_BOOL_YES;
 if (configFile) {
 if (virStoragePoolSaveConfig(configFile, def) < 0)
-- 
2.9.4

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

Re: [libvirt] [PATCH] docs: add entry to download table listing the Rust language binding

[John Ferlan]

On 07/03/2017 11:58 AM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange 
> ---
>  docs/downloads.html.in | 16 
>  1 file changed, 16 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH] news: Update for 3.5.0 release

[John Ferlan]

On 07/03/2017 08:16 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/news.xml | 82 
> +++
>  1 file changed, 82 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] Entering freeze for libvirt-3.5.0

[Guido Günther]

On Mon, Jul 03, 2017 at 10:49:46AM +0200, Ján Tomko wrote:
> [cc: Guido]
> 
> On Sat, Jul 01, 2017 at 02:18:58PM +0400, Roman Bogorodskiy wrote:
> >  Andrea Bolognani wrote:
> > > virnetsockettest also fails pretty often for me, certainly
> > > more than your figure; even if that wasn't the case, 1/5
> > > failure rate is way too high for a CI job.
> > 
> > I played a little more with virnetsockettest to get real stats and
> > figured the following:
> > 
> > 1. On my desktop (i5) and laptop (i3), I didn't get any failures in 50
> > 'check' runs
> > 2. On a VM that I use to run test builds in Jenkins, out of 50 runs it
> > fails from 1 to 6 times; I did this test a couple of times and either I
> > was lucky or failure rate is higher when my Jenkins perform regular
> > builds.
> > 
> > Anyway, I'll try to find a way to debug what's going on with
> > virnetsockettest.
> > 
> 
> IIRC Debian disabled this test years ago.
> 
> Guido, have you ever discovered the cause?

No, sorry. I diabled it ages ago since it failed randomly and never got
around to have a look.
Cheers,
 -- Guido

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

[libvirt] [PATCH] docs: add entry to download table listing the Rust language binding

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange 
---
 docs/downloads.html.in | 16 
 1 file changed, 16 insertions(+)

diff --git a/docs/downloads.html.in b/docs/downloads.html.in
index 99da7a6..0306945 100644
--- a/docs/downloads.html.in
+++ b/docs/downloads.html.in
@@ -179,6 +179,22 @@
   
 
 
+  Rust
+  
+ftp://libvirt.org/libvirt/rust/";>ftp
+http://libvirt.org/sources/rust/";>http
+https://libvirt.org/sources/rust/";>https
+  
+  
+http://libvirt.org/git/?p=libvirt-rust.git;a=summary";>libvirt
+  
+  
+https://gitlab.com/libvirt/libvirt-rust";>gitlab
+https://github.com/libvirt/libvirt-rust";>github
+  
+  
+
+
   Integration modules
 
 
-- 
2.9.4

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

Re: [libvirt] [PATCHv2 1/2] lxc: add support for docker-json Memory and VCPU conversion

[Cedric Bosdonnat]

On Tue, 2017-06-27 at 16:07 -0400, John Ferlan wrote:
> 
> On 06/27/2017 01:02 PM, Venkat Datta N H wrote:
> > Docker Memory and VCPU configuration is converted to fit for LXC container 
> > XML configuration
> > ---
> >  po/POTFILES.in |   1 +
> >  src/Makefile.am|   1 +
> >  src/lxc/lxc_docker.c   | 116 
> >  src/lxc/lxc_docker.h   |  32 +
> >  src/lxc/lxc_driver.c   |  13 +-
> >  src/lxc/lxc_native.h   |   1 +
> >  tests/Makefile.am  |   8 +-
> >  .../lxcdocker2xmldata-simple.json  |  36 +
> >  .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml |  15 +++
> >  tests/lxcdocker2xmltest.c  | 149 
> > +
> >  10 files changed, 366 insertions(+), 6 deletions(-)
> >  create mode 100644 src/lxc/lxc_docker.c
> >  create mode 100644 src/lxc/lxc_docker.h
> >  create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json
> >  create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml
> >  create mode 100644 tests/lxcdocker2xmltest.c
> > 
> 
> Should the drvlxc.html.in page be modified too?
> 
> http://libvirt.org/drvlxc.html
> 
> To include something that indicates docker config files being read and
> what fields are taken/used.. IOW how things are mapped.
> 
> > diff --git a/po/POTFILES.in b/po/POTFILES.in
> > index 275df1f..421b32e 100644
> > --- a/po/POTFILES.in
> > +++ b/po/POTFILES.in
> > @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c
> >  src/lxc/lxc_fuse.c
> >  src/lxc/lxc_hostdev.c
> >  src/lxc/lxc_native.c
> > +src/lxc/lxc_docker.c
> >  src/lxc/lxc_process.c
> >  src/network/bridge_driver.c
> >  src/network/bridge_driver_linux.c
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index eae32dc..1341e5a 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES =
> > \
> >     lxc/lxc_process.c lxc/lxc_process.h \
> >     lxc/lxc_fuse.c lxc/lxc_fuse.h   \
> >     lxc/lxc_native.c lxc/lxc_native.h   \
> > +   lxc/lxc_docker.c lxc/lxc_docker.h \
> >     lxc/lxc_driver.c lxc/lxc_driver.h
> >  
> >  LXC_CONTROLLER_SOURCES =   \
> > diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c
> > new file mode 100644
> > index 000..dbb2a81
> > --- /dev/null
> > +++ b/src/lxc/lxc_docker.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * lxc_docker.c: LXC native docker configuration import
> > + *
> > + * Copyright (C) 2017 Venkat Datta N H
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library.  If not, see
> > + * .
> > + *
> > + * Author: Venkat Datta N H 
> > + */
> > +
> > +#include 
> > +
> > +#include "util/viralloc.h"
> > +#include "util/virfile.h"
> > +#include "util/virstring.h"
> > +#include "util/virconf.h"
> > +#include "util/virjson.h"
> > +#include "util/virutil.h"
> > +#include "virerror.h"
> > +#include "virlog.h"
> > +#include "conf/domain_conf.h"
> > +#include "lxc_docker.h"
> > +#include "secret_conf.h"
> > +
> > +#define VIR_FROM_THIS VIR_FROM_LXC
> > +
> > +VIR_LOG_INIT("lxc.lxc_docker");
> > +
> > +static int virLXCDockerParseCpu(virDomainDefPtr dom,
> > +virDomainXMLOptionPtr xmlopt,
> > +virJSONValuePtr prop)
> 
> Stylistically, these should be:
> 
> static int
> virLXCDockerParseCpu(virDomainDefPtr dom,
>  virDomainXMLOptionPtr xmlopt,
>  virJSONValuePtr prop)
> 
> 
> note the multiple lines for function and how the arguments are aligned.
> 
> Similar point in every definition here.
> 
> And yes, I know/see that lxc_driver.c doesn't follow that model - this
> is the newer style we like to see in general.
> 
> > +{
> > +int vcpus;
> > +
> > +if (virJSONValueObjectGetNumberInt(prop, "CpuShares", &vcpus)  != 0)
> 
> s/ !=/ 
> > +return -1;
> 
> Why CpuShares and not CpuCount?  What about CpusetCpus?

After some more digging around that, maxvcpus value isn't used in the lxc 
driver.
It needs to be set to get the XML parser code hap

Re: [libvirt] [PATCH v3 12/12] nodedev: Convert virNodeDeviceObj to use virObjectLockable

[Erik Skultety]

On Thu, Jun 29, 2017 at 12:11:47PM -0400, John Ferlan wrote:
>
>
> On 06/03/2017 09:12 AM, John Ferlan wrote:
> > Now that we have a bit more control, let's convert our object into
> > a lockable object and let that magic handle the create and lock/unlock.
> >
> > This also involves creating a virNodeDeviceEndAPI in order to handle
> > the object cleaup for API's that use the Add or Find API's in order
> > to get a locked/reffed object. The EndAPI will unlock and unref the
> > object returning NULL to indicate to the caller to not use the obj.
> >
> > For now this also involves a forward reference to the Unlock, but
> > that'll be removed shortly when the object is convert to use the
> > virObjectLockable shortly.
> >
> > Signed-off-by: John Ferlan 
> > ---
> >  src/conf/virnodedeviceobj.c  | 156 
> > ++-
> >  src/conf/virnodedeviceobj.h  |  11 +--
> >  src/libvirt_private.syms |   4 +-
> >  src/node_device/node_device_driver.c |  17 ++--
> >  src/node_device/node_device_hal.c|   6 +-
> >  src/node_device/node_device_udev.c   |  12 +--
> >  src/test/test_driver.c   |  40 -
> >  7 files changed, 122 insertions(+), 124 deletions(-)
> >
>
> [...]
>
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 67fe252..7b67523 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
>
> [...]
>
> Oh my... looks like I forgot to go back and revisit the following hunk
> ;-), but of course tripped across it while working through merging
> changes for patch 1.
>
> > @@ -5597,28 +5596,29 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
> >   * taken, so we have to dup the parent's name and drop the lock
> >   * before calling it.  We don't need the reference to the object
> >   * any more once we have the parent's name.  */
> > -virNodeDeviceObjUnlock(obj);
> > +virNodeDeviceObjEndAPI(&obj);
>
> This should be "virObjectUnlock(obj);"
>
> >
> >  /* We do this just for basic validation, but also avoid finding a
> >   * vport capable HBA if for some reason our vHBA doesn't exist */
> >  if (virNodeDeviceObjListGetParentHost(driver->devs, def,
> > -  EXISTING_DEVICE) < 0) {
> > -obj = NULL;
> > +  EXISTING_DEVICE) < 0)
>
> This would require calling virObjectLock(obj);
>
> >  goto cleanup;
> > -}
> >
> >  event = virNodeDeviceEventLifecycleNew(dev->name,
> > VIR_NODE_DEVICE_EVENT_DELETED,
> > 0);
> >
> > -virNodeDeviceObjLock(obj);
>
> and calling virObjectLock(obj) here prior to calling ObjListRemove and
> setting obj = NULL;
>
> > +/*
> > + *
> > + * REVIEW THIS
> > + *
> > + */
> >  virNodeDeviceObjListRemove(driver->devs, obj);
> > -virNodeDeviceObjFree(obj);
> > +virObjectUnref(obj);
> >  obj = NULL;
>
> Hope this makes sense... I'm guessing some of this series will need to
> be reposted in a v4 anyway - including the "next" logical step to alter
> the ObjList which is what you're after anyway, but was further down in
> my original stack of patches.

Well, these would be really hard to track, so I agree that posting a v4 would
make things easier, since 10 (11 technically) out of 12 patches have already
been ACK'd, so it wouldn't prolong the overall process more than just those 2
patches we discuss.

Erik

>
> John
> >
> >   cleanup:
> > -if (obj)
> > -virNodeDeviceObjUnlock(obj);
> > +virNodeDeviceObjEndAPI(&obj);
> >  testObjectEventQueue(driver, event);
> >  VIR_FREE(parent_name);
> >  VIR_FREE(wwnn);
> >
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove

[Erik Skultety]

On Fri, Jun 30, 2017 at 09:47:07AM -0400, John Ferlan wrote:
>
>
> On 06/30/2017 07:41 AM, Erik Skultety wrote:
> > On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 06/30/2017 04:40 AM, Erik Skultety wrote:
> >>> On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote:
> 
> 
>  On 06/29/2017 10:28 AM, Erik Skultety wrote:
> > On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 06/29/2017 08:12 AM, Erik Skultety wrote:
> >>> On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote:
>  Rather than passing the object to be removed by reference, pass by 
>  value
>  and then let the caller decide whether or not the object should be 
>  free'd.
>  This function should just handle the remove of the object from the 
>  list
>  for which it was placed during virNodeDeviceObjAssignDef.
> 
>  One caller in node_device_hal would fail to go through the 
>  dev_create path
>  since the @dev would have been NULL after returning from the Remove 
>  API.
> >>>
> >>> This is the main motivation for the patch I presume - in which case, 
> >>> I'm
> >>> wondering why do we actually have to remove the device from the list 
> >>> when
> >>> handling 'change'/'update' for hal instead of just replacing the 
> >>> ->def with a
> >>> new instance but it's perfectly fine to do that for udev...I don't 
> >>> see the
> >>> point in doing what we currently do for hal.
> >>>
> >>> [...]
> >>
> >> The main motivation is that in the previous review pass there was a
> >> "dislike" of passing the pointer to a pointer for something else I
> >> changed, see:
> >>
> >> https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html
> >>
> >> Also the initial pass at altering this function was questioned, see:
> >>
> >> https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html
> >>
> >
> > Well, that comment is true, but the commit message of this patch says 
> > that it
> > drops the requirement of passing by reference, thus leaving the 
> > responsibility
> > to free the obj to the caller. Now, the way I see it what we should aim 
> > at
> > achieving here is reference counted objects, so the vir*ObjFree in the 
> > caller
> > would become and *Unref. Therefore IMHO it's not the best approach to 
> > introduce
> > another boolean for HAL and leave the vir*ObjFree in the Remove 
> > routine, you
> > wouldn't be clearing the object for the caller anyway, so I don't think 
> > that is
> > the way to go.
> >
> 
>  I actually think the better course of action is to be more like the
>  other functions. I believe virNodeDeviceObjRemove should do the
>  virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the
>  alloc and list insertion and *Remove is the antecedent doing the list
> >>>
> >>> Well, eventually we'll hopefully end up with reference-counted objects so 
> >>> doing
> >>> it with having *ObjFree in ObjRemove or in the caller won't matter in the 
> >>> end.
> >>
> >> Keeping the Free/Unref as part of the callers responsibility would mean
> >> that once we ended up with common object code doing the remove, then
> >> each of the nodedev callers would need to remove their extra Free/Unref.
> >
> > I don't think so. Looking at other examples, what happens in most cases is 
> > that
> > caller already gets the ref'd object, calls Remove() which calls Unref(),
> > returns to the caller who then calls EndAPI() which in turn destroys the 
> > last
> > reference. Therefore in here, to actually get rid of the object all the 
> > callers
> > would need to do essentially the same thing - keep their Unref() in the 
> > function
> > block, solely because GetObj() returns an already locked and ref'd object.
> >
>
> Doh...  true - of course I have in mind the end goal which is making
> this whole process awfully difficult. Going step by step hasn't really
> helped as much as it should have /-(.  Adjustments to later patches will
> be necessary to either Unref or EndAPI.

I'm sorry, I don't follow. What changes to later patches are we talking about
exactly? I agree with the original approach you took because IMHO it is the most
transparent in terms of gradual changes - replacing frees with unrefs or EndAPIs
for that matter, and it also keeps things consistent.

>
> Without getting into specifics of each EndAPI model (domain, network,
> and secret) - I'm assuming you can agree that each does things a bit
> differently and gives a sense for the impetus behind this overall
> effort. I generally sit here looking at code keeping track on my fingers
> of the RefCnt and whether the lock is held ;-)
>
> >> Which like you point out below is essentially equal work as long as

Re: [libvirt] [PATCH] qemu: Take all PHBs into account while calculating memlock limits

[Andrea Bolognani]

On Fri, 2017-06-30 at 13:56 +0530, Shivaprasad G Bhat wrote:
> -/* TODO: Detect at runtime once we start using more than just
> - *   the default PCI Host Bridge */
> -nPCIHostBridges = 1;
> +for (i = 0; i < def->ncontrollers; i++) {
> +if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI 
> ||
> +def->controllers[i]->model != 
> VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
> +continue;
> +}
> +nPCIHostBridges++;
> +}

Just to be on the safe side, we should probably make sure the
pci-root controller is actually a PHB by looking at modelName
as well, like:

  for (i = 0; i < def->ncontrollers; i++) {
  virDomainControllerDefPtr cont = def->controllers[i];

  if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI ||
  cont->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
  cont->opts.pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
  continue;
  }

  nPCIHostBridges++;
  }

Boy, that model name sure is a mouthful[1].

I think we might have enough occurrences of this pattern to
warrant the creation of a virDomainControllerIsPCIHostBridge()
helper function, which you could then use in your patch.

That said, it might be smarter to do so in a follow-up cleanup
commit in order not to invalidate existing Reviewed-by tags.
Laine, what would be your preference?


[1] Except for fingers. Fingerful?
-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] Entering freeze for libvirt-3.5.0

[Roman Bogorodskiy]

  Ján Tomko wrote:

> [cc: Guido]
> 
> On Sat, Jul 01, 2017 at 02:18:58PM +0400, Roman Bogorodskiy wrote:
> >  Andrea Bolognani wrote:
> >> virnetsockettest also fails pretty often for me, certainly
> >> more than your figure; even if that wasn't the case, 1/5
> >> failure rate is way too high for a CI job.
> >
> >I played a little more with virnetsockettest to get real stats and
> >figured the following:
> >
> > 1. On my desktop (i5) and laptop (i3), I didn't get any failures in 50
> > 'check' runs
> > 2. On a VM that I use to run test builds in Jenkins, out of 50 runs it
> > fails from 1 to 6 times; I did this test a couple of times and either I
> > was lucky or failure rate is higher when my Jenkins perform regular
> > builds.
> >
> >Anyway, I'll try to find a way to debug what's going on with
> >virnetsockettest.
> >
> 
> IIRC Debian disabled this test years ago.
> 
> Guido, have you ever discovered the cause?
> 
> Jan

I made some experiments on the weekend, and here are my results:

On a box where test fails from time to time, it fails at this point:

virObjectUnref(csock);

for (i = 0; i < nlsock; i++) {
if (virNetSocketAccept(lsock[i], &ssock) != -1 && ssock) {
char c = 'a';
if (virNetSocketWrite(ssock, &c, 1) != -1 &&
virNetSocketRead(ssock, &c, 1) != -1) {
VIR_DEBUG("Unexpected client socket present"); <--- HERE
goto cleanup;
}   
}   
virObjectUnref(ssock);
ssock = NULL;
}  

On a box where this test never fails, it reaches this block, but:

 * virNetSocketWrite(ssock, &c, 1) != -1
 * virNetSocketRead(ssock, &c, 1) == -1

It's enough to make the test pass. On a failing box both Write() and Read()
return != -1 when the test fails. 

I'm not quite sure what this specific block is testing though. My guess
was that calling "virObjectUnref(csock);" will destroy the client socket
and Accept() will not work (this should also make the test pass I
guess).

Anyway, I tried to insert sleep(1) right after virObjectUnref(csock),
the one before the virNetSocketAccept() call,
and the test stopped failing. I've started it in a loop like:

 while test $? -eq 0; do ./virnetsockettest; done

and actually forgot to stop, so it's running since Saturday without
failures.

I've haven't had a chance yet to debug it further.

Roman Bogorodskiy


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 09/12] nodedev: Introduce virNodeDeviceGetSCSIHostCaps

[Erik Skultety]

On Sat, Jun 03, 2017 at 09:11:59AM -0400, John Ferlan wrote:
> We're about to move the call to nodeDeviceSysfsGetSCSIHostCaps from
> node_device_driver into virnodedeviceobj, so move the guts of the code
> from the driver specific node_device_linux_sysfs into its own API
> since virnodedeviceobj cannot callback into the driver.
>
> Nothing in the code deals with sysfs anyway, as that's hidden by the
> various virSCSIHost* and virVHBA* utility function calls.
>

ACK

Erik

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

Re: [libvirt] [PATCH v3 10/12] nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs

[Erik Skultety]

On Sat, Jun 03, 2017 at 09:12:00AM -0400, John Ferlan wrote:
> Alter the nodeDeviceLookupSCSIHostByWWN to use the new API in order
> to find what it's looking for.
>

Would you mind enhancing the commit message a bit? I assume the new API is
virNodeDeviceGetSCSIHostCaps but a random reader would lack the context. It
misses the main motivation to move nodeDeviceLookupSCSIHostByWWN out of the
driver simply because of the NodeDeviceObj privatization. Also, this got me
thinking, whether 9/10 and 10/10 is critical for this series, so I tried to
rewrite it without it and somewhere down the road I decided that it was an
ugly, pointless waste of time and I like it this way much more.

ACK if you enhance the commit message a bit.

Erik

> Signed-off-by: John Ferlan 
> ---
>  src/conf/virnodedeviceobj.c  | 33 +
>  src/conf/virnodedeviceobj.h  |  5 
>  src/libvirt_private.syms |  1 +
>  src/node_device/node_device_driver.c | 56 
> +++-
>  4 files changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index a9187be..fa61a2d 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr 
> devs,
>  }
>
>
> +virNodeDeviceObjPtr
> +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
> +   const char *wwnn,
> +   const char *wwpn)
> +{
> +size_t i;
> +
> +for (i = 0; i < devs->count; i++) {
> +virNodeDeviceObjPtr obj = devs->objs[i];
> +virNodeDevCapsDefPtr cap;
> +
> +virNodeDeviceObjLock(obj);
> +cap = obj->def->caps;
> +
> +while (cap) {
> +if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
> +virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
> +if (cap->data.scsi_host.flags &
> +VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> +if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
> +STREQ(cap->data.scsi_host.wwpn, wwpn))
> +return obj;
> +}
> +}
> +cap = cap->next;
> +}
> +virNodeDeviceObjUnlock(obj);
> +}
> +
> +return NULL;
> +}
> +
> +
>  void
>  virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
>  {
> diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
> index 6194c6c..6ec5ee7 100644
> --- a/src/conf/virnodedeviceobj.h
> +++ b/src/conf/virnodedeviceobj.h
> @@ -53,6 +53,11 @@ 
> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>  ATTRIBUTE_NONNULL(2);
>
>  virNodeDeviceObjPtr
> +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
> +   const char *wwnn,
> +   const char *wwpn);
> +
> +virNodeDeviceObjPtr
>  virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
>virNodeDeviceDefPtr def);
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 33fc9fc..d415888 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -963,6 +963,7 @@ virNodeDeviceObjListAssignDef;
>  virNodeDeviceObjListExport;
>  virNodeDeviceObjListFindByName;
>  virNodeDeviceObjListFindBySysfsPath;
> +virNodeDeviceObjListFindSCSIHostByWWNs;
>  virNodeDeviceObjListFree;
>  virNodeDeviceObjListGetNames;
>  virNodeDeviceObjListGetParentHost;
> diff --git a/src/node_device/node_device_driver.c 
> b/src/node_device/node_device_driver.c
> index 348539f..4a5f168 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>const char *wwpn,
>unsigned int flags)
>  {
> -size_t i;
> -virNodeDeviceObjListPtr devs = driver->devs;
> -virNodeDevCapsDefPtr cap = NULL;
>  virNodeDeviceObjPtr obj = NULL;
>  virNodeDeviceDefPtr def;
>  virNodeDevicePtr device = NULL;
> @@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>  virCheckFlags(0, NULL);
>
>  nodeDeviceLock();
> +obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn);
> +nodeDeviceUnlock();
>
> -for (i = 0; i < devs->count; i++) {
> -obj = devs->objs[i];
> -virNodeDeviceObjLock(obj);
> -def = virNodeDeviceObjGetDef(obj);
> -cap = def->caps;
> -
> -while (cap) {
> -if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
> -nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host);
> -if (cap->data.scsi_host.flags &
> -VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> -if (STREQ(cap->data.scsi_host.wwnn, wwnn

[libvirt] [PATCH] news: Update for 3.5.0 release

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 docs/news.xml | 82 +++
 1 file changed, 82 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 636154e..f38abeb 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -68,6 +68,15 @@
   running.
 
   
+  
+
+  qemu: Allow VirtIO devices to use vIOMMU
+
+
+  It is now possible to turn on IOTBL for the vIOMMU and have VirtIO
+  devices use it, provided they have been configured appropriately.
+
+  
 
 
   
@@ -100,6 +109,20 @@
   gathered and presented in capabilities if available.
 
   
+  
+
+  apparmor: Several improvements
+
+
+  Allow access to Ceph config, EFI firmware on both x86_64 and
+  aarch64, device tree on ppc64 and more.
+
+  
+  
+
+  qemu: Support host-model on POWER9 machines
+
+  
 
 
   
@@ -115,6 +138,65 @@
   images when doing a block-commit.
 
   
+  
+
+  Parse decimal numbers in a locale-independent way
+
+
+  Some locales, such as de_DE and pt_BR,
+  use comma rather than dot to separate the integer part from the
+  fractional part of a decimal number; however, several data sources
+  such as the kernel use a locale-independent representation and need
+  to be treated accordingly.
+
+  
+  
+
+  Support compilation with newer compiler and libc versions
+
+
+  Several fixes have been included to make compilation with Clang
+  4.0.0, GCC 7.1 and glibc >= 2.25.90 possible.
+
+  
+  
+
+  qemu: Query name for vhost-user interfaces at runtime
+
+
+  This makes it possible to use virsh subcommands such
+  as domiflist and domifstat on vhost-user
+  interfaces.
+
+  
+  
+
+  qemu: Set MTU for hotplugged interfaces correctly
+
+
+  When hotplugging a network interface, the MTU was only set on the
+  guest side. Set it on the host side as well.
+
+  
+  
+
+  qemu: Forbid updating MTU for interfaces of running guests
+
+
+  The MTU setting can't be modified while the guest is running, so any
+  attempt to alter it at runtime will now result in an error rather
+  than being silently ignored.
+
+  
+  
+
+  qemu: Fix specifying QXL heads with older QEMU releases
+
+
+  Specifying the number of QXL heads was not working correctly for
+  QEMU releases older than 1.6.
+
+  
 
   
   
-- 
2.7.5

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

Re: [libvirt] [RFC PATCH v2 1/4] numa: describe siblings distances within cells

[Daniel P. Berrange]

On Mon, Jul 03, 2017 at 11:19:07AM +0200, Wim ten Have wrote:
> On Wed, 28 Jun 2017 15:21:29 +0100
> "Daniel P. Berrange"  wrote:
> 
> Hi Daniel,
> 
>   Appreciate your input and comments. Let me work on package and return
>   asap.
> 
>   One quick question; For future i am looking into way to somehow auto-
>   tune node (cell/socket/cpu) assignment under  directive. Such
>   in a way brings intelligence into libvirt and therefor not wanted.

On the  elemnet we have a placement=auto attribute, which tells
libvirt to pick host pCPU placement for vCPUs automatically. In the
QEMU driver, this asks 'numad' for best placement.   This is only
done during initial guest startup - there's no active monitoring or
adjustment thereafter.

Anything more advanced than this, is out of scope for libvirt and
best served by an external app above.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC PATCH v2 1/4] numa: describe siblings distances within cells

[Wim ten Have]

On Wed, 28 Jun 2017 15:21:29 +0100
"Daniel P. Berrange"  wrote:

Hi Daniel,

  Appreciate your input and comments. Let me work on package and return
  asap.

  One quick question; For future i am looking into way to somehow auto-
  tune node (cell/socket/cpu) assignment under  directive. Such
  in a way brings intelligence into libvirt and therefor not wanted.

  Perhaps this is up to s/w stack utilizing guests per libvirt. If
  there are thoughts on such matter then i am very interested to hear
  about them.

> On Wed, Jun 28, 2017 at 03:56:36PM +0200, Wim Ten Have wrote:
> > From: Wim ten Have 
> > 
> > Add libvirtd NUMA cell domain administration functionality to
> > describe underlying cell id sibling distances in full fashion
> > when configuring HVM guests.
> > 
> > [below is an example of a 4 node setup]
> > 
> >   
> > 
> >   
> > 
> >   
> >   
> >   
> >   
> > 
> >   
> >   
> > 
> >   
> >   
> >   
> >   
> > 
> >   
> >   
> > 
> >   
> >   
> >   
> >   
> > 
> >   
> > 
> >   
> >   
> >   
> >   
> > 
> >   
> > 
> >   
> > 
> > Changes under this commit concern all those that require adding
> > the valid data structures, virDomainNuma* functional routines and the
> > XML doc schema enhancements to enforce appropriate administration.
> > 
> > These changes are accompanied with topic related documentation under
> > docs/formatdomain.html within the "CPU model and topology" extending the
> > "Guest NUMA topology" paragraph.
> > 
> > For terminology we refer to sockets as "nodes" where access to each
> > others' distinct resources such as memory make them "siblings" with a
> > designated "distance" between them.  A specific design is described under
> > the ACPI (Advanced Configuration and Power Interface Specification)
> > within the chapter explaining the system's SLIT (System Locality Distance
> > Information Table).
> > 
> > These patches extend core libvirt's XML description of a virtual machine's
> > hardware to include NUMA distance information for sibling nodes, which
> > is then passed to Xen guests via libxl. Recently qemu landed support for
> > constructing the SLIT since commit 0f203430dd ("numa: Allow setting NUMA
> > distance for different NUMA nodes"), hence these core libvirt extensions
> > can also help other drivers in supporting this feature.
> > 
> > These changes alter the docs/schemas/cputypes.rng enforcing
> > domain administration to follow the syntax below per numa cell id.
> > 
> > These changes also alter docs/schemas/basictypes.rng to add
> > "numaDistanceValue" which is an "unsignedInt" with a minimum value
> > of 10 as 0-9 are reserved values and can not be used as System
> > Locality Distance Information Table data.
> > 
> > Signed-off-by: Wim ten Have 
> > ---
> >  docs/formatdomain.html.in   |  64 ++-
> >  docs/schemas/basictypes.rng |   8 ++
> >  docs/schemas/cputypes.rng   |  18 +++
> >  src/conf/cpu_conf.c |   2 +-
> >  src/conf/numa_conf.c| 260 
> > +++-
> >  src/conf/numa_conf.h|  25 -
> >  src/libvirt_private.syms|   6 +
> >  7 files changed, 376 insertions(+), 7 deletions(-)
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 36bea67..b9736e3 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1510,7 +1510,69 @@
> >  
> >  
> >  
> > -  This guest NUMA specification is currently available only for 
> > QEMU/KVM.
> > +  This guest NUMA specification is currently available only for
> > +  QEMU/KVM and Xen.  Whereas Xen driver also allows for a distinct
> > +  description of NUMA arranged sibling cell
> > +  distances Since 3.6.0.
> > +
> > +
> > +
> > +  Under NUMA h/w architecture, distinct resources such as memory
> > +  create a designated distance between cell and
> > +  siblings that now can be described with the help of
> > +  distances. A detailed describtion can be found within
> > +  the ACPI (Advanced Configuration and Power Interface Specification)
> > +  within the chapter explaining the system's SLIT (System Locality
> > +  Distance Information Table).
> > +
> > +
> > +
> > +...
> > +
> > +  ...
> > +  
> > +
> > +  
> > +
> > +
> > +
> > +
> > +  
> > +
> > + > memAccess='shared'>
> > +  
> > +
> > +
>

Re: [libvirt] Entering freeze for libvirt-3.5.0

[Ján Tomko]

[cc: Guido]

On Sat, Jul 01, 2017 at 02:18:58PM +0400, Roman Bogorodskiy wrote:

 Andrea Bolognani wrote:

virnetsockettest also fails pretty often for me, certainly
more than your figure; even if that wasn't the case, 1/5
failure rate is way too high for a CI job.


I played a little more with virnetsockettest to get real stats and
figured the following:

1. On my desktop (i5) and laptop (i3), I didn't get any failures in 50
'check' runs
2. On a VM that I use to run test builds in Jenkins, out of 50 runs it
fails from 1 to 6 times; I did this test a couple of times and either I
was lucky or failure rate is higher when my Jenkins perform regular
builds.

Anyway, I'll try to find a way to debug what's going on with
virnetsockettest.



IIRC Debian disabled this test years ago.

Guido, have you ever discovered the cause?

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH RESEND] qemu: Remove inactive vm when failed to start it

[wang.yi59]

Hi all,

Could someone review my patch please?




> Libvirt forgets to remove inactive vm when failed to start a defined vm.

> That may result in residual domain in driver->domains on such condition:

> during the process of starting a vm, undefine it, and qemu exit because

> of some exception. As we undefined the vm successfully, the vm->persistent

> was set to 0, we will always fail to undefine it until restart libvirtd.

> 

> Signed-off-by: Yi Wang 

> ---

>  src/qemu/qemu_driver.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c

> index cdb727b..af8afab 100644

> --- a/src/qemu/qemu_driver.c

> +++ b/src/qemu/qemu_driver.c

> @@ -7185,6 +7185,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned 
> int flags)

>  

>   endjob:

>  qemuProcessEndJob(driver, vm)

> +if (ret < 0)

> +qemuDomainRemoveInactive(driver, vm)

>  

>   cleanup:

>  virDomainObjEndAPI(&vm)

> -- 

> 1.8.3.1








---

Best wishes

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

Re: [libvirt] [PATCH v3 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs

[Erik Skultety]

On Fri, Jun 30, 2017 at 01:08:22PM -0400, John Ferlan wrote:
>
>
> On 06/30/2017 08:06 AM, Erik Skultety wrote:
> > On Sat, Jun 03, 2017 at 09:11:58AM -0400, John Ferlan wrote:
> >> Create local @obj and @def for the API's rather than referencing the
> >> devs->objs[i][->def->].  It'll make future patches easier to read.
>
> ^ [1]
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/conf/virnodedeviceobj.c | 60 
> >> -
> >>  1 file changed, 37 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> >> index fab1cfd..a9187be 100644
> >> --- a/src/conf/virnodedeviceobj.c
> >> +++ b/src/conf/virnodedeviceobj.c
> >> @@ -172,12 +172,16 @@ 
> >> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
> >>  size_t i;
> >>
> >>  for (i = 0; i < devs->count; i++) {
> >> -virNodeDeviceObjLock(devs->objs[i]);
> >> -if ((devs->objs[i]->def->sysfs_path != NULL) &&
> >> -(STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
> >> -return devs->objs[i];
> >> +virNodeDeviceObjPtr obj = devs->objs[i];
> >> +virNodeDeviceDefPtr def;
> >
> > Although with good intension, I think having virNodeDeviceObjPtr obj = foo 
> > only
> > will make all the difference, thus dereferencing @def is IMHO not that much
> > more beneficial.
>
> [1] I'm trying to avoid syntax such as:
>
>devs->objs[i]->def->sysfs_path
>
> or
>
>objs->def->sysfs_path
>
> If some day ->def is "owned-by" some vir*Object, then -> def would need
> to be fetched, e.g. def = virObject*GetDef(obj), so this just makes that
> future easier.

Yeah, I'm realizing it was a pointless suggestion, strange I didn't even think
about that when I was looking at 10/12, you can disregard my previous comment
then...^good point btw.

ACK as is.

Erik

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