[libvirt] [PATCH 0/4] Automatic lockable object unlocking
A spin-off from the refactors to checkpoints where Eric suggested that we might want to replace virDomainObjEndAPI. See patch 2 for the implementation and patch 3 for example use. Peter Krempa (4): util: object: Note that VIR_AUTOUNREF variables must have the reference util: object: Add VIR_AUTOUNLOCK and VIR_AUTORELEASE qemu: driver: Convert qemuDomainGetInfo to use VIR_AUTORELEASE qemu: Don't repeat virDomainObjEndAPI in qemuDomainBlockPull src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++------------------ src/util/virobject.c | 29 +++++++++++++++++++++++++++++ src/util/virobject.h | 34 +++++++++++++++++++++++++++++++++- 4 files changed, 75 insertions(+), 19 deletions(-) -- 2.21.0
When a pointer is assigned to a variable marked by VIR_AUTOUNREF it will be unref'd when the stack frame of the variable is destroyed and thus users must assign it only when they wish to shed the reference. Add this as a note to the VIR_AUTOUNREF macro. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virobject.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virobject.h b/src/util/virobject.h index fe5dbe7326..6fca4cd166 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -117,7 +117,9 @@ virObjectAutoUnref(void *objptr); * @type: type of an virObject subclass to be unref'd automatically * * Declares a variable of @type which will be automatically unref'd when - * control goes out of the scope. + * control goes out of the scope. The object referenced by the pointer assigned + * to the variable declared by this macro must already have the reference + * counter increased at the time of assignment. */ #define VIR_AUTOUNREF(type) \ __attribute__((cleanup(virObjectAutoUnref))) type -- 2.21.0
Add helpers for using automatic stack'd variable cleaning for lockable objects. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 29 +++++++++++++++++++++++++++++ src/util/virobject.h | 30 ++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7b681fac64..5c8fd6a929 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2635,6 +2635,8 @@ virClassForObjectRWLockable; virClassIsDerivedFrom; virClassName; virClassNew; +virObjectAutoRelease; +virObjectAutoUnlock; virObjectAutoUnref; virObjectFreeCallback; virObjectFreeHashData; diff --git a/src/util/virobject.c b/src/util/virobject.c index 919519735a..6bc8b08c89 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -381,6 +381,35 @@ virObjectAutoUnref(void *objptr) } +/** + * virObjectAutoUnlock: + * + * Helper used by VIR_AUTOUNLOCK + */ +void +virObjectAutoUnlock(void *objptr) +{ + virObjectPtr *obj = objptr; + virObjectUnlock(*obj); + *obj = NULL; +} + + +/** + * virObjectAutoRelease: + * + * Helper used by VIR_AUTORELEASE + */ +void +virObjectAutoRelease(void *objptr) +{ + virObjectPtr *obj = objptr; + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; +} + + /** * virObjectRef: * @anyobj: any instance of virObjectPtr diff --git a/src/util/virobject.h b/src/util/virobject.h index 6fca4cd166..2f3f6d207e 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -112,6 +112,12 @@ virObjectUnref(void *obj); void virObjectAutoUnref(void *objptr); +void +virObjectAutoUnlock(void *objptr); + +void +virObjectAutoRelease(void *objptr); + /** * VIR_AUTOUNREF: * @type: type of an virObject subclass to be unref'd automatically @@ -124,6 +130,30 @@ virObjectAutoUnref(void *objptr); #define VIR_AUTOUNREF(type) \ __attribute__((cleanup(virObjectAutoUnref))) type +/** + * VIR_AUTOUNLOCK: + * @type: type of an virObjectLockable subclass to be unlocked automatically + * + * Declares a variable of @type which will be automatically unlocked when + * control goes out of the scope. The lockable object referenced by the pointer + * assigned to the variable declared by this macro must already be locked + * at the time of assignment. + */ +#define VIR_AUTOUNLOCK(type) \ + __attribute__((cleanup(virObjectAutoUnlock))) type + +/** + * VIR_AUTORELEASE: + * @type: type of an virObjectLockable subclass to be unlocked and unref'd automatically + * + * Declares a variable of @type which will be automatically unlocked and unref'd + * when control goes out of the scope. The lockable object referenced by the + * pointer assigned to the variable declared by this macro must already be + * locked and referenced at the time of assignment. + */ +#define VIR_AUTORELEASE(type) \ + __attribute__((cleanup(virObjectAutoRelease))) type + void * virObjectRef(void *obj); -- 2.21.0
As an example of how to use VIR_AUTORELEASE let's convert this API's 'vm' object to use the new helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c61295bbbb..0988071708 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2723,14 +2723,13 @@ qemuDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { unsigned long long maxmem; - virDomainObjPtr vm; - int ret = -1; + VIR_AUTORELEASE(virDomainObjPtr) vm = NULL; if (!(vm = qemuDomObjFromDomain(dom))) - goto cleanup; + return -1; if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; + return -1; qemuDomainUpdateCurrentMemorySize(vm); @@ -2742,33 +2741,29 @@ qemuDomainGetInfo(virDomainPtr dom, if (VIR_ASSIGN_IS_OVERFLOW(info->maxMem, maxmem)) { virReportError(VIR_ERR_OVERFLOW, "%s", _("Initial memory size too large")); - goto cleanup; + return -1; } if (VIR_ASSIGN_IS_OVERFLOW(info->memory, vm->def->mem.cur_balloon)) { virReportError(VIR_ERR_OVERFLOW, "%s", _("Current memory size too large")); - goto cleanup; + return -1; } if (virDomainObjIsActive(vm)) { if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot read cputime for domain")); - goto cleanup; + return -1; } } if (VIR_ASSIGN_IS_OVERFLOW(info->nrVirtCpu, virDomainDefGetVcpus(vm->def))) { virReportError(VIR_ERR_OVERFLOW, "%s", _("cpu count too large")); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virDomainObjEndAPI(&vm); - return ret; + return 0; } static int -- 2.21.0
Use VIR_AUTORELEASE instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0988071708..ed59e64c10 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18911,21 +18911,19 @@ static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { - virDomainObjPtr vm; + VIR_AUTORELEASE(virDomainObjPtr) vm = NULL; + virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; - if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) { - virDomainObjEndAPI(&vm); + if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) return -1; - } if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot perform block pull while checkpoint exists")); - virDomainObjEndAPI(&vm); return -1; } -- 2.21.0
On 9/27/19 3:05 PM, Peter Krempa wrote:
Use VIR_AUTORELEASE instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0988071708..ed59e64c10 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18911,21 +18911,19 @@ static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { - virDomainObjPtr vm; + VIR_AUTORELEASE(virDomainObjPtr) vm = NULL; + virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1);
if (!(vm = qemuDomObjFromDomain(dom))) return -1;
- if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) { - virDomainObjEndAPI(&vm); + if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) return -1; - }
if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot perform block pull while checkpoint exists")); - virDomainObjEndAPI(&vm); return -1; }
This needs to be rebased. But also, you've missed one virDomainObjEndAPI() at the end. This patch needs to look like this: diff --git c/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 103187709b..1bcb99f13d 100644 --- c/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -18497,25 +18497,23 @@ static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { - virDomainObjPtr vm; - int ret = -1; + VIR_AUTORELEASE(virDomainObjPtr) vm = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomainObjFromDomain(dom))) return -1; if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; + return -1; if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) - goto cleanup; + return -1; - ret = qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); + if (qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags) < 0) + return -1; - cleanup: - virDomainObjEndAPI(&vm); - return ret; + return 0; } Michal
On Mon, Sep 30, 2019 at 16:22:34 +0200, Michal Privoznik wrote:
On 9/27/19 3:05 PM, Peter Krempa wrote:
Use VIR_AUTORELEASE instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0988071708..ed59e64c10 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18911,21 +18911,19 @@ static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { - virDomainObjPtr vm; + VIR_AUTORELEASE(virDomainObjPtr) vm = NULL; + virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1);
if (!(vm = qemuDomObjFromDomain(dom))) return -1;
- if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) { - virDomainObjEndAPI(&vm); + if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) return -1; - }
if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot perform block pull while checkpoint exists")); - virDomainObjEndAPI(&vm); return -1; }
This needs to be rebased. But also, you've missed one virDomainObjEndAPI() at the end. This patch needs to look like this:
Yes as I've pushed the other patches now. Also I'm not really sure I want to go ahead with any other VIR_AUTO things since we'll be converting them to glib afterwards.
On 9/27/19 3:05 PM, Peter Krempa wrote:
A spin-off from the refactors to checkpoints where Eric suggested that we might want to replace virDomainObjEndAPI. See patch 2 for the implementation and patch 3 for example use.
Peter Krempa (4): util: object: Note that VIR_AUTOUNREF variables must have the reference util: object: Add VIR_AUTOUNLOCK and VIR_AUTORELEASE qemu: driver: Convert qemuDomainGetInfo to use VIR_AUTORELEASE qemu: Don't repeat virDomainObjEndAPI in qemuDomainBlockPull
src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++------------------ src/util/virobject.c | 29 +++++++++++++++++++++++++++++ src/util/virobject.h | 34 +++++++++++++++++++++++++++++++++- 4 files changed, 75 insertions(+), 19 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Peter Krempa