[libvirt] [PATCH 0/5] Yet another set of VIR_AUTO-related patches
Peter Krempa (5): cfg: Add VIR_AUTO(UNREF|CLEAN|STRINGLIST) to sc_requirere_attribute_cleanup_initialization util: object: Reset pointer when unrefing object in virObjectAutoUnref util: alloc: Clarify docs for VIR_DEFINE_AUTOCLEAN_FUNC util: XML: Introduce automatic reset of XPath's current node qemu: Use VIR_XPATH_NODE_AUTORESTORE when XPath context is modified cfg.mk | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 3 +-- src/qemu/qemu_domain.c | 6 ++---- src/qemu/qemu_migration_cookie.c | 9 +++------ src/util/viralloc.h | 4 ++-- src/util/virobject.c | 1 + src/util/virxml.c | 10 ++++++++++ src/util/virxml.h | 22 ++++++++++++++++++++++ 9 files changed, 43 insertions(+), 15 deletions(-) -- 2.20.1
The syntaxcheck should also check the new ones. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 7762422da7..f99b8ae631 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1070,7 +1070,7 @@ sc_prohibit_backslash_alignment: # Rule to ensure that variables declared using a cleanup macro are # always initialized. sc_require_attribute_cleanup_initialization: - @prohibit='VIR_AUTO((FREE|PTR)\(.+\)|CLOSE) *[^=]+;' \ + @prohibit='VIR_AUTO((FREE|PTR|UNREF|CLEAN)\(.+\)|CLOSE|STRINGLIST) *[^=]+;' \ in_vc_files='\.[chx]$$' \ halt='variable declared with a cleanup macro must be initialized' \ $(_sc_search_regexp) -- 2.20.1
d/'requirere'/ how about: syntax-check: require intialization for new VIR_AUTO macros On Tue, Feb 26, 2019 at 06:08:08PM +0100, Peter Krempa wrote:
The syntaxcheck should also check the new ones.
s/syntaxcheck/syntax-check/
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index 7762422da7..f99b8ae631 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1070,7 +1070,7 @@ sc_prohibit_backslash_alignment: # Rule to ensure that variables declared using a cleanup macro are # always initialized. sc_require_attribute_cleanup_initialization: - @prohibit='VIR_AUTO((FREE|PTR)\(.+\)|CLOSE) *[^=]+;' \ + @prohibit='VIR_AUTO((FREE|PTR|UNREF|CLEAN)\(.+\)|CLOSE|STRINGLIST) *[^=]+;' \
Note that VIR_AUTOSTRINGLIST is not merged yet.
in_vc_files='\.[chx]$$' \ halt='variable declared with a cleanup macro must be initialized' \ $(_sc_search_regexp)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The helper function is used by the VIR_AUTOUNREF macro. Prior art is to clear the pointer even if the variable goes out of scope. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virobject.c b/src/util/virobject.c index a4cbd08077..f08c18ce44 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -372,6 +372,7 @@ virObjectAutoUnref(void *objptr) { virObjectPtr *obj = objptr; virObjectUnref(*obj); + *obj = NULL; } -- 2.20.1
The commit summary mentions object thrice. util: reset pointer in virObjectAutoUnref is less wasteful. Otherwise at least s/unrefing/unreffing/ On Tue, Feb 26, 2019 at 06:08:09PM +0100, Peter Krempa wrote:
The helper function is used by the VIR_AUTOUNREF macro. Prior art is to clear the pointer even if the variable goes out of scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virobject.c | 1 + 1 file changed, 1 insertion(+)
This saves developer time that would otherwise be spent on bikeshedding. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Document that @func must take pointer to @type. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 15451d4673..cecabc246c 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -623,8 +623,8 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * @func: cleanup function to be automatically called * * This macro defines a function for automatic clearing of - * resources in a stack'd variable of type @type. This newly - * defined function works as a necessary wrapper around @func. + * resources in a stack'd variable of type @type. Note that @func must + * take pointer to @type. */ # define VIR_DEFINE_AUTOCLEAN_FUNC(type, func) \ static inline void VIR_AUTOCLEAN_FUNC_NAME(type)(type *_ptr) \ -- 2.20.1
On Tue, Feb 26, 2019 at 06:08:10PM +0100, Peter Krempa wrote:
Document that @func must take pointer to @type.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Quite a few parts modify the XPath context current node to shif the scope and allow easier queries. This also means that the node needs to be restored afterwards. Introduce a macro based on 'VIR_AUTOCLEAN' which adds a local structure on the stack remembering the original node along with a function which will make sure that the node is reset when the local structure leaves scope. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 10 ++++++++++ src/util/virxml.h | 22 ++++++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..ee5d9957b0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3246,6 +3246,7 @@ virXMLValidatorFree; virXMLValidatorInit; virXMLValidatorValidate; virXPathBoolean; +virXPathContextNodeRestore; virXPathInt; virXPathLong; virXPathLongHex; diff --git a/src/util/virxml.c b/src/util/virxml.c index 8eb201fddf..f55b9a362c 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1398,3 +1398,13 @@ virXMLFormatElement(virBufferPtr buf, virBufferFreeAndReset(childBuf); return ret; } + + +void +virXPathContextNodeRestore(virXPathContextNodeSavePtr save) +{ + if (!save->ctxt) + return; + + save->ctxt->node = save->node; +} diff --git a/src/util/virxml.h b/src/util/virxml.h index 23a4da1b7e..1bd2c0e518 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -219,4 +219,26 @@ virXMLFormatElement(virBufferPtr buf, virBufferPtr attrBuf, virBufferPtr childBuf); +struct _virXPathContextNodeSave { + xmlXPathContextPtr ctxt; + xmlNodePtr node; +}; +typedef struct _virXPathContextNodeSave virXPathContextNodeSave; +typedef virXPathContextNodeSave *virXPathContextNodeSavePtr; + +void +virXPathContextNodeRestore(virXPathContextNodeSavePtr save); + +VIR_DEFINE_AUTOCLEAN_FUNC(virXPathContextNodeSave, virXPathContextNodeRestore); + +/** + * VIR_XPATH_NODE_AUTORESTORE: + * @ctxt: XML XPath context pointer + * + * This macro ensures that when the scope where it's used ends @ctxt's current + * node pointer is reset to the original value when this macro was used. + */ +# define VIR_XPATH_NODE_AUTORESTORE(ctxt) \ + VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt ## CtxtSave = {(ctxt), (ctxt)->node} + #endif /* LIBVIRT_VIRXML_H */ -- 2.20.1
On 2/26/19 11:08 AM, Peter Krempa wrote:
Quite a few parts modify the XPath context current node to shif the
shift
scope and allow easier queries. This also means that the node needs to be restored afterwards.
Introduce a macro based on 'VIR_AUTOCLEAN' which adds a local structure on the stack remembering the original node along with a function which will make sure that the node is reset when the local structure leaves scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 10 ++++++++++ src/util/virxml.h | 22 ++++++++++++++++++++++ 3 files changed, 33 insertions(+)
Nice idea.
+/** + * VIR_XPATH_NODE_AUTORESTORE: + * @ctxt: XML XPath context pointer + * + * This macro ensures that when the scope where it's used ends @ctxt's current
Reads better with s/ends/ends,/
+ * node pointer is reset to the original value when this macro was used. + */ +# define VIR_XPATH_NODE_AUTORESTORE(ctxt) \ + VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt ## CtxtSave = {(ctxt), (ctxt)->node}
Worth using C99 syntax as in { .ctxt = (ctxt), .node = (ctxt)->node, } ? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Feb 26, 2019 at 11:29:33 -0600, Eric Blake wrote:
On 2/26/19 11:08 AM, Peter Krempa wrote:
Quite a few parts modify the XPath context current node to shif the
shift
scope and allow easier queries. This also means that the node needs to be restored afterwards.
Introduce a macro based on 'VIR_AUTOCLEAN' which adds a local structure on the stack remembering the original node along with a function which will make sure that the node is reset when the local structure leaves scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 10 ++++++++++ src/util/virxml.h | 22 ++++++++++++++++++++++ 3 files changed, 33 insertions(+)
Nice idea.
+/** + * VIR_XPATH_NODE_AUTORESTORE: + * @ctxt: XML XPath context pointer + * + * This macro ensures that when the scope where it's used ends @ctxt's current
Reads better with s/ends/ends,/
+ * node pointer is reset to the original value when this macro was used. + */ +# define VIR_XPATH_NODE_AUTORESTORE(ctxt) \ + VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt ## CtxtSave = {(ctxt), (ctxt)->node}
Worth using C99 syntax as in { .ctxt = (ctxt), .node = (ctxt)->node, } ?
It would require renaming the argument of the macro as it would be replaced otherwise. Interrestingly in most cases it would actually work as in most cases the macro is used with 'ctxt'.
On 2/27/19 3:27 AM, Peter Krempa wrote:
+ * node pointer is reset to the original value when this macro was used. + */ +# define VIR_XPATH_NODE_AUTORESTORE(ctxt) \ + VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt ## CtxtSave = {(ctxt), (ctxt)->node}
Worth using C99 syntax as in { .ctxt = (ctxt), .node = (ctxt)->node, } ?
It would require renaming the argument of the macro as it would be replaced otherwise. Interrestingly in most cases it would actually work as in most cases the macro is used with 'ctxt'.
And hence why it's a good idea to use macro parameter names which can't conflict with normal cods, such as: #define VIR_XPATH_NODE_AUTORESTORE(_ctxt) \ VIR_AUTOCLEAN(virXPathContextNodeSave) _ctxt##CtxtSave = { \ .ctxt = (_ctxt), .node = (_ctxt)->node, } But I'll leave it up to you to decide whether it makes sense to worry about C99 initializers. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Feb 26, 2019 at 18:08:11 +0100, Peter Krempa wrote:
Quite a few parts modify the XPath context current node to shif the scope and allow easier queries. This also means that the node needs to be restored afterwards.
Introduce a macro based on 'VIR_AUTOCLEAN' which adds a local structure on the stack remembering the original node along with a function which will make sure that the node is reset when the local structure leaves scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 10 ++++++++++ src/util/virxml.h | 22 ++++++++++++++++++++++ 3 files changed, 33 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..ee5d9957b0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3246,6 +3246,7 @@ virXMLValidatorFree; virXMLValidatorInit; virXMLValidatorValidate; virXPathBoolean; +virXPathContextNodeRestore; virXPathInt; virXPathLong; virXPathLongHex; diff --git a/src/util/virxml.c b/src/util/virxml.c index 8eb201fddf..f55b9a362c 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1398,3 +1398,13 @@ virXMLFormatElement(virBufferPtr buf, virBufferFreeAndReset(childBuf); return ret; } + + +void +virXPathContextNodeRestore(virXPathContextNodeSavePtr save) +{ + if (!save->ctxt) + return; + + save->ctxt->node = save->node; +} diff --git a/src/util/virxml.h b/src/util/virxml.h index 23a4da1b7e..1bd2c0e518 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -219,4 +219,26 @@ virXMLFormatElement(virBufferPtr buf, virBufferPtr attrBuf, virBufferPtr childBuf);
+struct _virXPathContextNodeSave { + xmlXPathContextPtr ctxt; + xmlNodePtr node; +}; +typedef struct _virXPathContextNodeSave virXPathContextNodeSave; +typedef virXPathContextNodeSave *virXPathContextNodeSavePtr; + +void +virXPathContextNodeRestore(virXPathContextNodeSavePtr save); + +VIR_DEFINE_AUTOCLEAN_FUNC(virXPathContextNodeSave, virXPathContextNodeRestore); + +/** + * VIR_XPATH_NODE_AUTORESTORE: + * @ctxt: XML XPath context pointer + * + * This macro ensures that when the scope where it's used ends @ctxt's current + * node pointer is reset to the original value when this macro was used. + */ +# define VIR_XPATH_NODE_AUTORESTORE(ctxt) \ + VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt ## CtxtSave = {(ctxt), (ctxt)->node}
Oops, forgot to commit some local changes: diff --git a/src/util/virxml.h b/src/util/virxml.h index 1bd2c0e518..72b9d749d0 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -239,6 +239,7 @@ VIR_DEFINE_AUTOCLEAN_FUNC(virXPathContextNodeSave, virXPathContextNodeRestore); * node pointer is reset to the original value when this macro was used. */ # define VIR_XPATH_NODE_AUTORESTORE(ctxt) \ - VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt ## CtxtSave = {(ctxt), (ctxt)->node} + VIR_AUTOCLEAN(virXPathContextNodeSave) ctxt##CtxtSave = {ctxt, ctxt->node}; \ + ignore_value(&ctxt##CtxtSave) #endif /* LIBVIRT_VIRXML_H */
On Tue, Feb 26, 2019 at 06:08:11PM +0100, Peter Krempa wrote:
Quite a few parts modify the XPath context current node to shif the
shift
scope and allow easier queries. This also means that the node needs to be restored afterwards.
Introduce a macro based on 'VIR_AUTOCLEAN' which adds a local structure on the stack remembering the original node along with a function which will make sure that the node is reset when the local structure leaves scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 10 ++++++++++ src/util/virxml.h | 22 ++++++++++++++++++++++ 3 files changed, 33 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the new helper when moving around the current node of the XPath context. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 +-- src/qemu/qemu_domain.c | 6 ++---- src/qemu/qemu_migration_cookie.c | 9 +++------ 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b48bcbebee..1d1bb80b39 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3042,7 +3042,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps, char *str = NULL; xmlNodePtr hostCPUNode; xmlNodePtr *nodes = NULL; - xmlNodePtr oldnode = ctxt->node; + VIR_XPATH_NODE_AUTORESTORE(ctxt); qemuMonitorCPUModelInfoPtr hostCPU = NULL; int ret = -1; size_t i; @@ -3160,7 +3160,6 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps, ret = 0; cleanup: - ctxt->node = oldnode; VIR_FREE(str); VIR_FREE(nodes); qemuMonitorCPUModelInfoFree(hostCPU); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 59fe1eb401..690a57521e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2717,7 +2717,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, xmlXPathContextPtr ctxt, virDomainDiskDefPtr disk) { - xmlNodePtr savedNode = ctxt->node; + VIR_XPATH_NODE_AUTORESTORE(ctxt); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); char *format = NULL; char *type = NULL; @@ -2772,7 +2772,6 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, cleanup: VIR_FREE(format); VIR_FREE(type); - ctxt->node = savedNode; return ret; } @@ -2827,7 +2826,7 @@ qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv, xmlXPathContextPtr ctxt) { - xmlNodePtr savedNode = ctxt->node; + VIR_XPATH_NODE_AUTORESTORE(ctxt); char *tmp = NULL; int ret = -1; @@ -2884,7 +2883,6 @@ qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm, ret = 0; cleanup: - ctxt->node = savedNode; VIR_FREE(tmp); return ret; } diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index ae2546f98b..74b8575a91 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -946,7 +946,7 @@ qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt) int n; xmlNodePtr *interfaces = NULL; char *vporttype; - xmlNodePtr save_ctxt = ctxt->node; + VIR_XPATH_NODE_AUTORESTORE(ctxt); if (VIR_ALLOC(optr) < 0) goto error; @@ -978,7 +978,6 @@ qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt) VIR_FREE(interfaces); cleanup: - ctxt->node = save_ctxt; return optr; error: @@ -997,7 +996,7 @@ qemuMigrationCookieNBDXMLParse(xmlXPathContextPtr ctxt) size_t i; int n; xmlNodePtr *disks = NULL; - xmlNodePtr save_ctxt = ctxt->node; + VIR_XPATH_NODE_AUTORESTORE(ctxt); if (VIR_ALLOC(ret) < 0) goto error; @@ -1044,7 +1043,6 @@ qemuMigrationCookieNBDXMLParse(xmlXPathContextPtr ctxt) VIR_FREE(port); VIR_FREE(capacity); VIR_FREE(disks); - ctxt->node = save_ctxt; return ret; error: qemuMigrationCookieNBDFree(ret); @@ -1058,7 +1056,7 @@ qemuMigrationCookieStatisticsXMLParse(xmlXPathContextPtr ctxt) { qemuDomainJobInfoPtr jobInfo = NULL; qemuMonitorMigrationStats *stats; - xmlNodePtr save_ctxt = ctxt->node; + VIR_XPATH_NODE_AUTORESTORE(ctxt); if (!(ctxt->node = virXPathNode("./statistics", ctxt))) goto cleanup; @@ -1136,7 +1134,6 @@ qemuMigrationCookieStatisticsXMLParse(xmlXPathContextPtr ctxt) virXPathInt("string(./" VIR_DOMAIN_JOB_AUTO_CONVERGE_THROTTLE "[1])", ctxt, &stats->cpu_throttle_percentage); cleanup: - ctxt->node = save_ctxt; return jobInfo; } -- 2.20.1
On Tue, Feb 26, 2019 at 06:08:12PM +0100, Peter Krempa wrote:
Use the new helper when moving around the current node of the XPath context.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 +-- src/qemu/qemu_domain.c | 6 ++---- src/qemu/qemu_migration_cookie.c | 9 +++------ 3 files changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa