[libvirt] [PATCH] news: Fix typo
Signed-off-by: Andrea Bolognani --- Pushed as trivial. docs/news.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/news.xml b/docs/news.xml index caa0363..b25a42f 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -130,7 +130,7 @@ - qemu: Misceleanous domain NS fixes + qemu: Miscellaneous domain NS fixes Libvirt starts qemu domains in separate Linux namespaces for a while -- 2.7.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC]Add new mdev interface for QoS
On 2017/7/28 0:00, Gao, Ping A wrote: > On 2017/7/27 0:43, Alex Williamson wrote: >> [cc +libvir-list] >> >> On Wed, 26 Jul 2017 21:16:59 +0800 >> "Gao, Ping A" wrote: >> >>> The vfio-mdev provide the capability to let different guest share the >>> same physical device through mediate sharing, as result it bring a >>> requirement about how to control the device sharing, we need a QoS >>> related interface for mdev to management virtual device resource. >>> >>> E.g. In practical use, vGPUs assigned to different quests almost has >>> different performance requirements, some guests may need higher priority >>> for real time usage, some other may need more portion of the GPU >>> resource to get higher 3D performance, corresponding we can define some >>> interfaces like weight/cap for overall budget control, priority for >>> single submission control. >>> >>> So I suggest to add some common attributes which are vendor agnostic in >>> mdev core sysfs for QoS purpose. >> I think what you're asking for is just some standardization of a QoS >> attribute_group which a vendor can optionally include within the >> existing mdev_parent_ops.mdev_attr_groups. The mdev core will >> transparently enable this, but it really only provides the standard, >> all of the support code is left for the vendor. I'm fine with that, >> but of course the trouble with and sort of standardization is arriving >> at an agreed upon standard. Are there QoS knobs that are generic >> across any mdev device type? Are there others that are more specific >> to vGPU? Are there existing examples of this that we can steal their >> specification? > Yes, you are right, standardization QoS knobs are exactly what I wanted. > Only when it become a part of the mdev framework and libvirt, then QoS > such critical feature can be leveraged by cloud usage. HW vendor only > need to focus on the implementation of the corresponding QoS algorithm > in their back-end driver. > > Vfio-mdev framework provide the capability to share the device that lack > of HW virtualization support to guests, no matter the device type, > mediated sharing actually is a time sharing multiplex method, from this > point of view, QoS can be take as a generic way about how to control the > time assignment for virtual mdev device that occupy HW. As result we can > define QoS knob generic across any device type by this way. Even if HW > has build in with some kind of QoS support, I think it's not a problem > for back-end driver to convert mdev standard QoS definition to their > specification to reach the same performance expectation. Seems there are > no examples for us to follow, we need define it from scratch. > > I proposal universal QoS control interfaces like below: > > Cap: The cap limits the maximum percentage of time a mdev device can own > physical device. e.g. cap=60, means mdev device cannot take over 60% of > total physical resource. > > Weight: The weight define proportional control of the mdev device > resource between guests, it’s orthogonal with Cap, to target load > balancing. E.g. if guest 1 should take double mdev device resource > compare with guest 2, need set weight ratio to 2:1. > > Priority: The guest who has higher priority will get execution first, > target to some real time usage and speeding interactive response. > > Above QoS interfaces cover both overall budget control and single > submission control. I will sent out detail design later once get aligned. Hi Alex, Any comments about the interface mentioned above? >> Also, mdev devices are not necessarily the exclusive users of the >> hardware, we can have a native user such as a local X client. They're >> not an mdev user, so we can't support them via the mdev_attr_group. >> Does there need to be a per mdev parent QoS attribute_group standard >> for somehow defining the QoS of all the child mdev devices, or perhaps >> representing the remaining host QoS attributes? > That's really an open, if we don't take host workload into consideration > for cloud usage, it's not a problem any more, however such assumption is > not reasonable. Any way if we take mdev devices as clients of host > driver, and host driver provide the capability to divide out a portion > HW resource to mdev devices, then it's only need to take care about the > resource that host assigned for mdev devices. Follow this way QoS for > mdev focus on the relationship between mdev devices no need to take care > the host workload. > > -Ping > >> Ultimately libvirt and upper level management tools would be the >> consumer of these control knobs, so let's immediately get libvirt >> involved in the discussion. Thanks, >> >> Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 7/8] util: Add magic number check for object validity
The virObjectIsClass API has only ever checked object validity
based on if the @obj is not NULL and it was derived from some class.
While this has worked well in general, there is one additional
check that could be made prior to calling virClassIsDerivedFrom
which loops through the classes checking the magic number against
the klass expected magic number.
If by chance a non virObject is passed, rather than assuming the
void * @obj is a _virObject and thus offsetting to obj->klass,
obj->magic, and obj->parent, let's check that the void * @obj
has at least the "base part" of the magic number in the right
place and generate a more specific VIR_WARN message if not.
There are many consumers to virObjectIsClass, include the locking
primitives virObject{Lock|Unlock}, virObjectRWLock{Read|Write},
and virObjectRWUnlock. For those callers, the locking call will
not fail, but it also will not attempt a virMutex* call which
will "most likely" fail since the &obj->lock is used.
In order to avoid some possible future wrap on the 0xCAFE
value, add a check during initialization that some new class
won't cause the wrap. Should be good for a few years at least!
It is still left up to the caller to handle the failed API calls
just as it would be if it passed a NULL opaque pointer anyobj.
Signed-off-by: John Ferlan
---
src/util/virobject.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index af3f252..54d78b0 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -47,14 +47,21 @@ struct _virClass {
virObjectDisposeCallback dispose;
};
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0x) !=
0xCAFE))
+
#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass)\
do {\
virObjectPtr obj = anyobj; \
-if (!obj) \
-VIR_WARN("Object cannot be NULL"); \
-else\
+if (VIR_OBJECT_NOTVALID(obj)) { \
+if (!obj) \
+VIR_WARN("Object cannot be NULL"); \
+else\
+VIR_WARN("Object %p has a bad magic number %X", \
+ obj, obj->u.s.magic); \
+} else {\
VIR_WARN("Object %p (%s) is not a %s instance", \
anyobj, obj->klass->name, #objclass); \
+} \
} while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
goto error;
klass->parent = parent;
+klass->magic = virAtomicIntInc(&magicCounter);
+if (klass->magic > 0xCAFE) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many object classes defined"));
+goto error;
+}
if (VIR_STRDUP(klass->name, name) < 0)
goto error;
-klass->magic = virAtomicIntInc(&magicCounter);
klass->objectSize = objectSize;
klass->dispose = dispose;
@@ -535,7 +547,7 @@ virObjectIsClass(void *anyobj,
virClassPtr klass)
{
virObjectPtr obj = anyobj;
-if (!obj)
+if (VIR_OBJECT_NOTVALID(obj))
return false;
return virClassIsDerivedFrom(obj->klass, klass);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 8/8] util: Add object checking for virObject{Ref|Unref}
Rather than assuming that what's passed to virObject{Ref|Unref}
would be a virObjectPtr as long as it's not NULL, let's do the
similar checks virObjectIsClass in order to prevent a possible
increment or decrement to some field at the obj->u.s.refs offset.
Signed-off-by: John Ferlan
---
src/util/virobject.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 54d78b0..cfa821c 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -343,7 +343,7 @@ virObjectUnref(void *anyobj)
{
virObjectPtr obj = anyobj;
-if (!obj)
+if (VIR_OBJECT_NOTVALID(obj))
return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
@@ -382,7 +382,7 @@ virObjectRef(void *anyobj)
{
virObjectPtr obj = anyobj;
-if (!obj)
+if (VIR_OBJECT_NOTVALID(obj))
return NULL;
virAtomicIntInc(&obj->u.s.refs);
PROBE(OBJECT_REF, "obj=%p", obj);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/8] util: Introduce and use virObjectRWLockWrite
Instead of making virObjectLock be the entry point for two
different types of locks, let's create a virObjectRWLockWrite API
which will only handle the virObjectRWLockableClass objects.
Use the new virObjectRWLockWrite for the virdomainobjlist code
in order to handle the Add, Remove, Rename, and Load operations
that need to be very synchronous.
Signed-off-by: John Ferlan
---
src/conf/virdomainobjlist.c | 8
src/libvirt_private.syms| 1 +
src/util/virobject.c| 33 -
src/util/virobject.h| 4
4 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 9bc6603..573032f 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -327,7 +327,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr
doms,
{
virDomainObjPtr ret;
-virObjectLock(doms);
+virObjectRWLockWrite(doms);
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
virObjectUnlock(doms);
return ret;
@@ -349,7 +349,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virObjectRef(dom);
virObjectUnlock(dom);
-virObjectLock(doms);
+virObjectRWLockWrite(doms);
virObjectLock(dom);
virHashRemoveEntry(doms->objs, uuidstr);
virHashRemoveEntry(doms->objsName, dom->def->name);
@@ -394,7 +394,7 @@ virDomainObjListRename(virDomainObjListPtr doms,
* hold a lock on dom but not refcount it. */
virObjectRef(dom);
virObjectUnlock(dom);
-virObjectLock(doms);
+virObjectRWLockWrite(doms);
virObjectLock(dom);
virObjectUnref(dom);
@@ -573,7 +573,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
return rc;
-virObjectLock(doms);
+virObjectRWLockWrite(doms);
while ((ret = virDirRead(dir, &entry, configDir)) > 0) {
virDomainObjPtr dom;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 99302d2..7f2f6d6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2309,6 +2309,7 @@ virObjectNew;
virObjectRef;
virObjectRWLockableNew;
virObjectRWLockRead;
+virObjectRWLockWrite;
virObjectUnlock;
virObjectUnref;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index b97f251..f49af62 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -430,11 +430,42 @@ virObjectRWLockRead(void *anyobj)
/**
+ * virObjectRWLockWrite:
+ * @anyobj: any instance of virObjectRWLockable
+ *
+ * Acquire a write lock on @anyobj. The lock must be
+ * released by virObjectUnlock.
+ *
+ * The caller is expected to have acquired a reference
+ * on the object before locking it (eg virObjectRef).
+ * The object must be unlocked before releasing this
+ * reference.
+ *
+ * NB: It's possible to return without the lock if
+ * @anyobj was invalid - this has been considered
+ * a programming error rather than something that
+ * should be checked.
+ */
+void
+virObjectRWLockWrite(void *anyobj)
+{
+if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+virObjectRWLockablePtr obj = anyobj;
+virRWLockWrite(&obj->lock);
+} else {
+virObjectPtr obj = anyobj;
+VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+}
+}
+
+
+/**
* virObjectUnlock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
* Release a lock on @anyobj. The lock must have been acquired by
- * virObjectLock or virObjectRWLockRead.
+ * virObjectLock, virObjectRWLockRead, or virObjectRWLockWrite.
*/
void
virObjectUnlock(void *anyobj)
diff --git a/src/util/virobject.h b/src/util/virobject.h
index e7ca983..24ee6dd 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -129,6 +129,10 @@ virObjectRWLockRead(void *lockableobj)
ATTRIBUTE_NONNULL(1);
void
+virObjectRWLockWrite(void *lockableobj)
+ATTRIBUTE_NONNULL(1);
+
+void
virObjectUnlock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/8] util: Only have virObjectLock handle virObjectLockable
Now that virObjectRWLockWrite exists to handle the virObjectRWLockable
objects, let's restore virObjectLock to only handle virObjectLockable
class locks. There still exists the possibility that the input @anyobj
isn't a valid object and the resource isn't truly locked, but that
also exists before commit id '77f4593b'.
This also restores some logic that commit id '77f4593b' removed
with respect to a common code path that commit id '10c2bb2b' had
introduced as virObjectGetLockableObj. This code path merely does
the same checks as the original virObjectLock commit 'b545f65d',
but in callable/reusable helper to ensure the @obj at least has
some validity before using.
Signed-off-by: John Ferlan
---
src/util/virobject.c | 37 +++--
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index f49af62..4903393 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -367,13 +367,28 @@ virObjectRef(void *anyobj)
}
+static virObjectLockablePtr
+virObjectGetLockableObj(void *anyobj)
+{
+virObjectPtr obj;
+
+if (virObjectIsClass(anyobj, virObjectLockableClass))
+return anyobj;
+
+obj = anyobj;
+VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+
+return NULL;
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
* Acquire a lock on @anyobj. The lock must be released by
- * virObjectUnlock. In case the passed object is instance of
- * virObjectRWLockable a write lock is acquired.
+ * virObjectUnlock.
*
* The caller is expected to have acquired a reference
* on the object before locking it (eg virObjectRef).
@@ -383,18 +398,12 @@ virObjectRef(void *anyobj)
void
virObjectLock(void *anyobj)
{
-if (virObjectIsClass(anyobj, virObjectLockableClass)) {
-virObjectLockablePtr obj = anyobj;
-virMutexLock(&obj->lock);
-} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
-virObjectRWLockablePtr obj = anyobj;
-virRWLockWrite(&obj->lock);
-} else {
-virObjectPtr obj = anyobj;
-VIR_WARN("Object %p (%s) is not a virObjectLockable "
- "nor virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-}
+virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+if (!obj)
+return;
+
+virMutexLock(&obj->lock);
}
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/8] util: Introduce and use virObjectRWUnlock
Rather than overload virObjectUnlock as commit id '77f4593b' has
done, create a separate virObjectRWUnlock API that will force the
consumers to make the proper decision regarding unlocking the
RWLock's. Similar to the RWLockRead and RWLockWrite, use the
virObjectGetRWLockableObj helper. This restores the virObjectUnlock
code to using the virObjectGetLockableObj.
Signed-off-by: John Ferlan
---
src/conf/virdomainobjlist.c | 36 ++--
src/libvirt_private.syms| 1 +
src/util/virobject.c| 45 +
src/util/virobject.h| 4
4 files changed, 52 insertions(+), 34 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 573032f..d874133 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -122,7 +122,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
if (ref) {
virObjectRef(obj);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
}
if (obj) {
virObjectLock(obj);
@@ -134,7 +134,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
}
}
if (!ref)
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return obj;
}
@@ -166,7 +166,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
obj = virHashLookup(doms->objs, uuidstr);
if (ref) {
virObjectRef(obj);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
}
if (obj) {
virObjectLock(obj);
@@ -178,7 +178,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
}
}
if (!ref)
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return obj;
}
@@ -207,7 +207,7 @@ virDomainObjPtr
virDomainObjListFindByName(virDomainObjListPtr doms,
virObjectRWLockRead(doms);
obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -329,7 +329,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr
doms,
virObjectRWLockWrite(doms);
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return ret;
}
@@ -355,7 +355,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virHashRemoveEntry(doms->objsName, dom->def->name);
virObjectUnlock(dom);
virObjectUnref(dom);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
}
@@ -420,7 +420,7 @@ virDomainObjListRename(virDomainObjListPtr doms,
ret = 0;
cleanup:
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
VIR_FREE(old_name);
return ret;
}
@@ -609,7 +609,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
}
VIR_DIR_CLOSE(dir);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return ret;
}
@@ -655,7 +655,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
struct virDomainObjListData data = { filter, conn, active, 0 };
virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListCount, &data);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return data.count;
}
@@ -699,7 +699,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
0, maxids, ids };
virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return data.numids;
}
@@ -753,7 +753,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
size_t i;
virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
if (data.oom) {
for (i = 0; i < data.numnames; i++)
VIR_FREE(data.names[i]);
@@ -794,7 +794,7 @@ virDomainObjListForEach(virDomainObjListPtr doms,
};
virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListHelper, &data);
-virObjectUnlock(doms);
+virObjectRWUnlock(doms);
return data.ret;
}
@@ -928,12 +928,12 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
virObjectRWLockRead(domlist);
sa_assert(domlist->objs);
if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
-virObjectUnlock(domlist);
+virObjectRWUnlock(domlist);
return -1;
}
virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data);
-virObjectUnlock(domlist);
+virObjectRWUnlock(domlist);
virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags);
@@ -972,7 +972,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
if (skip_missing)
[libvirt] [PATCH v2 4/8] util: Introduce virObjectGetRWLockableObj
Introduce a helper to handle the error path more cleanly. The same
as virObjectGetLockableObj in order to essentially follow the original
logic of commit 'b545f65d' to ensure that the input argument at least
has some validity before using.
Signed-off-by: John Ferlan
---
src/util/virobject.c | 44
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 4903393..c1e4474 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -383,6 +383,22 @@ virObjectGetLockableObj(void *anyobj)
}
+static virObjectRWLockablePtr
+virObjectGetRWLockableObj(void *anyobj)
+{
+virObjectPtr obj;
+
+if (virObjectIsClass(anyobj, virObjectRWLockableClass))
+return anyobj;
+
+obj = anyobj;
+VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+
+return NULL;
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
@@ -427,14 +443,12 @@ virObjectLock(void *anyobj)
void
virObjectRWLockRead(void *anyobj)
{
-if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
-virObjectRWLockablePtr obj = anyobj;
-virRWLockRead(&obj->lock);
-} else {
-virObjectPtr obj = anyobj;
-VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-}
+virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+if (!obj)
+return;
+
+virRWLockRead(&obj->lock);
}
@@ -458,14 +472,12 @@ virObjectRWLockRead(void *anyobj)
void
virObjectRWLockWrite(void *anyobj)
{
-if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
-virObjectRWLockablePtr obj = anyobj;
-virRWLockWrite(&obj->lock);
-} else {
-virObjectPtr obj = anyobj;
-VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-}
+virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+if (!obj)
+return;
+
+virRWLockWrite(&obj->lock);
}
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 6/8] util: Create common error path for invalid object
If virObjectIsClass fails "internally" to virobject.c, create a
macro to generate the VIR_WARN describing what the problem is.
Also improve the checks and message a bit to indicate which was
the failure - whether the obj was NULL or just not the right class
Signed-off-by: John Ferlan
---
src/util/virobject.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 85e5a53..af3f252 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -47,6 +47,17 @@ struct _virClass {
virObjectDisposeCallback dispose;
};
+#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass)\
+do {\
+virObjectPtr obj = anyobj; \
+if (!obj) \
+VIR_WARN("Object cannot be NULL"); \
+else\
+VIR_WARN("Object %p (%s) is not a %s instance", \
+ anyobj, obj->klass->name, #objclass); \
+} while (0)
+
+
static virClassPtr virObjectClass;
static virClassPtr virObjectLockableClass;
static virClassPtr virObjectRWLockableClass;
@@ -370,15 +381,10 @@ virObjectRef(void *anyobj)
static virObjectLockablePtr
virObjectGetLockableObj(void *anyobj)
{
-virObjectPtr obj;
-
if (virObjectIsClass(anyobj, virObjectLockableClass))
return anyobj;
-obj = anyobj;
-VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-
+VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockable);
return NULL;
}
@@ -386,15 +392,10 @@ virObjectGetLockableObj(void *anyobj)
static virObjectRWLockablePtr
virObjectGetRWLockableObj(void *anyobj)
{
-virObjectPtr obj;
-
if (virObjectIsClass(anyobj, virObjectRWLockableClass))
return anyobj;
-obj = anyobj;
-VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
- anyobj, obj ? obj->klass->name : "(unknown)");
-
+VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectRWLockable);
return NULL;
}
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/8] Some virObjectRW* adjustments
v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html
Differences from v1:
* Use the names virObjectRWLockRead, virObjectRWLockWrite and
virObjectRWUnlock
* Instead of an 'int' return, make the virObjectRWLock{Read|Write} be
void returns just like virObject{Lock|Unlock}
* Separate out the magic number check for the virObjectIsClass consumers
into its own patch and describe the reasons for usage.
* Apply the same magic number check to virObject{Ref|Unref} separately.
BTW: This looks and works eally nice with what I have for common objects...
John Ferlan (8):
util: Rename virObjectLockRead to virObjectRWLockRead
util: Introduce and use virObjectRWLockWrite
util: Only have virObjectLock handle virObjectLockable
util: Introduce virObjectGetRWLockableObj
util: Introduce and use virObjectRWUnlock
util: Create common error path for invalid object
util: Add magic number check for object validity
util: Add object checking for virObject{Ref|Unref}
src/conf/virdomainobjlist.c | 62
src/libvirt_private.syms| 4 +-
src/util/virobject.c| 169 +---
src/util/virobject.h| 10 ++-
4 files changed, 169 insertions(+), 76 deletions(-)
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/8] util: Rename virObjectLockRead to virObjectRWLockRead
Since the class it represents is based on virObjectRWLockableClass
and in order to make sure we diffentiate lest anyone somehow
believes they could use virObjectLockRead for a virObjectLockableClass,
let's rename the API to use the RW in the name. Besides the RW locks
refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the
other locks refer to pthread_mutex_{init|lock|unlock|destroy}.
Signed-off-by: John Ferlan
---
src/conf/virdomainobjlist.c | 18 +-
src/libvirt_private.syms| 2 +-
src/util/virobject.c| 11 ---
src/util/virobject.h| 2 +-
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 1d027a4..9bc6603 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
bool ref)
{
virDomainObjPtr obj;
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
if (ref) {
virObjectRef(obj);
@@ -160,7 +160,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainObjPtr obj;
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr);
@@ -204,7 +204,7 @@ virDomainObjPtr
virDomainObjListFindByName(virDomainObjListPtr doms,
{
virDomainObjPtr obj;
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
virObjectUnlock(doms);
@@ -653,7 +653,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
virConnectPtr conn)
{
struct virDomainObjListData data = { filter, conn, active, 0 };
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListCount, &data);
virObjectUnlock(doms);
return data.count;
@@ -697,7 +697,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
{
struct virDomainIDData data = { filter, conn,
0, maxids, ids };
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
virObjectUnlock(doms);
return data.numids;
@@ -751,7 +751,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
struct virDomainNameData data = { filter, conn,
0, 0, maxnames, names };
size_t i;
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
virObjectUnlock(doms);
if (data.oom) {
@@ -792,7 +792,7 @@ virDomainObjListForEach(virDomainObjListPtr doms,
struct virDomainListIterData data = {
callback, opaque, 0,
};
-virObjectLockRead(doms);
+virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListHelper, &data);
virObjectUnlock(doms);
return data.ret;
@@ -925,7 +925,7 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
{
struct virDomainListData data = { NULL, 0 };
-virObjectLockRead(domlist);
+virObjectRWLockRead(domlist);
sa_assert(domlist->objs);
if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
virObjectUnlock(domlist);
@@ -962,7 +962,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
*nvms = 0;
*vms = NULL;
-virObjectLockRead(domlist);
+virObjectRWLockRead(domlist);
for (i = 0; i < ndoms; i++) {
virDomainPtr dom = doms[i];
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 37b815c..99302d2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2305,10 +2305,10 @@ virObjectListFree;
virObjectListFreeCount;
virObjectLock;
virObjectLockableNew;
-virObjectLockRead;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
+virObjectRWLockRead;
virObjectUnlock;
virObjectUnref;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index b1bb378..b97f251 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -399,7 +399,7 @@ virObjectLock(void *anyobj)
/**
- * virObjectLockRead:
+ * virObjectRWLockRead:
* @anyobj: any instance of virObjectRWLockable
*
* Acquire a read lock on @anyobj. The lock must be
@@ -409,9 +409,14 @@ virObjectLock(void *anyobj)
* on the object before locking it (eg virObjectRef).
* The object must be unlocked before releasing this
* reference.
+ *
+ * NB: It's possible to return without the lock if
+ * @anyobj was invalid - this has been considered
+ * a programming error rather than something that
+ * should be checked.
*/
void
-virObjectLockRead(void *anyobj)
+virObjectRWLockRead(void *anyobj)
{
if (v
Re: [libvirt] [PATCH 0/7] Misc improvements
On Mon, Jul 31, 2017 at 10:26:30AM +0100, Daniel P. Berrange wrote:
On Mon, Jul 31, 2017 at 08:58:41AM +0200, Martin Kletzander wrote:
On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote:
> On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
> >
> >
> > On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
> > > On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
> > >>
> > >>
> > >> On 07/28/2017 10:32 AM, Martin Kletzander wrote:
> > >>> On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
> > As I started to turn more object into using RW locks, I've found
> > couple of
> > areas for improvement too.
> >
> > Michal Privoznik (7):
> > virConnect: Update comment for @privateData
> > Report error if virMutexInit fails
> > virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static
> > again
> > virNetworkObjList: Derive from virObjectRWLockable
> > virNodeDeviceObjList: Derive from virObjectRWLockable
> > virConnect: Derive from virObjectRWLockable
> > storageDriver: Use RW locks
> >
> > >>>
> > >>> The patches I have not replied to look fine, but I think it would be
> > >>> easier to modify the common object after John's patches. Are any of
> > >>> those non-conflicting with those series? If yes, I can review those
> > >>> into more detail.
> > >>>
> > >>
> > >> I had contacted Michal via IRC about this when I saw these hit the list.
> > >> I'd prefer to see them handled via a common object set of patches.
> > >>
> > >> However, that said... I wish the RWLockable hadn't just gone in so
> > >> quickly, but what's done is done. I have a couple of other thoughts in
> > >> this area:
> > >>
> > >> * I think virObjectLockableRead should return 0/-1 and have the caller
> > >> handle it.
> > >> * I think there should be a virObjectLockableWrite w/ same return value
> > >> checking.
> > >
> > > I rather disagree with that - it just adds a massive amount more
> > > code to deal with failures from the lock apis that should never
> > > happen unless you've already screwed up somewhere else in your
> > > code. If the object you've passed into the methods has already
> > > been freed, then you're already doomed and trying to recover from
> > > that is never going to be reliable - in fact it could cause more
> > > trouble. The memory for the object passed in is either in the free
> > > pool (and so shouldn't be touched at all), or has been reused and
> > > allocated for some other object now (and so again touching it is
> > > a bad idea). Trying to detect & handle these situatuons is just
> > > doomed to be racy or dangerous or both
> > >
> >
> > I agree w/ the screw up part. Obviously for me it's the RW vs non-RW
> > usage that sent me down this path...
> >
> > Still, I'm not sure what you mean by massive amount of code to deal with
> > failures. I was considering only the RW lock mgmt. Currently only
> > virdomainobjlist was modified to add virObjectLockRead and only done
> > within the last week. There's 9 virObjectLockRead calls and would be 4
> > virObjectLockWrite calls.
> >
> > if (virObjectLock{Read|Write}(obj) < 0)
> > {goto {cleanup|error}|return -1|return NULL};
>
> That's probably buggy if you use existing goto's, because many of
> those cleanup/error locations will call virObjectUnlock(obj), so
> you'll need to introduce another set of gotoo labels to optionally
> skip the unlock step. This is why I think it makes the code more
> complex for dubious benefit.
>
> > The only place this doesn't work properly is the vir*Remove() calls
> > which are void functions. We'd still be "stuck" with them.
>
> Yes that's another scenario I imagined - there are case where it simply
> isn't practical to do cleanup when locking fails.
>
> > Well I can propose the abort() on error if so desired. I agree w/r/t
> > some awful things that could happen...
>
> If we separate virObjectLock vs virObjectRWLockWrite() then, we can
> just unconditionally reference the object in the virObjectLock method
> and just let the abort happen naturally, without needing explicit abort
>
I agree with most of it, but I can't wrap my head around what you meant
by this paragraph, could you explain it to someone whose brain is just
not working yet, please?
Currently we have:
void
virObjectLock(void *anyobj)
{
if (virObjectIsClass(anyobj, virObjectLockableClass)) {
virObjectLockablePtr obj = anyobj;
virMutexLock(&obj->lock);
} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
virObjectRWLockablePtr obj = anyobj;
virRWLockWrite(&obj->lock);
} else {
virObjectPtr obj = anyobj;
VIR_WARN("Object %p (%s) is not a virObjectLockable "
"nor virObjectRWLockable instance",
anyobj, obj ? obj->klass->name : "(unknown)");
}
}
What I'm suggesting is
void
virObjectLock(void *anyobj)
{
virObjectLockablePtr obj
Re: [libvirt] [libvirt-php PATCH 0/7] add bindings for NWFilter APIs
On Mon, 2017-07-31 at 12:35 +0200, Michal Privoznik wrote: > On 06/27/2017 03:45 PM, Dawid Zamirski wrote: > > Dawid - any progress on this? I'd like to make the release as > requested. > For that the NWFilter usage example should be enough. The split of > libvirt-php.c is rather a big change and as such not really a fit for > last minute merge before the release. > > Again, if you don't have the time I can look into this. > > Michal Hi Michal, I've been busy at work lately with little or no spare time for my OSS work :-( I've started on this last week but have been dragged away. I'll try to send the patches by tomorrow morning, which I should manage given I won't run into any difficulties. Regards, Dawid -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
[...] > > I really don't think these changes are a positive move. > > If you have code that is passing in something that is not a valid object, > then silently doing nothing in virObjectRef / virObjectIsClass is not > going to make the code any more correct. In fact you're turning something > that could be an immediate crash (and thus easy to diagnose) into > something that could be silent bad behaviour, which is much harder to > diagnose, or cause a crash much further away from the original root bug. > > Consider the longevity of the patch being on list - I cannot remember all the details, although the commit message does help a bit. I do recall looking at the code and thinking incorrect usage could lead down the road of bad things. For Ref/Unref all that has been checked is !obj and we Incr/Decr a location. For Lock/Unlock as described in my 1/7 response class checking is/was added. Adding in object validity for Ref/Unref at least avoids rogue corruption which is really hard to diagnose in favor of leaving an entrail. Even without the additional check, the @obj someone may have thought they were incr/decr the refcnt isn't going to happen. Still, it just seems better in the event that we don't have a real @obj to at least message that in the hopes that someone trips across it. Similarly for Lock/Unlock same thing. IMO: The patches aren't making it harder to diagnose a problem - they're helping and they're not altering the value of some field for a valid @obj address. But if that's not desired, then fine - at least attempt was made and the feedback has been provided. Tks - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
[...]
>> --- a/src/util/virobject.c
>> +++ b/src/util/virobject.c
>> @@ -410,17 +410,21 @@ virObjectLock(void *anyobj)
>> * The object must be unlocked before releasing this
>> * reference.
>> */
>> -void
>> +int
>
> I'm NACK on this return value change.
>
OK - that's fine.
>> virObjectLockRead(void *anyobj)
>> {
>> if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>> virObjectRWLockablePtr obj = anyobj;
>> virRWLockRead(&obj->lock);
>> +return 0;
>> } else {
>> virObjectPtr obj = anyobj;
>> VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
>> anyobj, obj ? obj->klass->name : "(unknown)");
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("unable to obtain rwlock for object=%p"), anyobj);
>> }
>> +return -1;
>> }
>
> IMHO this should just be simplified to
>
> virObjectLockRead(void *anyobj)
> {
> virObjectRWLockablePtr obj = anyobj;
> virRWLockRead(&obj->lock);
> }
>
>
I'm not as sure for this one though, but I do understand the reasoning
especially if @obj == NULL since we'd have a core. Still if @obj is
passed in as a non NULL address, I don't believe we're going to get a
failure - sure pthread_rwlock_{rdlock|wrlock} or pthread_mutex_lock will
fail, but we don't fail on that. Still getting a VIR_WARN somewhere
would hopefully help us debug. It's too bad we couldn't have the extra
checks be for developer builds only and cause the abort then.
In the FWIW department...
Even in the first commit ('b545f65d') for virObject{Lock|Unlock},
rudimentary error checking such as !obj and using an @obj with the right
class was checked and a VIR_WARN issued.
As I was adding a new class for common objects, I made the mistake noted
in patch 7, so it seemed to be a good thing to do to add a few more
checks along the way and perhaps better entrails. Of course doing that
required a multi-step changes... So, commit id '10c2bb2b1' created
virObjectGetLockableObj as a way to combine the existing checks.
The next steps from my v2 weren't NACK'd, but they weren't ACK'd or
discouraged - they just needed some adjustments. The v3 made the
adjustments (along with more patches), but never got any reviews.
With the addition of RWLockable (commit id '77f4593b') those checks got
wiped out in favor of inline code again. I understand why - one API, one
place to check, etc.
If there's going to be 4 API's, then recreating the original check again
is where I was headed, but not it seems like it's not desired even
though checks like that have been around from the start.
We could proceed with no checks, but before posting patches for that I
would like to make sure that's what is really desired given the history
and side effect of doing so.
Tks -
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php PATCH 0/7] add bindings for NWFilter APIs
On 06/27/2017 03:45 PM, Dawid Zamirski wrote: > On Sat, 2017-06-24 at 19:35 +0200, Michal Privoznik wrote: >> On 06/23/2017 10:58 AM, Michal Privoznik wrote: >>> >>> >>> But this got me thinking, should we follow libvirt's example and >>> finally >>> split src/libvirt-php.c into smaller files that would handle just >>> one >>> object? For example: >>> >>> libvirt-domain.c >>> libvirt-nwfilter.c >>> libvirt-storage.c >>> libvirt-network.c >>> >>> and so on. >> >> Just a clarification on this: I think libvirt-php.c should contain >> just >> the necessary glue/register functions for PHP objects and include >> libvirt-domain.h, libvirt-nwfilter.h, libvirt-storage.h, etc. >> >> The macros we have for dealing with different version of PHP (e.g. >> VIRT_RETURN_RESOURE or VIRT_RETVAL_STRING and friends) can then go to >> util.h (which can be included from all the new *.c files). >> >> libvirt-domain.h could then have all those >> PHP_FUNCTION(libvirt_domain_*) declarations from libvirt-php.h; >> libvirt-nwfilter.h could have all those >> PHP_FUNCTION(libvirt_nwfilter_*) >> declarations, and so on. >> >> The reasoning for this is to have clear dependencies between source >> files. This basically mimics what we have in libvirt too. >> >> Unfortunately, I'm leaving the office for some time now so I will not >> have time to write this myself. So if you guys want to propose the >> patches, please be my guest. Otherwise I'll look into it once I'm >> back. >> >> Michal > > I'll take care of this some time and also add the examples for the > NWFilter API usage some time this or next week (that is, as soon as I'm > done with my php-dbus work) > > Dawid > Dawid - any progress on this? I'd like to make the release as requested. For that the NWFilter usage example should be enough. The split of libvirt-php.c is rather a big change and as such not really a fit for last minute merge before the release. Again, if you don't have the time I can look into this. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] util: Add safety net of checks to ensure valid object
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
> The virObject logic "assumes" that whatever is passed to its API's
> would be some sort of virObjectPtr; however, if it is not then some
> really bad things can happen.
>
> So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
> and virObjectIsClass and the virObject and virObjectLockable class
> consumers have been well behaved and code well tested. Soon there will
> be more consumers and one such consumer tripped over this during testing
> by passing a virHashTablePtr to virObjectIsClass which ends up calling
> virClassIsDerivedFrom using "obj->klass", which wasn't really a klass
> object causing one of those bad things to happen.
>
> To avoid the future possibility that a non virObject class memory was
> passed to some virObject* API, this patch adds two new checks - one
> to validate that the object has the 0xCAFE value in obj->->u.s.magic
> and the other to ensure obj->u.s.magic doesn't "wrap" some day to
> 0xCAFF if we ever get that many object classes. The check is also
> moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would
> be required on the failure path.
>
> It is still left up to the caller to handle the failed API calls just
> as it would be if it passed a NULL opaque pointer anyobj.
>
> Signed-off-by: John Ferlan
> ---
> src/util/virobject.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 2cf4743..dd4c39a 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -47,14 +47,21 @@ struct _virClass {
> virObjectDisposeCallback dispose;
> };
>
> +#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE) !=
> 0xCAFE))
> +
> #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass)\
> do {\
> virObjectPtr obj = anyobj; \
> -if (!obj) \
> -VIR_WARN("Object cannot be NULL"); \
> -else\
> +if (VIR_OBJECT_NOTVALID(obj)) { \
> +if (!obj) \
> +VIR_WARN("Object cannot be NULL"); \
> +else\
> +VIR_WARN("Object %p has a bad magic number %X", \
> + obj, obj->u.s.magic); \
> +} else {\
> VIR_WARN("Object %p (%s) is not a %s instance", \
>anyobj, obj->klass->name, #objclass); \
> +} \
> } while (0)
>
>
> @@ -177,9 +184,14 @@ virClassNew(virClassPtr parent,
> goto error;
>
> klass->parent = parent;
> +klass->magic = virAtomicIntInc(&magicCounter);
> +if (klass->magic > 0xCAFE) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("too many object classes defined"));
> +goto error;
> +}
> if (VIR_STRDUP(klass->name, name) < 0)
> goto error;
> -klass->magic = virAtomicIntInc(&magicCounter);
> klass->objectSize = objectSize;
> klass->dispose = dispose;
>
> @@ -331,7 +343,7 @@ virObjectUnref(void *anyobj)
> {
> virObjectPtr obj = anyobj;
>
> -if (!obj)
> +if (VIR_OBJECT_NOTVALID(obj))
> return false;
>
> bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
> @@ -370,7 +382,7 @@ virObjectRef(void *anyobj)
> {
> virObjectPtr obj = anyobj;
>
> -if (!obj)
> +if (VIR_OBJECT_NOTVALID(obj))
> return NULL;
> virAtomicIntInc(&obj->u.s.refs);
> PROBE(OBJECT_REF, "obj=%p", obj);
> @@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj,
> virClassPtr klass)
> {
> virObjectPtr obj = anyobj;
> -if (!obj)
> +if (VIR_OBJECT_NOTVALID(obj))
> return false;
I really don't think these changes are a positive move.
If you have code that is passing in something that is not a valid object,
then silently doing nothing in virObjectRef / virObjectIsClass is not
going to make the code any more correct. In fact you're turning something
that could be an immediate crash (and thus easy to diagnose) into
something that could be silent bad behaviour, which is much harder to
diagnose, or cause a crash much further away from the original root bug.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://lib
Re: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote:
> Instead of making virObjectLock be the entry point for two
> different types of locks, let's create a virObjectLockWrite API
> which will be able to return status and force the (new) consumers
> of the RWLock to make sure the lock is really obtained when the
> "right" object type is passed.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virdomainobjlist.c | 18 ++
> src/libvirt_private.syms| 1 +
> src/util/virobject.c| 32
> src/util/virobject.h| 4
> 4 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index fed4029..08a51cc 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -330,7 +330,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr
> doms,
> {
> virDomainObjPtr ret;
>
> -virObjectLock(doms);
> +if (virObjectLockWrite(doms) < 0)
> +return NULL;
> ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
> virObjectUnlock(doms);
> return ret;
> @@ -352,7 +353,9 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
> virObjectRef(dom);
> virObjectUnlock(dom);
>
> -virObjectLock(doms);
> +/* We really shouldn't ignore this,
> + * but that ship sailed a long time ago */
> +ignore_value(virObjectLockWrite(doms));
> virObjectLock(dom);
> virHashRemoveEntry(doms->objs, uuidstr);
> virHashRemoveEntry(doms->objsName, dom->def->name);
> @@ -397,9 +400,13 @@ virDomainObjListRename(virDomainObjListPtr doms,
> * hold a lock on dom but not refcount it. */
> virObjectRef(dom);
> virObjectUnlock(dom);
> -virObjectLock(doms);
> +rc = virObjectLockWrite(doms);
> virObjectLock(dom);
> virObjectUnref(dom);
> +if (rc < 0) {
> +VIR_FREE(old_name);
> +return ret;
> +}
>
> if (virHashLookup(doms->objsName, new_name) != NULL) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -576,7 +583,10 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
> if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
> return rc;
>
> -virObjectLock(doms);
> +if (virObjectLockWrite(doms) < 0) {
> +VIR_DIR_CLOSE(dir);
> +return -1;
> +}
>
> while ((ret = virDirRead(dir, &entry, configDir)) > 0) {
> virDomainObjPtr dom;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 37b815c..f1a6510 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2306,6 +2306,7 @@ virObjectListFreeCount;
> virObjectLock;
> virObjectLockableNew;
> virObjectLockRead;
> +virObjectLockWrite;
> virObjectNew;
> virObjectRef;
> virObjectRWLockableNew;
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 73de4d3..c48f88c 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -429,6 +429,38 @@ virObjectLockRead(void *anyobj)
>
>
> /**
> + * virObjectLockWrite:
> + * @anyobj: any instance of virObjectRWLockable
> + *
> + * Acquire a write lock on @anyobj. The lock must be
> + * released by virObjectUnlock.
> + *
> + * The caller is expected to have acquired a reference
> + * on the object before locking it (eg virObjectRef).
> + * The object must be unlocked before releasing this
> + * reference.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virObjectLockWrite(void *anyobj)
> +{
> +if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> +virObjectRWLockablePtr obj = anyobj;
> +virRWLockWrite(&obj->lock);
> +return 0;
> +} else {
> +virObjectPtr obj = anyobj;
> +VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> + anyobj, obj ? obj->klass->name : "(unknown)");
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to obtain rwlock for object=%p"), anyobj);
> +}
> +return -1;
> +}
IMHO this can be simplified
void
virObjectLockWrite(void *anyobj)
{
virObjectRWLockablePtr obj = anyobj;
virRWLockWrite(&obj->lock);
}
because if some caller has passed in a bogus argument (something not an
object, or an object that's already free), then the virObjectIsClass
is just as likley to crash as the simpler code, and IMHO it is preferrable
to get an immediate crash, than to carry on with a warning mesage on
the console and no lock acquired.
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] [PATCH 4/7] util: Introduce virObjectGetRWLockableObj
On Fri, Jul 28, 2017 at 12:38:58PM -0400, John Ferlan wrote:
> Introduce a helper to handle the error path more cleanly. The same
> as virObjectGetLockableObj.
>
> Signed-off-by: John Ferlan
> ---
> src/util/virobject.c | 51 ++-
> 1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index f9047a1..0db98c3 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -383,6 +383,22 @@ virObjectGetLockableObj(void *anyobj)
> }
>
>
> +static virObjectRWLockablePtr
> +virObjectGetRWLockableObj(void *anyobj)
> +{
> +virObjectPtr obj;
> +
> +if (virObjectIsClass(anyobj, virObjectRWLockableClass))
> +return anyobj;
> +
> +obj = anyobj;
> +VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> + anyobj, obj ? obj->klass->name : "(unknown)");
> +
> +return NULL;
> +}
> +
> +
> /**
> * virObjectLock:
> * @anyobj: any instance of virObjectLockable or virObjectRWLockable
> @@ -422,18 +438,13 @@ virObjectLock(void *anyobj)
> int
> virObjectLockRead(void *anyobj)
> {
> -if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> -virObjectRWLockablePtr obj = anyobj;
> -virRWLockRead(&obj->lock);
> -return 0;
> -} else {
> -virObjectPtr obj = anyobj;
> -VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> - anyobj, obj ? obj->klass->name : "(unknown)");
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unable to obtain rwlock for object=%p"), anyobj);
> -}
> -return -1;
> +virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
> +
> +if (!obj)
> +return -1;
> +
> +virRWLockRead(&obj->lock);
> +return 0;
> }
>
>
> @@ -454,18 +465,16 @@ virObjectLockRead(void *anyobj)
> int
> virObjectLockWrite(void *anyobj)
> {
> -if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> -virObjectRWLockablePtr obj = anyobj;
> -virRWLockWrite(&obj->lock);
> -return 0;
> -} else {
> -virObjectPtr obj = anyobj;
> -VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> - anyobj, obj ? obj->klass->name : "(unknown)");
> +virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
> +
> +if (!obj) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("unable to obtain rwlock for object=%p"), anyobj);
> +return -1;
> }
> -return -1;
> +
> +virRWLockWrite(&obj->lock);
> +return 0;
> }
I don't really thing this is a good idea for reasons outlined in the
replies to earlier patches.
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] [PATCH 3/7] util: Only have virObjectLock handle virObjectLockable
On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
> Now that virObjectLockWrite exists to handle the virObjectRWLockable
> objects, let's restore virObjectLock to only handle virObjectLockable
> type locks. There still exists the possibility that the input @anyobj
> isn't a valid object and the resource isn't truly locked, but that
> also exists before commit id '77f4593b'.
>
> This also restores some logic that commit id '77f4593b' removed
> with respect to a common code path that commit id '10c2bb2b' had
> introduced as virObjectGetLockableObj.
>
> Signed-off-by: John Ferlan
> ---
> src/util/virobject.c | 37 +++--
> 1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index c48f88c..f9047a1 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -367,13 +367,28 @@ virObjectRef(void *anyobj)
> }
>
>
> +static virObjectLockablePtr
> +virObjectGetLockableObj(void *anyobj)
> +{
> +virObjectPtr obj;
> +
> +if (virObjectIsClass(anyobj, virObjectLockableClass))
> +return anyobj;
> +
> +obj = anyobj;
> +VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
> + anyobj, obj ? obj->klass->name : "(unknown)");
> +
> +return NULL;
> +}
> +
> +
> /**
> * virObjectLock:
> * @anyobj: any instance of virObjectLockable or virObjectRWLockable
> *
> * Acquire a lock on @anyobj. The lock must be released by
> - * virObjectUnlock. In case the passed object is instance of
> - * virObjectRWLockable a write lock is acquired.
> + * virObjectUnlock.
> *
> * The caller is expected to have acquired a reference
> * on the object before locking it (eg virObjectRef).
> @@ -383,18 +398,12 @@ virObjectRef(void *anyobj)
> void
> virObjectLock(void *anyobj)
> {
> -if (virObjectIsClass(anyobj, virObjectLockableClass)) {
> -virObjectLockablePtr obj = anyobj;
> -virMutexLock(&obj->lock);
> -} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> -virObjectRWLockablePtr obj = anyobj;
> -virRWLockWrite(&obj->lock);
> -} else {
> -virObjectPtr obj = anyobj;
> -VIR_WARN("Object %p (%s) is not a virObjectLockable "
> - "nor virObjectRWLockable instance",
> - anyobj, obj ? obj->klass->name : "(unknown)");
> -}
> +virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
> +
> +if (!obj)
> +return;
> +
> +virMutexLock(&obj->lock);
Again I'd prefer to see
virObjectLockablePtr obj = anyobj;
virMutexLock(&obj->lock);
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] [PATCH 1/7] util: Alter virObjectLockRead to return status
On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote:
> Rather than ignore errors, let's have virObjectLockRead check for
> the correct usage and issue an error when not properly used so
> so that we don't run into situations where the resource we think
> we're locking really isn't locked because the void input parameter
> wasn't valid.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virdomainobjlist.c | 27 ++-
> src/util/virobject.c| 6 +-
> src/util/virobject.h| 2 +-
> 3 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 1d027a4..fed4029 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -118,7 +118,8 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
> bool ref)
> {
> virDomainObjPtr obj;
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return NULL;
> obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
> if (ref) {
> virObjectRef(obj);
> @@ -160,7 +161,8 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr
> doms,
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virDomainObjPtr obj;
>
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return NULL;
> virUUIDFormat(uuid, uuidstr);
>
> obj = virHashLookup(doms->objs, uuidstr);
> @@ -204,7 +206,8 @@ virDomainObjPtr
> virDomainObjListFindByName(virDomainObjListPtr doms,
> {
> virDomainObjPtr obj;
>
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return NULL;
> obj = virHashLookup(doms->objsName, name);
> virObjectRef(obj);
> virObjectUnlock(doms);
> @@ -653,7 +656,8 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
> virConnectPtr conn)
> {
> struct virDomainObjListData data = { filter, conn, active, 0 };
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return -1;
> virHashForEach(doms->objs, virDomainObjListCount, &data);
> virObjectUnlock(doms);
> return data.count;
> @@ -697,7 +701,8 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
> {
> struct virDomainIDData data = { filter, conn,
> 0, maxids, ids };
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return -1;
> virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
> virObjectUnlock(doms);
> return data.numids;
> @@ -751,7 +756,8 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
> struct virDomainNameData data = { filter, conn,
>0, 0, maxnames, names };
> size_t i;
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return -1;
> virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
> virObjectUnlock(doms);
> if (data.oom) {
> @@ -792,7 +798,8 @@ virDomainObjListForEach(virDomainObjListPtr doms,
> struct virDomainListIterData data = {
> callback, opaque, 0,
> };
> -virObjectLockRead(doms);
> +if (virObjectLockRead(doms) < 0)
> +return -1;
> virHashForEach(doms->objs, virDomainObjListHelper, &data);
> virObjectUnlock(doms);
> return data.ret;
> @@ -925,7 +932,8 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
> {
> struct virDomainListData data = { NULL, 0 };
>
> -virObjectLockRead(domlist);
> +if (virObjectLockRead(domlist) < 0)
> +return -1;
> sa_assert(domlist->objs);
> if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
> virObjectUnlock(domlist);
> @@ -962,7 +970,8 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
> *nvms = 0;
> *vms = NULL;
>
> -virObjectLockRead(domlist);
> +if (virObjectLockRead(domlist) < 0)
> +return -1;
> for (i = 0; i < ndoms; i++) {
> virDomainPtr dom = doms[i];
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index b1bb378..73de4d3 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -410,17 +410,21 @@ virObjectLock(void *anyobj)
> * The object must be unlocked before releasing this
> * reference.
> */
> -void
> +int
I'm NACK on this return value change.
> virObjectLockRead(void *anyobj)
> {
> if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> virObjectRWLockablePtr obj = anyobj;
> virRWLockRead(&obj->lock);
> +return 0;
> } else {
> virObjectPtr obj = anyobj;
> VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> anyobj, obj ? obj->klass->name : "(unknown)");
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to obtain rwlock
Re: [libvirt] [PATCH 0/7] Misc improvements
On Mon, Jul 31, 2017 at 08:58:41AM +0200, Martin Kletzander wrote:
> On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote:
> > On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
> > >
> > >
> > > On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
> > > > On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
> > > >>
> > > >>
> > > >> On 07/28/2017 10:32 AM, Martin Kletzander wrote:
> > > >>> On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
> > > As I started to turn more object into using RW locks, I've found
> > > couple of
> > > areas for improvement too.
> > >
> > > Michal Privoznik (7):
> > > virConnect: Update comment for @privateData
> > > Report error if virMutexInit fails
> > > virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static
> > > again
> > > virNetworkObjList: Derive from virObjectRWLockable
> > > virNodeDeviceObjList: Derive from virObjectRWLockable
> > > virConnect: Derive from virObjectRWLockable
> > > storageDriver: Use RW locks
> > >
> > > >>>
> > > >>> The patches I have not replied to look fine, but I think it would be
> > > >>> easier to modify the common object after John's patches. Are any of
> > > >>> those non-conflicting with those series? If yes, I can review those
> > > >>> into more detail.
> > > >>>
> > > >>
> > > >> I had contacted Michal via IRC about this when I saw these hit the
> > > >> list.
> > > >> I'd prefer to see them handled via a common object set of patches.
> > > >>
> > > >> However, that said... I wish the RWLockable hadn't just gone in so
> > > >> quickly, but what's done is done. I have a couple of other thoughts in
> > > >> this area:
> > > >>
> > > >> * I think virObjectLockableRead should return 0/-1 and have the caller
> > > >> handle it.
> > > >> * I think there should be a virObjectLockableWrite w/ same return
> > > >> value
> > > >> checking.
> > > >
> > > > I rather disagree with that - it just adds a massive amount more
> > > > code to deal with failures from the lock apis that should never
> > > > happen unless you've already screwed up somewhere else in your
> > > > code. If the object you've passed into the methods has already
> > > > been freed, then you're already doomed and trying to recover from
> > > > that is never going to be reliable - in fact it could cause more
> > > > trouble. The memory for the object passed in is either in the free
> > > > pool (and so shouldn't be touched at all), or has been reused and
> > > > allocated for some other object now (and so again touching it is
> > > > a bad idea). Trying to detect & handle these situatuons is just
> > > > doomed to be racy or dangerous or both
> > > >
> > >
> > > I agree w/ the screw up part. Obviously for me it's the RW vs non-RW
> > > usage that sent me down this path...
> > >
> > > Still, I'm not sure what you mean by massive amount of code to deal with
> > > failures. I was considering only the RW lock mgmt. Currently only
> > > virdomainobjlist was modified to add virObjectLockRead and only done
> > > within the last week. There's 9 virObjectLockRead calls and would be 4
> > > virObjectLockWrite calls.
> > >
> > > if (virObjectLock{Read|Write}(obj) < 0)
> > > {goto {cleanup|error}|return -1|return NULL};
> >
> > That's probably buggy if you use existing goto's, because many of
> > those cleanup/error locations will call virObjectUnlock(obj), so
> > you'll need to introduce another set of gotoo labels to optionally
> > skip the unlock step. This is why I think it makes the code more
> > complex for dubious benefit.
> >
> > > The only place this doesn't work properly is the vir*Remove() calls
> > > which are void functions. We'd still be "stuck" with them.
> >
> > Yes that's another scenario I imagined - there are case where it simply
> > isn't practical to do cleanup when locking fails.
> >
> > > Well I can propose the abort() on error if so desired. I agree w/r/t
> > > some awful things that could happen...
> >
> > If we separate virObjectLock vs virObjectRWLockWrite() then, we can
> > just unconditionally reference the object in the virObjectLock method
> > and just let the abort happen naturally, without needing explicit abort
> >
>
> I agree with most of it, but I can't wrap my head around what you meant
> by this paragraph, could you explain it to someone whose brain is just
> not working yet, please?
Currently we have:
void
virObjectLock(void *anyobj)
{
if (virObjectIsClass(anyobj, virObjectLockableClass)) {
virObjectLockablePtr obj = anyobj;
virMutexLock(&obj->lock);
} else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
virObjectRWLockablePtr obj = anyobj;
virRWLockWrite(&obj->lock);
} else {
virObjectPtr obj = anyobj;
VIR_WARN("Object %p (%s) is not a virObjectLockable "
"nor virObjectRWLockable instance",
Re: [libvirt] [PATCH v2] tools: virsh: Adding unix socket support to 'domdisplay' command.
On 07/28/2017 11:49 PM, Julio Faracco wrote: > This commit adds the unix socket URL support to 'domdisplay' command. > Before, even if an user was using unix socket to define a spice graphics, > the command 'domdisplay' showed that the settings were not supported. Now, > the command shows the proper URL: spice+unix://foo/bar.sock. > > Settings: > > > > > > > > Before: > virsh # domdisplay --all Windows7 > vnc://127.0.0.1:0 > > After: > virsh # domdisplay --all Windows7 > vnc://127.0.0.1:0 > spice+unix:///tmp/spice.sock > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336720 > > Signed-off-by: Julio Faracco > --- > tools/virsh-domain.c | 52 > +--- > 1 file changed, 41 insertions(+), 11 deletions(-) Looks good. ACK. I'll push it after the release. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status
On 07/28/2017 08:26 PM, John Ferlan wrote: > > > On 07/28/2017 12:56 PM, Pavel Hrdina wrote: >> On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote: >>> Rather than ignore errors, let's have virObjectLockRead check for >>> the correct usage and issue an error when not properly used so >>> so that we don't run into situations where the resource we think >>> we're locking really isn't locked because the void input parameter >>> wasn't valid. >> >> I agree with Dan that this doesn't give any benefit. We should rather >> consider start using abort() since this is a programming error, not >> something that depends on an input from user. It should not happen if >> if it does we have serious issues and abort is a best choice. >> >> Pavel >> > > I'm in the minority, but that's fine. I could also change this patch to > be rename virObjectLockRead to be virObjectRWLockRead as suggested later > on too. Actually, me choosing virObjectLockRead over virObjectRWLockRead was arbitrary. The reason is that my text editor can offer me completions: virObjectLock virObjectLockWrite virObjectLockRead BTW: Following your reasoning here, it should have been called virObjectLockableLock() instead of virObjectLock() ;-) IOW, I'm failing to see the need for 'RW' in the name you're suggesting. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/7] util: Introduce and use virObjectRWUnlock
On 07/28/2017 07:07 PM, Pavel Hrdina wrote: > On Fri, Jul 28, 2017 at 12:38:59PM -0400, John Ferlan wrote: >> Rather than overload virObjectUnlock as commit id '77f4593b' has >> done, create a separate virObjectRWUnlock API that will force the >> consumers to make the proper decision regarding unlocking the >> RWLock's. This restores the virObjectUnlock code to using the >> virObjectGetLockableObj as well. >> >> Signed-off-by: John Ferlan >> --- >> src/conf/virdomainobjlist.c | 36 ++-- >> src/libvirt_private.syms| 1 + >> src/util/virobject.c| 41 +++-- >> src/util/virobject.h| 4 >> 4 files changed, 50 insertions(+), 32 deletions(-) > > The virObjectLockRead and virObjectLockWrite should be probably renamed > to virObjectRWLockRead and virObjectRWLockWrite for consistency with the > virObjectRWUnlock. > > Reviewed-by: Pavel Hrdina This is where I disagree. While you may dislike lock() polymorphism, unlock() is used widely in literature for both mutexes and RW locks. And it makes sense because they are fundamentally the same. I'm not going to NACK this patch only because I find it impolite. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Availability of libvirt-3.6.0 Release Candidate 2
On Mon, Jul 31, 2017 at 10:11:14 +0800, Daniel Veillard wrote: >It is out, I have tagged it in git and pushed the signed tarball and rpms > to the usual place: > ftp://libvirt.org/libvirt/ > > Seems to work for me, Jenkins also complains about libvirt-master-build > having > failed but that's more than 3 days ago so looks suspicious. Maybe someone > need to retrigger a build there I talked to Pavel about this. Looks like the jenkins agent failed (the java exeption log in the console output). After restarting the agent the build seems to go along. I hope that everything passes, since I've pushed the patches that should fix the MinGW build issue I've introduced. The build takes an hour(!) on that box, so it's still in progress. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
