5:55 a.m.
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) {