[PATCH 2/2] Use more of VIR_XPATH_NODE_AUTORESTORE
Ján Tomko
jtomko at redhat.com
Wed Jun 3 13:32:10 UTC 2020
On a Wednesday in 2020, Michal Privoznik wrote:
>This is convenience macro, use it more. This commit was generated
>using the following spatch:
>
> @@
> symbol node;
> identifier old;
> identifier ctxt;
> type xmlNodePtr;
> @@
> - xmlNodePtr old;
> + VIR_XPATH_NODE_AUTORESTORE(ctxt);
> ...
> - old = ctxt->node;
> ... when != old
> - ctxt->node = old;
>
> @@
> symbol node;
> identifier old;
> identifier ctxt;
> type xmlNodePtr;
> @@
> - xmlNodePtr old = ctxt->node;
> + VIR_XPATH_NODE_AUTORESTORE(ctxt);
> ... when != old
> - ctxt->node = old;
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/conf/cpu_conf.c | 3 +-
> src/conf/domain_conf.c | 7 +--
> src/conf/interface_conf.c | 16 ++-----
> src/conf/netdev_vlan_conf.c | 3 +-
> src/conf/network_conf.c | 25 +++-------
> src/conf/networkcommon_conf.c | 4 +-
> src/conf/node_device_conf.c | 84 +++++++++------------------------
> src/conf/numa_conf.c | 3 +-
> src/conf/snapshot_conf.c | 3 +-
> src/conf/storage_adapter_conf.c | 3 +-
> src/conf/storage_conf.c | 4 +-
> src/conf/virsavecookie.c | 3 +-
> src/cpu/cpu_map.c | 12 +----
> src/cpu/cpu_x86.c | 3 +-
> src/lxc/lxc_domain.c | 4 +-
> src/util/virstorageencryption.c | 8 +---
> src/util/virstoragefile.c | 3 +-
> tools/virsh-domain.c | 6 +--
> 18 files changed, 52 insertions(+), 142 deletions(-)
>
>@@ -797,12 +793,12 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
> xmlNodePtr node,
> virNodeDevCapStoragePtr storage)
> {
>- xmlNodePtr orignode, *nodes = NULL;
>+ VIR_XPATH_NODE_AUTORESTORE(ctxt);
>+ xmlNodePtr *nodes = NULL;
> size_t i;
> int n, ret = -1;
> unsigned long long val;
>
>- orignode = ctxt->node;
> ctxt->node = node;
>
> storage->block = virXPathString("string(./block[1])", ctxt);
>@@ -835,11 +831,8 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
> if (STREQ(type, "hotpluggable")) {
> storage->flags |= VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE;
> } else if (STREQ(type, "removable")) {
>- xmlNodePtr orignode2;
>-
> storage->flags |= VIR_NODE_DEV_CAP_STORAGE_REMOVABLE;
>
>- orignode2 = ctxt->node;
This set orignode2 to 'node'
> ctxt->node = nodes[i];
>
> if (virXPathBoolean("count(./media_available[. = '1']) > 0", ctxt))
>@@ -851,13 +844,10 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
> if (virNodeDevCapsDefParseULongLong("number(./media_size[1])", ctxt, &val, def,
> _("no removable media size supplied for '%s'"),
> _("invalid removable media size supplied for '%s'")) < 0) {
>- ctxt->node = orignode2;
> VIR_FREE(type);
> goto out;
> }
> storage->removable_media_size = val;
>-
>- ctxt->node = orignode2;
This restored it back to 'node'.
The new code can possibly leave it at node[i],
but the code below using the context is specifically run only for
non-removable devices.
My sympathy to anyone touching this code in the future :)
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("unknown storage capability type '%s' for '%s'"),
>@@ -881,7 +871,6 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
> ret = 0;
> out:
> VIR_FREE(nodes);
>- ctxt->node = orignode;
> return ret;
> }
>
[...]
>diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>index 61cd72f714..069e3de064 100644
>--- a/tools/virsh-domain.c
>+++ b/tools/virsh-domain.c
>@@ -6818,7 +6818,7 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
> xmlDocPtr xml = NULL;
> xmlXPathContextPtr ctxt = NULL;
> xmlNodePtr *nodes = NULL;
>- xmlNodePtr old;
>+ VIR_XPATH_NODE_AUTORESTORE(ctxt);
==1644488== Invalid read of size 8
==1644488== at 0x169BE9: virshDomainGetVcpuBitmap (virsh-domain.c:6821)
==1644488== by 0x16965A: virshVcpuinfoInactive (virsh-domain.c:6898)
==1644488== by 0x160132: cmdVcpuinfo (virsh-domain.c:6970)
==1644488== by 0x19890E: vshCommandRun (vsh.c:1291)
==1644488== by 0x1362F9: main (virsh.c:905)
==1644488== Address 0x8 is not stack'd, malloc'd or (recently) free'd
Since 'ctxt' is freed at the end of the function, there is not much
point in restoring it.
> int nnodes;
> size_t i;
> unsigned int curvcpus = 0;
>@@ -6853,8 +6853,6 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
> goto cleanup;
> }
>
>- old = ctxt->node;
>-
> for (i = 0; i < nnodes; i++) {
> ctxt->node = nodes[i];
>
>@@ -6868,8 +6866,6 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
> VIR_FREE(online);
> }
>
>- ctxt->node = old;
>-
> if (virBitmapCountBits(ret) != curvcpus) {
> vshError(ctl, "%s", _("Failed to retrieve vcpu state bitmap"));
> virBitmapFree(ret);
With the crash fixed:
Reviewed-by: Ján Tomko <jtomko at redhat.com>
Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200603/9f79a96a/attachment-0001.sig>
More information about the libvir-list
mailing list