On 03/06/2014 08:24 AM, Laine Stump wrote:
> Many of the domain xml format functions (including all of the device
> format functions) had hard-coded spaces, which made for incorrect
> indentation when those functions were called in a different context
> (for example, commit 2122cf39 added <interface> XML into the document
> provided to a network hook script, and in this case it should have
> been indented by 2 spaces, but was instead indented by 6 spaces).
>
> To make it possible to insert a properly indented device anywhere into
> an XML document, this patch removes hardcoded spaces from the
> formatting functions, and calls virBufferAdjustIndent() at appropriate
> places instead. (a regex search of domain_conf.c was done to assure
> that all occurrences of hardcoded spaces were removed).
>
> virDomainDiskSourceDefFormatInternal() is also called from
> snapshot_conf.c, so two virBufferAdjustIndent() calls were temporarily
> added around that call - those functions will have hardcoded spaces
> removed in a separate patch.
>
> This could cause some conflicts when backporting future changes to the
> formatting functions to older branches, but fortunately the changes
> are almost all trivial, so conflict resolution will be obvious.
> ---
> src/conf/domain_conf.c | 599 ++++++++++++++++++++++++++---------------------
> src/conf/snapshot_conf.c | 4 +-
> 2 files changed, 336 insertions(+), 267 deletions(-)
>
> @@ -14671,8 +14671,10 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf,
>
> if (def->label) {
> virBufferAddLit(buf, ">\n");
> - virBufferEscapeString(buf, " <label>%s</label>\n",
> + virBufferAdjustIndent(buf, 2);
> + virBufferEscapeString(buf, "<label>%s</label>\n",
> def->label);
> + virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</seclabel>\n");
For small hunks like this where all the context is local, this just adds
lines of code. I'm almost of the opinion that it's easier to have each
function start with no indent, and any strings it prints locally are
correctly indented (allowing leading whitespace), while any helper
functions it calls are surrounded by AdjustIndent (since the helper
function also prints at a local top level).
> } else {
> virBufferAddLit(buf, "/>\n");
> @@ -14684,14 +14686,16 @@ static int
> virDomainLeaseDefFormat(virBufferPtr buf,
> virDomainLeaseDefPtr def)
> {
> - virBufferAddLit(buf, " <lease>\n");
> - virBufferEscapeString(buf, " <lockspace>%s</lockspace>\n", def->lockspace);
> - virBufferEscapeString(buf, " <key>%s</key>\n", def->key);
> - virBufferEscapeString(buf, " <target path='%s'", def->path);
> + virBufferAddLit(buf, "<lease>\n");
That is, changes like this are good (this is the first print of the
function, so it should be done as if at the top level)...
> + virBufferAdjustIndent(buf, 2);
> + virBufferEscapeString(buf, "<lockspace>%s</lockspace>\n", def->lockspace);
> + virBufferEscapeString(buf, "<key>%s</key>\n", def->key);
> + virBufferEscapeString(buf, "<target path='%s'", def->path);
...while all these are local so using " <" would be just as easy to
follow. On the other hand, your argument of consistency (that it is
easier to grep for '" ' violations) is mechanically testable, while my
idea of local indentation is not. So I can live with your patch.
The clincher is that our testsuite validates that we didn't break any
output :)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list