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