[libvirt] [PATCHv2 12/13] snapshot: minor cleanups from reviewing indentation

[Eric Blake]

On 10/21/2011 02:08 PM, Peter Krempa wrote:

>> - if (def->data.ethernet.dev)
>> - virBufferEscapeString(buf, "<source dev='%s'/>\n",
>> - def->data.ethernet.dev);
>> + virBufferEscapeString(buf, "<source dev='%s'/>\n",
>> + def->data.ethernet.dev);
>> if (def->data.ethernet.ipaddr)
>> virBufferAsprintf(buf, "<ip address='%s'/>\n",
>> def->data.ethernet.ipaddr);
>
> This looks like it could be changed to virBufferEscapeString and drop
> the test. (or is the ip address being mangled by the escape function?)

virBufferEscapeString prints nothing if the last argument is NULL.  Some 
of the code uses that as a nice side-effect so that "blah%s" omits blah 
if the string is also absent.

But virBufferAsprintf can't take that shortcut - it's a var-arg 
function, and besides, how do you know whether someone meant to pass 
NULL for %p vs. trying to pass NULL to bypass output?

I suppose I could convert some if(string)virBufferAsprintf(,string) to 
the simpler virBufferEscapeString(string), although it feels weird 
calling virBufferEscapeString for its side effect of omitting output 
when string is NULL, when there is nothing in the string that will be 
escaped.  But I'd rather save that for a separate patch.


>>
>> virBufferAsprintf(buf, "<memory mode='%s' nodeset='%s'/>\n",
>> - virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode),
>> + virDomainNumatuneMemModeTypeToString(def->numatune.
>> + memory.mode),
>
> They shoul have made terminals with more than 80 cols at the very
> beginning :( This looked a little bit confusing to me at first,
> interpreting the dot as a comma ... but, well, it works.

Good point on readability - I could move the . to the front of the line:

blah(dev->numatune
      .memory.mode)

or use a temporary variable, to avoid the confusion.
>
> ACK, you can safely ignore my comments to this patch, as they don't
> address any important issues :). I was looking for 13/13, but couldn't
> find one, so I suppose this is the last one.

13/13 exists, but I don't want to apply it, so it doesn't hurt my 
feelings if you don't review it:
https://www.redhat.com/archives/libvir-list/2011-September/msg01267.html

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org