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

[Martin Kletzander]

On Thu, Nov 26, 2015 at 02:40:55PM +0100, Peter Krempa wrote:
>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.
>

Well, don't ACK it if you don't agree, then.  I was kind of on the
edge, but I just wanted to see how much stuff it would take to fix
it.  Out of curiosity.  And when it worked, I figured I can send it
and we'll see =)

Anyway, it's not on a part of code that would be called all the time,
doesn't consume that much stuff and we have way bigger overkills that
don't even have any "nice to have" features.  And this is virsh, the
thing that's supposed to be user interactive, so I think it doesn't
hurt that much.

At the end, I pushed it, discussions about such tiny things would
probably waste more time than anything else, so let's put an end on at
least this one.

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