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

[John Ferlan]

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