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

[Ján Tomko]

On 09/15/2014 11:55 PM, John Ferlan wrote:
> 
> 
> 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
> 

I wanted to get rid of the giant condition, failing sooner is just a side effect.

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

We wouldn't know if str is NULL because the driver has no special options, or
if it's because we were out of memory. In the unlikely event of
virDomainVirtioNetDriverFormat getting OOM error and the other virBuffer*
calls succeeding, the <driver> element would not get formatted, but we
wouldn't report an error either.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140916/78dc71ab/attachment-0001.sig>