[libvirt] [PATCH 0/5] Get rid of "no_memory" labels
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()". As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them. The two exceptions are: - phyp code, as libvirt may end up dropping this code entirely; - virfirewall.c code, as it seems we heavily really on firewall->err being set to ENOMEM; If one thinks that virfirewall.c should also be converted, please, shout out and I'll work on that. Fabiano Fidêncio (5): conf: Get rid of "no_memory" labels openvz: Get rid of "no_memory" labels rpc: Get rid of "no_memory" labels util: Get rid of "no_memory" labels vbox: Get rid of "no_memory" labels src/conf/capabilities.c | 16 ++-------- src/conf/domain_audit.c | 40 +++++++------------------ src/openvz/openvz_conf.c | 18 +++++------ src/rpc/virnetclient.c | 42 ++++++++------------------ src/util/virsysinfo.c | 64 +++++++++++++++------------------------- src/util/virsysinfo.h | 2 ++ src/util/viruri.c | 28 +++++++----------- src/vbox/vbox_common.c | 20 +++++-------- 8 files changed, 75 insertions(+), 155 deletions(-) -- 2.24.1
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()". As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/conf/capabilities.c | 16 +++------------- src/conf/domain_audit.c | 40 ++++++++++------------------------------ 2 files changed, 13 insertions(+), 43 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index da54591c11..82129feaac 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -619,26 +619,16 @@ virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel, const char *type, const char *label) { - char *t = NULL, *l = NULL; - if (type == NULL || label == NULL) return -1; - t = g_strdup(type); - l = g_strdup(label); - if (VIR_EXPAND_N(secmodel->labels, secmodel->nlabels, 1) < 0) - goto no_memory; + return -1; - secmodel->labels[secmodel->nlabels - 1].type = t; - secmodel->labels[secmodel->nlabels - 1].label = l; + secmodel->labels[secmodel->nlabels - 1].type = g_strdup(type); + secmodel->labels[secmodel->nlabels - 1].label = g_strdup(label); return 0; - - no_memory: - VIR_FREE(l); - VIR_FREE(t); - return -1; } diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index a55dcd5f91..fdccc585fb 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -89,12 +89,12 @@ virDomainAuditGenericDev(virDomainObjPtr vm, const char *reason, bool success) { - char *newdev = NULL; - char *olddev = NULL; + g_autofree char *newdev = NULL; + g_autofree char *olddev = NULL; + g_autofree char *vmname = NULL; + g_autofree char *oldsrc = NULL; + g_autofree char *newsrc = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *vmname = NULL; - char *oldsrc = NULL; - char *newsrc = NULL; const char *virt = virDomainAuditGetVirtType(vm->def); /* if both new and old source aren't provided don't log anything */ @@ -107,29 +107,17 @@ virDomainAuditGenericDev(virDomainObjPtr vm, virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) - goto no_memory; + return; if (!(newsrc = virAuditEncode(newdev, VIR_AUDIT_STR(newsrcpath)))) - goto no_memory; + return; if (!(oldsrc = virAuditEncode(olddev, VIR_AUDIT_STR(oldsrcpath)))) - goto no_memory; + return; VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, "virt=%s resrc=%s reason=%s %s uuid=%s %s %s", virt, type, reason, vmname, uuidstr, oldsrc, newsrc); - - cleanup: - VIR_FREE(newdev); - VIR_FREE(olddev); - VIR_FREE(vmname); - VIR_FREE(oldsrc); - VIR_FREE(newsrc); - return; - - no_memory: - VIR_WARN("OOM while encoding audit message"); - goto cleanup; } @@ -957,13 +945,13 @@ virDomainAuditInput(virDomainObjPtr vm, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *vmname; + g_autofree char *vmname = NULL; const char *virt = virDomainAuditGetVirtType(vm->def); virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) - goto no_memory; + return; switch ((virDomainInputType) input->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -980,12 +968,4 @@ virDomainAuditInput(virDomainObjPtr vm, case VIR_DOMAIN_INPUT_TYPE_LAST: break; } - - cleanup: - VIR_FREE(vmname); - return; - - no_memory: - VIR_WARN("OOM while encoding audit message"); - goto cleanup; } -- 2.24.1
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()". As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/openvz/openvz_conf.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 971adaf71c..c4c6dec2f7 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -148,18 +148,18 @@ openvzParseBarrierLimit(const char* value, virCapsPtr openvzCapsInit(void) { - virCapsPtr caps; + g_autoptr(virCaps) caps = NULL; virCapsGuestPtr guest; if ((caps = virCapabilitiesNew(virArchFromHost(), false, false)) == NULL) - goto no_memory; + return NULL; if (!(caps->host.numa = virCapabilitiesHostNUMANewHost())) - goto no_memory; + return NULL; if (virCapabilitiesInitCaches(caps) < 0) - goto no_memory; + return NULL; if ((guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_EXE, @@ -168,7 +168,7 @@ virCapsPtr openvzCapsInit(void) NULL, 0, NULL)) == NULL) - goto no_memory; + return NULL; if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_OPENVZ, @@ -176,13 +176,9 @@ virCapsPtr openvzCapsInit(void) NULL, 0, NULL) == NULL) - goto no_memory; + return NULL; - return caps; - - no_memory: - virObjectUnref(caps); - return NULL; + return g_steal_pointer(&caps); } -- 2.24.1
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()". As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 42 ++++++++++++------------------------------ 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index eba8b865d1..50489b754c 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -439,7 +439,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, virURIPtr uri) { virNetSocketPtr sock = NULL; - virNetClientPtr ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; g_autofree char *nc = NULL; @@ -457,7 +456,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, confdir = virGetUserConfigDirectory(); virBufferAsprintf(&buf, "%s/known_hosts", confdir); if (!(knownhosts = virBufferContentAndReset(&buf))) - goto no_memory; + return NULL; } if (privkeyPath) { @@ -465,7 +464,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, } else { homedir = virGetUserDirectory(); if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0) - goto no_memory; + return NULL; } if (!authMethods) { @@ -483,11 +482,11 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, virBufferEscapeShell(&buf, netcatPath); if (!(nc = virBufferContentAndReset(&buf))) - goto no_memory; + return NULL; virBufferEscapeShell(&buf, nc); VIR_FREE(nc); if (!(nc = virBufferContentAndReset(&buf))) - goto no_memory; + return NULL; virBufferAsprintf(&buf, "sh -c " @@ -500,24 +499,16 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, nc, nc, socketPath); if (!(command = virBufferContentAndReset(&buf))) - goto no_memory; + return NULL; if (virNetSocketNewConnectLibSSH2(host, port, family, username, privkey, knownhosts, knownHostsVerify, authMethods, command, authPtr, uri, &sock) != 0) - goto cleanup; - - if (!(ret = virNetClientNew(sock, NULL))) - goto cleanup; - - cleanup: - return ret; + return NULL; - no_memory: - virReportOOMError(); - goto cleanup; + return virNetClientNew(sock, NULL); } #undef DEFAULT_VALUE @@ -538,7 +529,6 @@ virNetClientPtr virNetClientNewLibssh(const char *host, virURIPtr uri) { virNetSocketPtr sock = NULL; - virNetClientPtr ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; g_autofree char *nc = NULL; @@ -562,7 +552,7 @@ virNetClientPtr virNetClientNewLibssh(const char *host, } else { homedir = virGetUserDirectory(); if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0) - goto no_memory; + return NULL; } if (!authMethods) { @@ -580,11 +570,11 @@ virNetClientPtr virNetClientNewLibssh(const char *host, virBufferEscapeShell(&buf, netcatPath); if (!(nc = virBufferContentAndReset(&buf))) - goto no_memory; + return NULL; virBufferEscapeShell(&buf, nc); VIR_FREE(nc); if (!(nc = virBufferContentAndReset(&buf))) - goto no_memory; + return NULL; command = g_strdup_printf("sh -c " "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " @@ -596,17 +586,9 @@ virNetClientPtr virNetClientNewLibssh(const char *host, username, privkey, knownhosts, knownHostsVerify, authMethods, command, authPtr, uri, &sock) != 0) - goto cleanup; - - if (!(ret = virNetClientNew(sock, NULL))) - goto cleanup; - - cleanup: - return ret; + return NULL; - no_memory: - virReportOOMError(); - goto cleanup; + return virNetClientNew(sock, NULL); } #undef DEFAULT_VALUE -- 2.24.1
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()". As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them. Mind that virfirewall.c was not touched and still contains no_memory labels. The reason those are left behind, at least for now, is because the conversion seems to be slightly more complicated than the rest, as some other places are relying on firewall->err being set to ENOMEM. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virsysinfo.c | 64 ++++++++++++++++--------------------------- src/util/virsysinfo.h | 2 ++ src/util/viruri.c | 28 +++++++------------ 3 files changed, 35 insertions(+), 59 deletions(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 8132ab7a07..e5af4f25e0 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -302,33 +302,27 @@ virSysinfoParsePPCProcessor(const char *base, virSysinfoDefPtr ret) virSysinfoDefPtr virSysinfoReadPPC(void) { - virSysinfoDefPtr ret = NULL; - char *outbuf = NULL; + g_auto(virSysinfoDefPtr) ret = NULL; + g_autofree char *outbuf = NULL; if (VIR_ALLOC(ret) < 0) - goto no_memory; + return NULL; if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); - goto no_memory; + return NULL; } ret->nprocessor = 0; ret->processor = NULL; if (virSysinfoParsePPCProcessor(outbuf, ret) < 0) - goto no_memory; + return NULL; if (virSysinfoParsePPCSystem(outbuf, &ret->system) < 0) - goto no_memory; - - VIR_FREE(outbuf); - return ret; + return NULL; - no_memory: - VIR_FREE(outbuf); - virSysinfoDefFree(ret); - return NULL; + return g_steal_pointer(&ret); } @@ -433,13 +427,13 @@ virSysinfoParseARMProcessor(const char *base, virSysinfoDefPtr ret) virSysinfoDefPtr virSysinfoReadARM(void) { - virSysinfoDefPtr ret = NULL; - char *outbuf = NULL; + g_auto(virSysinfoDefPtr) ret = NULL; + g_autofree char *outbuf = NULL; /* Some ARM systems have DMI tables available. */ if ((ret = virSysinfoReadDMI())) { if (!virSysinfoDefIsEmpty(ret)) - return ret; + return g_steal_pointer(&ret); virSysinfoDefFree(ret); } @@ -447,29 +441,23 @@ virSysinfoReadARM(void) virResetLastError(); if (VIR_ALLOC(ret) < 0) - goto no_memory; + return NULL; if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); - goto no_memory; + return NULL; } ret->nprocessor = 0; ret->processor = NULL; if (virSysinfoParseARMProcessor(outbuf, ret) < 0) - goto no_memory; + return NULL; if (virSysinfoParseARMSystem(outbuf, &ret->system) < 0) - goto no_memory; - - VIR_FREE(outbuf); - return ret; + return NULL; - no_memory: - VIR_FREE(outbuf); - virSysinfoDefFree(ret); - return NULL; + return g_steal_pointer(&ret); } static char * @@ -606,21 +594,21 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret) virSysinfoDefPtr virSysinfoReadS390(void) { - virSysinfoDefPtr ret = NULL; - char *outbuf = NULL; + g_auto(virSysinfoDefPtr) ret = NULL; + g_autofree char *outbuf = NULL; if (VIR_ALLOC(ret) < 0) - goto no_memory; + return NULL; /* Gather info from /proc/cpuinfo */ if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); - goto no_memory; + return NULL; } if (virSysinfoParseS390Processor(outbuf, ret) < 0) - goto no_memory; + return NULL; /* Free buffer before reading next file */ VIR_FREE(outbuf); @@ -629,19 +617,13 @@ virSysinfoReadS390(void) if (virFileReadAll(SYSINFO, 8192, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), SYSINFO); - goto no_memory; + return NULL; } if (virSysinfoParseS390System(outbuf, &ret->system) < 0) - goto no_memory; - - VIR_FREE(outbuf); - return ret; + return NULL; - no_memory: - virSysinfoDefFree(ret); - VIR_FREE(outbuf); - return NULL; + return g_steal_pointer(&ret); } diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h index f30809294b..3ce936205c 100644 --- a/src/util/virsysinfo.h +++ b/src/util/virsysinfo.h @@ -144,6 +144,8 @@ void virSysinfoChassisDefFree(virSysinfoChassisDefPtr def); void virSysinfoOEMStringsDefFree(virSysinfoOEMStringsDefPtr def); void virSysinfoDefFree(virSysinfoDefPtr def); +G_DEFINE_AUTO_CLEANUP_FREE_FUNC(virSysinfoDefPtr, virSysinfoDefFree, NULL); + int virSysinfoFormat(virBufferPtr buf, virSysinfoDefPtr def) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/util/viruri.c b/src/util/viruri.c index 11163a2b40..1b848bd336 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -71,7 +71,8 @@ virURIParseParams(virURIPtr uri) return 0; while (*query) { - char *name = NULL, *value = NULL; + g_autofree char *name = NULL; + g_autofree char *value = NULL; /* Find the next separator, or end of the string. */ end = strchr(query, '&'); @@ -92,13 +93,15 @@ virURIParseParams(virURIPtr uri) * and consistent with CGI.pm we assume value is "". */ name = xmlURIUnescapeString(query, end - query, NULL); - if (!name) goto no_memory; + if (!name) + return -1; } else if (eq+1 == end) { /* Or if we have "name=" here (works around annoying * problem when calling xmlURIUnescapeString with len = 0). */ name = xmlURIUnescapeString(query, eq - query, NULL); - if (!name) goto no_memory; + if (!name) + return -1; } else if (query == eq) { /* If the '=' character is at the beginning then we have * "=value" and consistent with CGI.pm we _ignore_ this. @@ -108,22 +111,15 @@ virURIParseParams(virURIPtr uri) /* Otherwise it's "name=value". */ name = xmlURIUnescapeString(query, eq - query, NULL); if (!name) - goto no_memory; + return -1; value = xmlURIUnescapeString(eq+1, end - (eq+1), NULL); - if (!value) { - VIR_FREE(name); - goto no_memory; - } + if (!value) + return -1; } /* Append to the parameter set. */ - if (virURIParamAppend(uri, name, NULLSTR_EMPTY(value)) < 0) { - VIR_FREE(name); - VIR_FREE(value); + if (virURIParamAppend(uri, name, NULLSTR_EMPTY(value)) < 0) return -1; - } - VIR_FREE(name); - VIR_FREE(value); next: query = end; @@ -131,10 +127,6 @@ virURIParseParams(virURIPtr uri) } return 0; - - no_memory: - virReportOOMError(); - return -1; } /** -- 2.24.1
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()". As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_common.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 4493fe8582..fc67b716da 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -69,18 +69,18 @@ static virDomainDefParserConfig vboxDomainDefParserConfig = { static virCapsPtr vboxCapsInit(void) { - virCapsPtr caps; - virCapsGuestPtr guest; + g_autoptr(virCaps) caps = NULL; + virCapsGuestPtr guest = NULL; if ((caps = virCapabilitiesNew(virArchFromHost(), false, false)) == NULL) - goto no_memory; + return NULL; if (!(caps->host.numa = virCapabilitiesHostNUMANewHost())) - goto no_memory; + return NULL; if (virCapabilitiesInitCaches(caps) < 0) - goto no_memory; + return NULL; if ((guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, @@ -89,7 +89,7 @@ vboxCapsInit(void) NULL, 0, NULL)) == NULL) - goto no_memory; + return NULL; if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_VBOX, @@ -97,13 +97,9 @@ vboxCapsInit(void) NULL, 0, NULL) == NULL) - goto no_memory; - - return caps; + return NULL; - no_memory: - virObjectUnref(caps); - return NULL; + return g_steal_pointer(&caps); } static void -- 2.24.1
On 12/20/19 7:43 AM, Fabiano Fidêncio wrote:
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()".
As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them.
Series: Reviewed-by: Cole Robinson <crobinso@redhat.com>
The two exceptions are: - phyp code, as libvirt may end up dropping this code entirely; - virfirewall.c code, as it seems we heavily really on firewall->err being set to ENOMEM;
I looked at it a bit. It can probably all be ripped out but it's a little convoluted. virCommand seems to have some similar ENOMEM handling as well. I think a nice prep step that will simplify this style of cleanups, is to drop the return value from the VIR_*LLOC* macros. After the glib conversion, they always return 0, or abort. But everywhere in the code is still checking for 'if (VIR_ALLOC(foo) < 0)' and similar. Long term we should replace that with g_new0 but it's not a drop in replacement. An easy intermediate step we can do is entirely drop the '< 0' checking. This will removal a lot of 'if' conditionals that would need to be tweaked if we work on dropping cleanup: labels now. It could be mass done per directory and outside of a few cases I think they would all be trivial. I think after a step like that, there would be many util/ functions that never return error, which is another thing to unwind up the chain. - Cole
On Fri, Dec 20, 2019 at 03:29:42PM -0500, Cole Robinson wrote:
On 12/20/19 7:43 AM, Fabiano Fidêncio wrote:
As pointed out by Ján Tomko, "no_memory seems suspicious in the times of abort()".
As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of the no_memory labels and simplify the code around them.
Series: Reviewed-by: Cole Robinson <crobinso@redhat.com>
The two exceptions are: - phyp code, as libvirt may end up dropping this code entirely; - virfirewall.c code, as it seems we heavily really on firewall->err being set to ENOMEM;
I looked at it a bit. It can probably all be ripped out but it's a little convoluted. virCommand seems to have some similar ENOMEM handling as well.
I think a nice prep step that will simplify this style of cleanups, is to drop the return value from the VIR_*LLOC* macros. After the glib conversion, they always return 0, or abort. But everywhere in the code is still checking for 'if (VIR_ALLOC(foo) < 0)' and similar.
Long term we should replace that with g_new0 but it's not a drop in replacement. An easy intermediate step we can do is entirely drop the '< 0' checking. This will removal a lot of 'if' conditionals that would need to be tweaked if we work on dropping cleanup: labels now. It could be mass done per directory and outside of a few cases I think they would all be trivial.
When we first introduced the use of glib we declared that we should *not* simply remove the '< 0' from VIR_ALLOC statements, because this will double the churn in the code base by making it a 2 step conversion. We want to go straight to g_new0, which is why we kept the "ATTRIBUTE_RETURN_CHECK" annotation on VIR_ALLOC & friends. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
Fabiano Fidêncio