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@xxxxxxxxxx> --- 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/>. TheSave ...+ * 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list