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) { > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list