[libvirt] [PATCH] virsh: Try to keep printed XML pretty with change-media

[Peter Krempa]

On Thu, Nov 26, 2015 at 14:29:20 +0100, Martin Kletzander wrote:
> When adding a new media with change-media and --print-xml, let's try
> making it more readable and nice.
> 
> Before:
> <disk type="file" device="cdrom">
> ...
>       <target dev="hdb" bus="ide"/>
>       <address type="drive" controller="0" bus="0" target="0" unit="1"/>
>     <source file="/tmp/a.iso"/></disk>
> 
> After:
> <disk type="file" device="cdrom">
> ...
>       <source file="/tmp/a.iso"/>
>       <target dev="hdb" bus="ide"/>
>       <address type="drive" controller="0" bus="0" target="0" unit="1"/>
>     </disk>
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1219719
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  tools/virsh-domain.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index bd00785622b2..16b01d3dc631 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11566,7 +11566,10 @@ virshUpdateDiskXML(xmlNodePtr disk_node,
>                     const char *target,
>                     virshUpdateDiskXMLType type)
>  {
> +    xmlNodePtr tmp = NULL;
>      xmlNodePtr source = NULL;
> +    xmlNodePtr target_node = NULL;
> +    xmlNodePtr text_node = NULL;
>      char *device_type = NULL;
>      char *ret = NULL;
>      char *startupPolicy = NULL;
> @@ -11583,9 +11586,31 @@ virshUpdateDiskXML(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"))
> +    for (tmp = disk_node->children; tmp; tmp = tmp->next) {
> +        /*
> +         * Safe the last text node before the <target/>.  The

Save ...

> +         * reasoning behind this is that the target node will be
> +         * present in this case and also has a proper indentation.
> +         */
> +        if (!target_node && tmp->type == XML_TEXT_NODE)
> +            text_node = tmp;
> +
> +        /*
> +         * We need only element nodes from now on.
> +         */
> +        if (tmp->type != XML_ELEMENT_NODE)
> +            continue;
> +
> +        if (xmlStrEqual(tmp->name, BAD_CAST "source"))
> +            source = tmp;
> +
> +        if (xmlStrEqual(tmp->name, BAD_CAST "target"))
> +            target_node = tmp;
> +
> +        /*
> +         * We've found all we needed.
> +         */
> +        if (source && target_node)
>              break;
>      }
> 
> @@ -11637,7 +11662,22 @@ virshUpdateDiskXML(xmlNodePtr disk_node,
> 
>          if (startupPolicy)
>              xmlNewProp(source, BAD_CAST "startupPolicy", BAD_CAST startupPolicy);
> -        xmlAddChild(disk_node, source);
> +
> +        /*
> +         * So that the output XML looks nice in case anyone calls
> +         * 'change-media' with '--print-xml', let's attach the source
> +         * before target...
> +         */
> +        xmlAddPrevSibling(target_node, source);
> +
> +        /*
> +         * ... and duplicate the text node doing the indentation just
> +         * so it's more easily readable.  And don't make it fatal.
> +         */
> +        if ((tmp = xmlCopyNode(text_node, 0))) {
> +            if (!xmlAddPrevSibling(target_node, tmp))
> +                xmlFreeNode(tmp);
> +        }
>      }

Well, I think having all this code just to have pretty XML for
something that was used mostly for developer testing seems really
useless to me. I wanted to close that BZ, but Eric said it would be nice
to have. I don't think it's nice ... it's overkill.

ACK, but I don't think it's worth.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151126/cf256a14/attachment-0001.sig>