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) {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list