On 04/19/2012 04:21 PM, Eric Blake wrote:
> I almost copied-and-pasted some redundant () into my new code,
> and figured a general cleanup prereq patch would be better instead.
>
> * src/conf/domain_conf.c (virDomainLeaseDefParseXML)
> (virDomainDiskDefParseXML, virDomainFSDefParseXML)
> (virDomainActualNetDefParseXML, virDomainNetDefParseXML)
> (virDomainGraphicsDefParseXML, virDomainVideoAccelDefParseXML)
> (virDomainVideoDefParseXML, virDomainHostdevFind)
> (virDomainControllerInsertPreAlloced, virDomainDefParseXML)
> (virDomainObjParseXML, virDomainCpuSetFormat)
> (virDomainCpuSetParse, virDomainDiskDefFormat)
> (virDomainActualNetDefFormat, virDomainNetDefFormat)
> (virDomainTimerDefFormat, virDomainGraphicsListenDefFormat)
> (virDomainDefFormatInternal, virDomainNetGetActualHostdev)
> (virDomainNetGetActualBandwidth, virDomainGraphicsGetListen):
> Reduce extra (), per coding styles.
Actually the libvirt coding style given in HACKING doesn't say anything
about superfluous parentheses (not that I can find). Maybe you're
thinking of coreutils, or gnulib?
> ---
>
> This touches enough lines that I won't claim it to be trivial, but
> it should have no semantic impact.
>
> src/conf/domain_conf.c | 210 +++++++++++++++++++++++------------------------
> 1 files changed, 103 insertions(+), 107 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8f352ea..0d87970 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3233,14 +3233,13 @@ virDomainLeaseDefParseXML(xmlNodePtr node)
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE) {
> - if ((key == NULL) &&
Yeah, I prefer !key over (key == NULL) as well
> - (xmlStrEqual(cur->name, BAD_CAST "key"))) {
Heh. Wonder what the reasoning was behind *those* extra parens. Must be
the result of some automated search/replace.
> + if (!key && xmlStrEqual(cur->name, BAD_CAST "key")) {
> key = (char *)xmlNodeGetContent(cur);
> - } else if ((lockspace == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "lockspace"))) {
> + } else if (!lockspace &&
> + xmlStrEqual(cur->name, BAD_CAST "lockspace")) {
> lockspace = (char *)xmlNodeGetContent(cur);
> - } else if ((path == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "target"))) {
> + } else if (!path &&
> + xmlStrEqual(cur->name, BAD_CAST "target")) {
> path = virXMLPropString(cur, "path");
> offset = virXMLPropString(cur, "offset");
> }
> @@ -3355,8 +3354,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE) {
> - if ((source == NULL && hosts == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "source"))) {
> + if (!source && !hosts &&
> + xmlStrEqual(cur->name, BAD_CAST "source")) {
>
> sourceNode = cur;
>
> @@ -3433,8 +3432,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> those broken apps */
> if (source && STREQ(source, ""))
> VIR_FREE(source);
> - } else if ((target == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "target"))) {
> + } else if (!target &&
> + xmlStrEqual(cur->name, BAD_CAST "target")) {
> target = virXMLPropString(cur, "dev");
> bus = virXMLPropString(cur, "bus");
> tray = virXMLPropString(cur, "tray");
> @@ -3444,8 +3443,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> if (target &&
> STRPREFIX(target, "ioemu:"))
> memmove(target, target+6, strlen(target)-5);
> - } else if ((driverName == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
> + } else if (!driverName &&
> + xmlStrEqual(cur->name, BAD_CAST "driver")) {
> driverName = virXMLPropString(cur, "name");
> driverType = virXMLPropString(cur, "type");
> cachetag = virXMLPropString(cur, "cache");
> @@ -3579,8 +3578,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> cur);
> if (encryption == NULL)
> goto error;
> - } else if ((serial == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "serial"))) {
> + } else if (!serial &&
> + xmlStrEqual(cur->name, BAD_CAST "serial")) {
> serial = (char *)xmlNodeGetContent(cur);
> } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) {
> /* boot is parsed as part of virDomainDeviceInfoParseXML */
> @@ -4099,8 +4098,8 @@ virDomainFSDefParseXML(xmlNodePtr node,
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE) {
> - if ((source == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "source"))) {
> + if (!source &&
> + xmlStrEqual(cur->name, BAD_CAST "source")) {
>
> if (def->type == VIR_DOMAIN_FS_TYPE_MOUNT)
> source = virXMLPropString(cur, "dir");
> @@ -4110,12 +4109,12 @@ virDomainFSDefParseXML(xmlNodePtr node,
> source = virXMLPropString(cur, "dev");
> else if (def->type == VIR_DOMAIN_FS_TYPE_TEMPLATE)
> source = virXMLPropString(cur, "name");
> - } else if ((target == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "target"))) {
> + } else if (!target &&
> + xmlStrEqual(cur->name, BAD_CAST "target")) {
> target = virXMLPropString(cur, "dir");
> } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
> def->readonly = 1;
> - } else if ((fsdriver == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
> + } else if (!fsdriver && xmlStrEqual(cur->name, BAD_CAST "driver")) {
> fsdriver = virXMLPropString(cur, "type");
> wrpolicy = virXMLPropString(cur, "wrpolicy");
> }
> @@ -4261,8 +4260,8 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> */
> addrtype = virXPathString("string(./source/address/@type)", ctxt);
> /* if not explicitly stated, source/vendor implies usb device */
> - if ((!addrtype) && virXPathNode("./source/vendor", ctxt) &&
> - ((addrtype = strdup("usb")) == NULL)) {
> + if (!addrtype && virXPathNode("./source/vendor", ctxt) &&
> + (addrtype = strdup("usb")) == NULL) {
> virReportOOMError();
> goto error;
> }
> @@ -4361,61 +4360,60 @@ virDomainNetDefParseXML(virCapsPtr caps,
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE) {
> - if ((macaddr == NULL) &&
> - (xmlStrEqual(cur->name, BAD_CAST "mac"))) {
> + if (!macaddr && xmlStrEqual(cur->name, BAD_CAST "mac")) {
> macaddr = virXMLPropString(cur, "address");
> - } else if ((network == NULL) &&
> - (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) &&
Now *this* particular kind of extra parentheses I (for some reason)
don't really have a problem with.
[etc. etc.]
I may have skipped over something when my eyes glazed over (:-)), but
this does appear correct, so ACK.
I have one reservation, though - this is a lot of code churn, which
increases the likelyhood of merge conflicts when backporting anything
from future code to any recently rebased distro packages (unless you
plan to backport this patch prior to any other backports). I guess it
all comes down to whether you think the advantages of cleaner looking
code outweighs potential time spent resolving extra conflicts (of
course, if you think it's unlikely any bugfixes will need to be
backported to any of this code, then that's a non-problem).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list