On 07/28/2015 09:56 AM, Peter Krempa wrote:

Since the code is changing the source image path by modifying the existing XML snippet the <backingStore> element does not get dropped.

The element is ignored though but it might start being used in the future.

Before this patch, you'd get: $ virsh change-media --eject --print-xml 10 hdc <disk type="file" device="cdrom"> <driver name="qemu" type="qcow2"/>

<backingStore type="file" index="1"> <format type="qcow2"/> <source file="/var/lib/libvirt/images/vm.1436949097"/> <backingStore/> </backingStore> <target dev="hdc" bus="ide"/> ... </disk>

After:

$ virsh change-media --eject --print-xml 10 hdc <disk type="file" device="cdrom"> <driver name="qemu" type="qcow2"/>

<target dev="hdc" bus="ide"/> ... </disk> --- tools/virsh-domain.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)

Code will need to be updated with recent virsh changes... I'm not quite sure I understand what's being done or why it's necessary from the commit explanation.

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a61656f..d4d6987 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11415,6 +11415,7 @@ vshUpdateDiskXML(xmlNodePtr disk_node, vshUpdateDiskXMLType type) { xmlNodePtr source = NULL; + xmlNodePtr child; char *device_type = NULL; char *ret = NULL;

@@ -11430,10 +11431,16 @@ vshUpdateDiskXML(xmlNodePtr disk_node, }

/* find the current source subelement */ - for (source = disk_node->children; source; source = source->next) { - if (source->type == XML_ELEMENT_NODE && - xmlStrEqual(source->name, BAD_CAST "source")) - break; + for (child = disk_node->children; child; child = child->next) { + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "source")) + source = child; + + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "backingStore")) { + xmlUnlinkNode(child); + xmlFreeNode(child); + }

here you've "freed" child and from how I read the existing code when 'source' goes through the same process, there's a 'source = NULL;' following, so it seems there should be a child = NULL; added. Without that, would the next iteration of the loop reference something that was free'd? That is child is local to this function, it's not passed by reference, thus a subsequent "; child; child = child->next" may be by chance pointing to something valid, but could be pointing to something invalid too. I think with the merge with latest changes and the child = NULL this is ACK-able. John

}

if (type == VSH_UPDATE_DISK_XML_EJECT) {