[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]