On 09/11/2014 07:43 AM, Ján Tomko wrote:
Instead of checking upfront if the <driver> element will be needed in a big condition, just format all the attributes into a string and output the <driver> element if the string is not empty. --- src/conf/domain_conf.c | 68 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 24 deletions(-)
ACK I left some thoughts below, but I think I convinced myself that this was the way to go - just figured I'd keep the thoughts there though. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a2a7d92..fcf7fb6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16401,6 +16401,41 @@ virDomainActualNetDefFormat(virBufferPtr buf, return 0; }
+ +static int +virDomainVirtioNetDriverFormat(char **outstr, + virDomainNetDefPtr def)
Could perhaps have been: static char * virDomainVirtioNetDriverFormat(virDomainNetDefPtr def)
+{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + if (def->driver.virtio.name) { + virBufferAsprintf(&buf, "name='%s' ", + virDomainNetBackendTypeToString(def->driver.virtio.name)); + } + if (def->driver.virtio.txmode) { + virBufferAsprintf(&buf, "txmode='%s' ", + virDomainNetVirtioTxModeTypeToString(def->driver.virtio.txmode)); + } + if (def->driver.virtio.ioeventfd) { + virBufferAsprintf(&buf, "ioeventfd='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.ioeventfd)); + } + if (def->driver.virtio.event_idx) { + virBufferAsprintf(&buf, "event_idx='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.event_idx)); + } + if (def->driver.virtio.queues) + virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues); + + virBufferTrim(&buf, " ", -1); + + if (virBufferCheckError(&buf) < 0) + return -1;
So an 'error' here is more than likely a memory one (similar to old code), but the only difference here is you're checking and failing because of that; whereas, the old code didn't fail right away... Or at least until the similar error checking was done on the buffer. Hmm. so I guess this is why you took the approach of passing a string address to return your string buffer into. This will just cause a bit faster failure (more than likely) from the
+ + *outstr = virBufferContentAndReset(&buf);
and return virBufferContentAndReset(&buf);
+ return 0; +} + + int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, @@ -16576,30 +16611,15 @@ virDomainNetDefFormat(virBufferPtr buf, if (def->model) { virBufferEscapeString(buf, "<model type='%s'/>\n", def->model); - if (STREQ(def->model, "virtio") && - (def->driver.virtio.name || def->driver.virtio.txmode || - def->driver.virtio.ioeventfd || def->driver.virtio.event_idx || - def->driver.virtio.queues)) { - virBufferAddLit(buf, "<driver"); - if (def->driver.virtio.name) { - virBufferAsprintf(buf, " name='%s'", - virDomainNetBackendTypeToString(def->driver.virtio.name)); - } - if (def->driver.virtio.txmode) { - virBufferAsprintf(buf, " txmode='%s'", - virDomainNetVirtioTxModeTypeToString(def->driver.virtio.txmode)); - } - if (def->driver.virtio.ioeventfd) { - virBufferAsprintf(buf, " ioeventfd='%s'", - virTristateSwitchTypeToString(def->driver.virtio.ioeventfd)); - } - if (def->driver.virtio.event_idx) { - virBufferAsprintf(buf, " event_idx='%s'", - virTristateSwitchTypeToString(def->driver.virtio.event_idx)); - } - if (def->driver.virtio.queues) - virBufferAsprintf(buf, " queues='%u'", def->driver.virtio.queues); - virBufferAddLit(buf, "/>\n"); + if (STREQ(def->model, "virtio")) { + char *str; + + if (virDomainVirtioNetDriverFormat(&str, def) < 0) + return -1; + + if (str) + virBufferAsprintf(buf, "<driver %s/>\n", str);
Of course if you went with return char * option, it'd be if ((str = virDomainVirtioNetDriverFormat(def))) virBufferAsprintf(buf, "<driver %s/>\n", str);
+ VIR_FREE(str); } } if (def->filter) {