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(a)redhat.com&gt; > --- > 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