On 25.11.2015 09:42, Marc-André Lureau wrote:
Allowing to have the extra undefined/default state.
Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com> --- src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 4 ++-- src/vbox/vbox_common.c | 18 ++++++++++++------ 3 files changed, 40 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f501f14..111c2ae 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11961,8 +11961,9 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) { xmlNodePtr cur; virDomainVideoAccelDefPtr def; - char *accel3d = NULL; char *accel2d = NULL; + char *accel3d = NULL; + int val;
This @val variable is not much useful, since def->accel{2,3}d is already type of int, but doesn't hurt to have it here too. I bet compiler will optimize it out anyway. And I don't want to be too picky too.
cur = node->children; while (cur != NULL) { @@ -11983,21 +11984,26 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) return NULL;
There's another bug in here, just above this return NULL ^^^ there are two allocations, if the first succeeds but the second fails, we leak the first allocation. So this return NULL should be replaced with goto cleanup.
if (accel3d) { - if (STREQ(accel3d, "yes")) - def->accel3d = true; - else - def->accel3d = false; - VIR_FREE(accel3d); + if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown accel3d value '%s'"), accel3d); + goto end; + } + def->accel3d = val; }
if (accel2d) { - if (STREQ(accel2d, "yes")) - def->accel2d = true; - else - def->accel2d = false; - VIR_FREE(accel2d); + if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown accel2d value '%s'"), accel2d); + goto end; + } + def->accel2d = val; }
+end:
s/^/ / it's our coding style. Then, we tend to use cleanup as a name for labels rather than end.
+ VIR_FREE(accel2d); + VIR_FREE(accel3d); return def; }
@@ -20837,10 +20843,15 @@ static void virDomainVideoAccelDefFormat(virBufferPtr buf, virDomainVideoAccelDefPtr def) { - virBufferAsprintf(buf, "<acceleration accel3d='%s'", - def->accel3d ? "yes" : "no"); - virBufferAsprintf(buf, " accel2d='%s'", - def->accel2d ? "yes" : "no"); + virBufferAsprintf(buf, "<acceleration");
For adding literal strings to a buffer we have a special function: virBufferAddLit().
+ if (def->accel3d) { + virBufferAsprintf(buf, " accel3d='%s'", + virTristateBoolTypeToString(def->accel3d)); + } + if (def->accel2d) { + virBufferAsprintf(buf, " accel2d='%s'", + virTristateBoolTypeToString(def->accel2d)); + } virBufferAddLit(buf, "/>\n"); }
Right, I went through all occurrences of accel{2,3}d and you are fixing all the places needed. ACK with this squashed in: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e095115..e04227a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12027,13 +12027,13 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) return NULL; if (VIR_ALLOC(def) < 0) - return NULL; + goto cleanup; if (accel3d) { if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel3d value '%s'"), accel3d); - goto end; + goto cleanup; } def->accel3d = val; } @@ -12042,12 +12042,12 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown accel2d value '%s'"), accel2d); - goto end; + goto cleanup; } def->accel2d = val; } -end: + cleanup: VIR_FREE(accel2d); VIR_FREE(accel3d); return def; @@ -20879,7 +20879,7 @@ static void virDomainVideoAccelDefFormat(virBufferPtr buf, virDomainVideoAccelDefPtr def) { - virBufferAsprintf(buf, "<acceleration"); + virBufferAddLit(buf, "<acceleration"); if (def->accel3d) { virBufferAsprintf(buf, " accel3d='%s'", virTristateBoolTypeToString(def->accel3d)); Michal