Re: [libvirt] [PATCH] virsh: Remove <backingStore> when changing cdrom media source
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) {