Re: [libvirt] [PATCH 1/5] conf: split out virtio net driver formatting

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);