[libvirt] [PATCH] virsh: Remove <backingStore> when changing cdrom media source

[John Ferlan]

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) {
>