Re: [libvirt] [PATCH 06/18] domain_conf: cleanup error paths for graphics parser

On Mon, Apr 04, 2016 at 03:20:23PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 100 ++++++++++++++++++++++--------------------------- 1 file changed, 45 insertions(+), 55 deletions(-)

@@ -10846,17 +10845,19 @@ virDomainGraphicsDefParseXMLSdl(virDomainGraphicsDefPtr def, } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown fullscreen value '%s'"), fullscreen); - VIR_FREE(fullscreen); - return -1; + goto error; } - VIR_FREE(fullscreen); } else { def->data.sdl.fullscreen = false; } + def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display");

- return 0; + ret = 0; + error:

If you're adding a new label, 'cleanup' is better for paths shared by the success and the error paths.

+ VIR_FREE(fullscreen); + return ret; }

@@ -10866,52 +10867,44 @@ virDomainGraphicsDefParseXMLRdp(virDomainGraphicsDefPtr def, }

- if ((autoport = virXMLPropString(node, "autoport")) != NULL) { - if (STREQ(autoport, "yes")) - def->data.rdp.autoport = true; - - VIR_FREE(autoport); - } + if (autoport && STREQ(autoport, "yes")) + def->data.rdp.autoport = true;

Could be STREQ_NULLABLE.

if (def->data.rdp.autoport && (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) def->data.rdp.port = 0;

- if ((replaceUser = virXMLPropString(node, "replaceUser")) != NULL) { - if (STREQ(replaceUser, "yes")) - def->data.rdp.replaceUser = true; - VIR_FREE(replaceUser); - } + if (replaceUser && STREQ(replaceUser, "yes")) + def->data.rdp.replaceUser = true;

- if ((multiUser = virXMLPropString(node, "multiUser")) != NULL) { - if (STREQ(multiUser, "yes")) - def->data.rdp.multiUser = true; - VIR_FREE(multiUser); - } + if (multiUser && STREQ(multiUser, "yes")) + def->data.rdp.multiUser = true;

ret = 0; error: + VIR_FREE(port); + VIR_FREE(autoport); + VIR_FREE(replaceUser); + VIR_FREE(multiUser); return ret; }

@@ -10930,16 +10924,18 @@ virDomainGraphicsDefParseXMLDesktop(virDomainGraphicsDefPtr def, } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown fullscreen value '%s'"), fullscreen); - VIR_FREE(fullscreen); - return -1; + goto error; } - VIR_FREE(fullscreen); } else { def->data.desktop.fullscreen = false; }

def->data.desktop.display = virXMLPropString(node, "display"); - return 0; + + ret = 0; + error:

s/error/cleanup/

+ VIR_FREE(fullscreen); + return ret; }

ACK Jan