[libvirt PATCH 00/10] Cleanup some cleanup / error paths
Tim Wiederhake (10): adminConnectListServers: Cleanup virCHDomainObjBeginJob: Cleanup virDomainCapsCPUModelsCopy: Cleanup virNetworkEventDispatchDefaultFunc: Cleanup virSaveCookieParse: Cleanup virBufferAddBuffer: Cleanup virSCSIVHostOpenVhostSCSI: Cleanup fillXenCaps: Cleanup testLXCCapsInit: Cleanup testVshTableHeader: Cleanup src/admin/admin_server.c | 4 ++-- src/ch/ch_domain.c | 34 ++++++++++++++++------------------ src/conf/domain_capabilities.c | 10 +++------- src/conf/network_event.c | 9 +++------ src/conf/virsavecookie.c | 9 ++------- src/util/virbuffer.c | 10 ++++------ src/util/virscsivhost.c | 7 +------ tests/domaincapstest.c | 18 +++++------------- tests/testutilslxc.c | 17 +++++------------ tests/vshtabletest.c | 12 +++++------- 10 files changed, 46 insertions(+), 84 deletions(-) -- 2.31.1
Remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/admin/admin_server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 623c682b2d..6731a366cf 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -47,12 +47,12 @@ adminConnectListServers(virNetDaemon *dmn, virCheckFlags(0, -1); if ((ret = virNetDaemonGetServers(dmn, &srvs)) < 0) - goto cleanup; + return ret; if (servers) { *servers = g_steal_pointer(&srvs); } - cleanup: + if (ret > 0) virObjectListFreeCount(srvs, ret); return ret; -- 2.31.1
Remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/ch/ch_domain.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c index 9d32d8669a..dd4de9e1ea 100644 --- a/src/ch/ch_domain.c +++ b/src/ch/ch_domain.c @@ -87,8 +87,22 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum virCHDomainJob job) while (priv->job.active) { VIR_DEBUG("Wait normal job condition for starting job: %s", virCHDomainJobTypeToString(job)); - if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) - goto error; + if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) { + VIR_WARN("Cannot start job (%s) for domain %s;" + " current job is (%s) owned by (%d)", + virCHDomainJobTypeToString(job), + obj->def->name, + virCHDomainJobTypeToString(priv->job.active), + priv->job.owner); + + if (errno == ETIMEDOUT) + virReportError(VIR_ERR_OPERATION_TIMEOUT, + "%s", _("cannot acquire state change lock")); + else + virReportSystemError(errno, + "%s", _("cannot acquire job mutex")); + return -1; + } } virCHDomainObjResetJob(priv); @@ -98,22 +112,6 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum virCHDomainJob job) priv->job.owner = virThreadSelfID(); return 0; - - error: - VIR_WARN("Cannot start job (%s) for domain %s;" - " current job is (%s) owned by (%d)", - virCHDomainJobTypeToString(job), - obj->def->name, - virCHDomainJobTypeToString(priv->job.active), - priv->job.owner); - - if (errno == ETIMEDOUT) - virReportError(VIR_ERR_OPERATION_TIMEOUT, - "%s", _("cannot acquire state change lock")); - else - virReportSystemError(errno, - "%s", _("cannot acquire job mutex")); - return -1; } /* -- 2.31.1
Remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_capabilities.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 22f0963326..1766129092 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -157,7 +157,7 @@ virDomainCapsCPUModelsNew(size_t nmodels) virDomainCapsCPUModels * virDomainCapsCPUModelsCopy(virDomainCapsCPUModels *old) { - virDomainCapsCPUModels *cpuModels; + g_autoptr(virDomainCapsCPUModels) cpuModels = NULL; size_t i; if (!(cpuModels = virDomainCapsCPUModelsNew(old->nmodels))) @@ -169,14 +169,10 @@ virDomainCapsCPUModelsCopy(virDomainCapsCPUModels *old) old->models[i].usable, old->models[i].blockers, old->models[i].deprecated) < 0) - goto error; + return NULL; } - return cpuModels; - - error: - virObjectUnref(cpuModels); - return NULL; + return g_steal_pointer(&cpuModels); } -- 2.31.1
Remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/network_event.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/conf/network_event.c b/src/conf/network_event.c index a47bf4dd3e..6335cbf711 100644 --- a/src/conf/network_event.c +++ b/src/conf/network_event.c @@ -86,8 +86,8 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn, virConnectObjectEventGenericCallback cb, void *cbopaque) { - virNetworkPtr net = virGetNetwork(conn, event->meta.name, event->meta.uuid); - if (!net) + g_autoptr(virNetwork) net = NULL; + if (!(net = virGetNetwork(conn, event->meta.name, event->meta.uuid))) return; switch ((virNetworkEventID)event->eventID) { @@ -100,16 +100,13 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn, networkLifecycleEvent->type, networkLifecycleEvent->detail, cbopaque); - goto cleanup; + return; } case VIR_NETWORK_EVENT_ID_LAST: break; } VIR_WARN("Unexpected event ID %d", event->eventID); - - cleanup: - virObjectUnref(net); } -- 2.31.1
On 11/8/21 1:17 PM, Tim Wiederhake wrote:
Remove unnecessary label and goto.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/network_event.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/conf/network_event.c b/src/conf/network_event.c index a47bf4dd3e..6335cbf711 100644 --- a/src/conf/network_event.c +++ b/src/conf/network_event.c @@ -86,8 +86,8 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn, virConnectObjectEventGenericCallback cb, void *cbopaque) { - virNetworkPtr net = virGetNetwork(conn, event->meta.name, event->meta.uuid); - if (!net) + g_autoptr(virNetwork) net = NULL; + if (!(net = virGetNetwork(conn, event->meta.name, event->meta.uuid)))
I think we can use this opportunity to put an empty line in between declaration and code blocks.
return;
switch ((virNetworkEventID)event->eventID) { @@ -100,16 +100,13 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn, networkLifecycleEvent->type, networkLifecycleEvent->detail, cbopaque); - goto cleanup; + return; }
case VIR_NETWORK_EVENT_ID_LAST: break; } VIR_WARN("Unexpected event ID %d", event->eventID); - - cleanup: - virObjectUnref(net); }
Michal
Remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virsavecookie.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/conf/virsavecookie.c b/src/conf/virsavecookie.c index 6cb7fafb1f..c24a292355 100644 --- a/src/conf/virsavecookie.c +++ b/src/conf/virsavecookie.c @@ -58,19 +58,14 @@ virSaveCookieParse(xmlXPathContextPtr ctxt, virSaveCookieCallbacks *saveCookie) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - int ret = -1; *obj = NULL; if (!(ctxt->node = virXPathNode("./cookie", ctxt))) { - ret = 0; - goto cleanup; + return 0; } - ret = virSaveCookieParseNode(ctxt, obj, saveCookie); - - cleanup: - return ret; + return virSaveCookieParseNode(ctxt, obj, saveCookie); } -- 2.31.1
Remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virbuffer.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 8f9cd57e06..a4834174a1 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -185,13 +185,11 @@ virBufferAddBuffer(virBuffer *buf, virBuffer *toadd) if (!toadd || !toadd->str) return; - if (!buf) - goto cleanup; - - virBufferInitialize(buf); - g_string_append_len(buf->str, toadd->str->str, toadd->str->len); + if (buf) { + virBufferInitialize(buf); + g_string_append_len(buf->str, toadd->str->str, toadd->str->len); + } - cleanup: virBufferFreeAndReset(toadd); } -- 2.31.1
Remove unnecessary label, goto, and closing of not-open file descriptor. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virscsivhost.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index afbfddb0fb..487301ab64 100644 --- a/src/util/virscsivhost.c +++ b/src/util/virscsivhost.c @@ -90,15 +90,10 @@ virSCSIVHostOpenVhostSCSI(int *vhostfd) if (*vhostfd < 0) { virReportSystemError(errno, _("Failed to open %s"), VHOST_SCSI_DEVICE); - goto error; + return -1; } return 0; - - error: - VIR_FORCE_CLOSE(*vhostfd); - - return -1; } -- 2.31.1
Rework to remove unnecessary label and goto. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/domaincapstest.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 9ea5bed5c2..4a46acb9ad 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -135,25 +135,17 @@ fillQemuCaps(virDomainCaps *domCaps, static int fillXenCaps(virDomainCaps *domCaps) { - virFirmware **firmwares; - int ret = -1; - - firmwares = g_new0(virFirmware *, 2); - - firmwares[0] = g_new0(virFirmware, 1); - firmwares[1] = g_new0(virFirmware, 1); + g_autoptr(virFirmware) fw_hvmloader = g_new0(virFirmware, 1); + g_autoptr(virFirmware) fw_ovmf = g_new0(virFirmware, 1); + virFirmware *firmwares[] = { fw_hvmloader, fw_ovmf }; firmwares[0]->name = g_strdup("/usr/lib/xen/boot/hvmloader"); firmwares[1]->name = g_strdup("/usr/lib/xen/boot/ovmf.bin"); if (libxlMakeDomainCapabilities(domCaps, firmwares, 2) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virFirmwareFreeList(firmwares, 2); - return ret; + return 0; } #endif /* WITH_LIBXL */ -- 2.31.1
Remove unnecessary label and goto. Cleanup line breaks. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/testutilslxc.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/testutilslxc.c b/tests/testutilslxc.c index 857407dfb2..ac7a01a4e8 100644 --- a/tests/testutilslxc.c +++ b/tests/testutilslxc.c @@ -11,11 +11,10 @@ virCaps * testLXCCapsInit(void) { - virCaps *caps; + g_autoptr(virCaps) caps = NULL; virCapsGuest *guest; - if ((caps = virCapabilitiesNew(VIR_ARCH_X86_64, - false, false)) == NULL) + if ((caps = virCapabilitiesNew(VIR_ARCH_X86_64, false, false)) == NULL) return NULL; guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_EXE, @@ -33,20 +32,14 @@ testLXCCapsInit(void) virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_LXC, NULL, NULL, 0, NULL); if (virTestGetDebug()) { - g_autofree char *caps_str = NULL; - - caps_str = virCapabilitiesFormatXML(caps); + g_autofree char *caps_str = virCapabilitiesFormatXML(caps); if (!caps_str) - goto error; + return NULL; VIR_TEST_DEBUG("LXC driver capabilities:\n%s", caps_str); } - return caps; - - error: - virObjectUnref(caps); - return NULL; + return g_steal_pointer(&caps); } -- 2.31.1
Remove unnecessary label and goto. This also fixes a bug where a failure to create the table would result in the test passing. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/vshtabletest.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c index 716b11dbc0..41ceec0a51 100644 --- a/tests/vshtabletest.c +++ b/tests/vshtabletest.c @@ -45,7 +45,8 @@ static int testVshTableHeader(const void *opaque G_GNUC_UNUSED) { int ret = 0; - char *act = NULL; + g_autofree char *act = NULL; + g_autofree char *act2 = NULL; const char *exp = " 1 fedora28 running\n" " 2 rhel7.5 running\n"; @@ -58,7 +59,7 @@ testVshTableHeader(const void *opaque G_GNUC_UNUSED) g_autoptr(vshTable) table = vshTableNew("Id", "Name", "State", NULL); //to ask about return if (!table) - goto cleanup; + return -1; vshTableRowAppend(table, "1", "fedora28", "running", NULL); vshTableRowAppend(table, "2", "rhel7.5", "running", @@ -68,13 +69,10 @@ testVshTableHeader(const void *opaque G_GNUC_UNUSED) if (virTestCompareToString(exp, act) < 0) ret = -1; - VIR_FREE(act); - act = vshTablePrintToString(table, true); - if (virTestCompareToString(exp2, act) < 0) + act2 = vshTablePrintToString(table, true); + if (virTestCompareToString(exp2, act2) < 0) ret = -1; - cleanup: - VIR_FREE(act); return ret; } -- 2.31.1
On 11/8/21 1:17 PM, Tim Wiederhake wrote:
Tim Wiederhake (10): adminConnectListServers: Cleanup virCHDomainObjBeginJob: Cleanup virDomainCapsCPUModelsCopy: Cleanup virNetworkEventDispatchDefaultFunc: Cleanup virSaveCookieParse: Cleanup virBufferAddBuffer: Cleanup virSCSIVHostOpenVhostSCSI: Cleanup fillXenCaps: Cleanup testLXCCapsInit: Cleanup testVshTableHeader: Cleanup
src/admin/admin_server.c | 4 ++-- src/ch/ch_domain.c | 34 ++++++++++++++++------------------ src/conf/domain_capabilities.c | 10 +++------- src/conf/network_event.c | 9 +++------ src/conf/virsavecookie.c | 9 ++------- src/util/virbuffer.c | 10 ++++------ src/util/virscsivhost.c | 7 +------ tests/domaincapstest.c | 18 +++++------------- tests/testutilslxc.c | 17 +++++------------ tests/vshtabletest.c | 12 +++++------- 10 files changed, 46 insertions(+), 84 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake