[libvirt] [PATCH v2 2/5] domain: replace bool accel{2d, 3d} with a tristate
Michal Privoznik
mprivozn at redhat.com
Fri Nov 27 13:10:16 UTC 2015
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 at 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
More information about the libvir-list
mailing list