[PATCH 00/33] Remove the now misleading virReportOOMError infra
'virReportOOMError' is nowadays mostly useless since an OOM error will trigger yet another allocation failure in the logger when attempting to log the error. Additionally it was also used in few places which can fail with other failures than OOM. To prevent both errorneous usage types, let's remove it completely. In the rare case we'd want to allocate a massive buffer and want to fail gracefully we still can use virReportError with VIR_ERR_NO_MEMORY. The series starts by some cleanup, then removes error handling from places where it's now dead code as we'll abort before the reporting, then converts some real OOM scenarios to abort() directly and lastly fixes the error message for cases where other errors are possible and lastly removes virReportOOMError. Pipelines are stuck, so hopefully it will go through: https://gitlab.com/pipo.sk/libvirt/-/pipelines/261161989 Peter Krempa (33): Remove useless comments for VIR_FROM_THIS definition util: xml: Introduce autoptr cleanup support for 'xmlNode' virDomainDefSetMetadata: Avoid temporary variable for string copy virCommandAddEnv: Make stealing of argument more obvious virCommandAddArgBuffer: Simplify clearing of @buf virCPUx86DataParse: Don't check error from x86FeatureNames virhostcputest: linuxCPUStatsCompareFiles: Don't check return value of virBufferContentAndReset virBuildPath: Remove return value lxc_process: Remove OOM handling from logging setup virDomainDefSetMetadata: Rework memory handling util: vircommand: Remove OOM handling virCloseCallbacksGetForConn: Remove OOM handling virfirewall: Don't check OOM in ADD_ARG macro virfirewall: Remove OOM checks from virFirewallStartTransaction virfirewall: virFirewallAddRuleFullV: Remove OOM check from VIR_APPEND_ELEMENT virfirewall: Remove impossible OOM error reporting util: virnetlink: Add wrapper for 'nlmsg_alloc_simple' util: xml: Add virXMLBufferCreate wrapper util: xml: Add wrapper for 'xmlNewNode' Don't report OOM error on xmlCopyNode failure virXMLXPathContextNew: abort() on allocation failure virXMLParseHelper: abort() on allocation failure util: virprocess: abort() on CPU_ALLOC failure virURIFormat: abort() on failure util: iohelper: Don't handle OOM from posix_memalign hyperv: abort() failure of wsmc_fault_new() vbox: abort() on allocation failure in UTF8<->UTF16 conversion libxl: abort() on failure of libxl_cpu_bitmap_alloc() virVBoxSnapshotConfSaveVboxFile: abort() on failure to allocate xmlDoc and comment util: json: Report non-OOM error on yajl failure storage: Don't report OOM error on failure of glfs_new virVMXConvertToUTF8: Report non-OOM error on failure of xmlBufferCreateStatic util: virerror: Remove virReportOOMError build-aux/syntax-check.mk | 8 -- docs/internals/command.html.in | 8 +- src/conf/domain_conf.c | 62 ++++----- src/conf/network_conf.c | 5 +- src/cpu/cpu_x86.c | 6 +- src/hyperv/hyperv_wmi.c | 8 +- src/hypervisor/virclosecallbacks.c | 15 +-- src/libvirt_private.syms | 3 +- src/libxl/libxl_conf.c | 6 +- src/libxl/libxl_driver.c | 7 +- src/lxc/lxc_process.c | 9 +- src/storage/storage_backend_gluster.c | 3 +- .../storage_file_backend_gluster.c | 3 +- src/test/test_driver.c | 6 +- src/util/iohelper.c | 7 +- src/util/vircommand.c | 119 +++++------------- src/util/virerror.c | 22 ---- src/util/virerror.h | 8 -- src/util/virfcp.c | 3 +- src/util/virfile.c | 7 +- src/util/virfile.h | 2 +- src/util/virfirewall.c | 62 ++------- src/util/virhook.c | 14 +-- src/util/virjson.c | 6 +- src/util/virnetdev.c | 18 +-- src/util/virnetdevbridge.c | 10 +- src/util/virnetdevip.c | 15 +-- src/util/virnetdevvportprofile.c | 6 +- src/util/virnetlink.c | 38 +++--- src/util/virnetlink.h | 4 + src/util/virpci.c | 12 +- src/util/virprocess.c | 12 +- src/util/virscsi.c | 1 - src/util/virscsivhost.c | 1 - src/util/viruri.c | 8 +- src/util/virxml.c | 44 ++++--- src/util/virxml.h | 8 ++ src/vbox/vbox_common.c | 20 --- src/vbox/vbox_common.h | 15 ++- src/vbox/vbox_snapshot_conf.c | 46 ++----- src/vmx/vmx.c | 9 +- tests/virhostcputest.c | 5 +- tools/virsh-domain.c | 5 +- 43 files changed, 214 insertions(+), 462 deletions(-) -- 2.29.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpci.c | 1 - src/util/virscsi.c | 1 - src/util/virscsivhost.c | 1 - 3 files changed, 3 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 8147ce11e9..3df4199532 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -103,7 +103,6 @@ struct _virPCIDeviceList { }; -/* For virReportOOMError() and virReportSystemError() */ #define VIR_FROM_THIS VIR_FROM_NONE /* Specifications referenced in comments: diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 2a9f6da76a..d0ba33e254 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -37,7 +37,6 @@ #define SYSFS_SCSI_DEVICES "/sys/bus/scsi/devices" -/* For virReportOOMError() and virReportSystemError() */ #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("util.scsi"); diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index b7bf0da1b3..a0dfb8021a 100644 --- a/src/util/virscsivhost.c +++ b/src/util/virscsivhost.c @@ -28,7 +28,6 @@ #include "virstring.h" #include "viralloc.h" -/* For virReportOOMError() and virReportSystemError() */ #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("util.scsihost"); -- 2.29.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virxml.h b/src/util/virxml.h index f73b8975ba..e696dd25f5 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -255,6 +255,7 @@ G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virXPathContextNodeSave, virXPathContextNodeRes G_DEFINE_AUTOPTR_CLEANUP_FUNC(xmlDoc, xmlFreeDoc); G_DEFINE_AUTOPTR_CLEANUP_FUNC(xmlXPathContext, xmlXPathFreeContext); G_DEFINE_AUTOPTR_CLEANUP_FUNC(xmlBuffer, xmlBufferFree); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(xmlNode, xmlFreeNode); typedef int (*virXMLNamespaceParse)(xmlXPathContextPtr ctxt, void **nsdata); typedef void (*virXMLNamespaceFree)(void *nsdata); -- 2.29.2
Since error checking was removed when switching to g_strdup, it doesn't make much sense to have 'tmp' around. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b731744f04..6f4487fcfc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30426,7 +30426,6 @@ virDomainDefSetMetadata(virDomainDefPtr def, xmlDocPtr doc = NULL; xmlNodePtr old; xmlNodePtr new = NULL; - char *tmp = NULL; int ret = -1; if (type >= VIR_DOMAIN_METADATA_LAST) { @@ -30437,19 +30436,17 @@ virDomainDefSetMetadata(virDomainDefPtr def, switch ((virDomainMetadataType) type) { case VIR_DOMAIN_METADATA_DESCRIPTION: - if (STRNEQ_NULLABLE(metadata, "")) - tmp = g_strdup(metadata); + g_clear_pointer(&def->description, g_free); - VIR_FREE(def->description); - def->description = tmp; + if (STRNEQ_NULLABLE(metadata, "")) + def->description = g_strdup(metadata); break; case VIR_DOMAIN_METADATA_TITLE: - if (STRNEQ_NULLABLE(metadata, "")) - tmp = g_strdup(metadata); + g_clear_pointer(&def->title, g_free); - VIR_FREE(def->title); - def->title = tmp; + if (STRNEQ_NULLABLE(metadata, "")) + def->title = g_strdup(metadata); break; case VIR_DOMAIN_METADATA_ELEMENT: -- 2.29.2
The function is supposed to always consume the passed environment variable string. Use a temp variable with autofree and g_steal_pointer to prevent having to free it manually. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 323f841b98..b94ab615d5 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1325,8 +1325,10 @@ virCommandRawStatus(virCommandPtr cmd) * already set, then it is replaced in the list. */ static void -virCommandAddEnv(virCommandPtr cmd, char *env) +virCommandAddEnv(virCommandPtr cmd, + char *envstr) { + g_autofree char *env = envstr; size_t namelen; size_t i; @@ -1336,19 +1338,18 @@ virCommandAddEnv(virCommandPtr cmd, char *env) /* + 1 because we want to match the '=' character too. */ if (STREQLEN(cmd->env[i], env, namelen + 1)) { VIR_FREE(cmd->env[i]); - cmd->env[i] = env; + cmd->env[i] = g_steal_pointer(&env); return; } } /* Arg plus trailing NULL. */ if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { - VIR_FREE(env); cmd->has_error = ENOMEM; return; } - cmd->env[cmd->nenv++] = env; + cmd->env[cmd->nenv++] = g_steal_pointer(&env); } /** -- 2.29.2
Get the buffer contents into a temporary variable with automatic clearing so that the error branches don't have to reset the buffer. Additionally handle the NULL string case before assignment. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index b94ab615d5..f11caf0d6e 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1550,21 +1550,21 @@ virCommandAddArg(virCommandPtr cmd, const char *val) void virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf) { - if (!cmd || cmd->has_error) { - virBufferFreeAndReset(buf); + g_autofree char *str = virBufferContentAndReset(buf); + + if (!cmd || cmd->has_error) return; - } + + if (!str) + str = g_strdup(""); /* Arg plus trailing NULL. */ if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { cmd->has_error = ENOMEM; - virBufferFreeAndReset(buf); return; } - cmd->args[cmd->nargs] = virBufferContentAndReset(buf); - if (!cmd->args[cmd->nargs]) - cmd->args[cmd->nargs] = g_strdup(""); + cmd->args[cmd->nargs] = g_steal_pointer(&str); cmd->nargs++; } -- 2.29.2
x86FeatureNames uses virBuffer and thus can't fail nowadays. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/cpu/cpu_x86.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 92f945beb4..79c5868ae6 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1856,11 +1856,7 @@ virCPUx86DataParse(xmlXPathContextPtr ctxt) */ #define virX86CpuIncompatible(MSG, CPU_DEF) \ do { \ - g_autofree char *flagsStr = NULL; \ - if (!(flagsStr = x86FeatureNames(map, ", ", (CPU_DEF)))) { \ - virReportOOMError(); \ - return VIR_CPU_COMPARE_ERROR; \ - } \ + g_autofree char *flagsStr = x86FeatureNames(map, ", ", (CPU_DEF)); \ if (message) \ *message = g_strdup_printf("%s: %s", _(MSG), flagsStr); \ VIR_DEBUG("%s: %s", MSG, flagsStr); \ -- 2.29.2
The buffer won't encounter OOM condition nowadays Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virhostcputest.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c index 786a363e48..2608f0becc 100644 --- a/tests/virhostcputest.c +++ b/tests/virhostcputest.c @@ -141,10 +141,7 @@ linuxCPUStatsCompareFiles(const char *cpustatfile, goto fail; } - if (!(actualData = virBufferContentAndReset(&buf))) { - virReportOOMError(); - goto fail; - } + actualData = virBufferContentAndReset(&buf); if (virTestCompareToFile(actualData, outfile) < 0) goto fail; -- 2.29.2
The function can't fail nowadays, remove the return value and adjust callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/internals/command.html.in | 8 +------- src/util/virfcp.c | 3 +-- src/util/virfile.c | 7 +------ src/util/virfile.h | 2 +- src/util/virhook.c | 14 ++------------ src/util/virpci.c | 11 ++--------- 6 files changed, 8 insertions(+), 37 deletions(-) diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index 823d89cc71..634afdc937 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -568,13 +568,7 @@ int runhook(const char *drvstr, const char *id, char *path; virCommandPtr cmd; - ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); - if ((ret < 0) || (path == NULL)) { - virHookReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to build path for %s hook"), - drvstr); - return -1; - } + virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); cmd = virCommandNew(path); VIR_FREE(path); diff --git a/src/util/virfcp.c b/src/util/virfcp.c index 80773c7c5d..5e8fe72fec 100644 --- a/src/util/virfcp.c +++ b/src/util/virfcp.c @@ -40,8 +40,7 @@ virFCIsCapableRport(const char *rport) { g_autofree char *path = NULL; - if (virBuildPath(&path, SYSFS_FC_RPORT_PATH, rport) < 0) - return false; + virBuildPath(&path, SYSFS_FC_RPORT_PATH, rport); return virFileExists(path); } diff --git a/src/util/virfile.c b/src/util/virfile.c index 5710495bbf..27a647d1d0 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1308,13 +1308,12 @@ virFileFindMountPoint(const char *type G_GNUC_UNUSED) #endif /* defined WITH_MNTENT_H && defined WITH_GETMNTENT_R */ -int +void virBuildPathInternal(char **path, ...) { char *path_component = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; va_list ap; - int ret = 0; va_start(ap, path); @@ -1329,10 +1328,6 @@ virBuildPathInternal(char **path, ...) va_end(ap); *path = virBufferContentAndReset(&buf); - if (*path == NULL) - ret = -1; - - return ret; } /* Read no more than the specified maximum number of bytes. */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 733d652ac9..f237e98290 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -300,7 +300,7 @@ char *virFileFindMountPoint(const char *type); /* NB: this should be combined with virFileBuildPath */ #define virBuildPath(path, ...) \ virBuildPathInternal(path, __VA_ARGS__, NULL) -int virBuildPathInternal(char **path, ...) G_GNUC_NULL_TERMINATED; +void virBuildPathInternal(char **path, ...) G_GNUC_NULL_TERMINATED; typedef struct _virHugeTLBFS virHugeTLBFS; typedef virHugeTLBFS *virHugeTLBFSPtr; diff --git a/src/util/virhook.c b/src/util/virhook.c index e4e1945225..b52e2cd814 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -155,12 +155,7 @@ virHookCheck(int no, const char *driver) return -1; } - if (virBuildPath(&path, LIBVIRT_HOOK_DIR, driver) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to build path for %s hook"), - driver); - return -1; - } + virBuildPath(&path, LIBVIRT_HOOK_DIR, driver); if (!virFileExists(path)) { VIR_DEBUG("No hook script %s", path); @@ -405,12 +400,7 @@ virHookCall(int driver, if (extra == NULL) extra = "-"; - if (virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to build path for %s hook"), - drvstr); - return -1; - } + virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); script_ret = 1; diff --git a/src/util/virpci.c b/src/util/virpci.c index 3df4199532..515b642db4 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2293,10 +2293,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, *pf = NULL; - if (virBuildPath(&device_link, vf_sysfs_path, "physfn") == -1) { - virReportOOMError(); - return -1; - } + virBuildPath(&device_link, vf_sysfs_path, "physfn"); if ((*pf = virPCIGetDeviceAddressFromSysfsLink(device_link))) { VIR_DEBUG("PF for VF device '%s': " VIR_PCI_DEVICE_ADDRESS_FMT, @@ -2470,11 +2467,7 @@ virPCIGetNetName(const char *device_link_sysfs_path, *netname = NULL; - if (virBuildPath(&pcidev_sysfs_net_path, device_link_sysfs_path, - "net") == -1) { - virReportOOMError(); - return -1; - } + virBuildPath(&pcidev_sysfs_net_path, device_link_sysfs_path, "net"); if (virDirOpenQuiet(&dir, pcidev_sysfs_net_path) < 0) { /* this *isn't* an error - caller needs to check for netname == NULL */ -- 2.29.2
'virLogGetFilters' doesn't return failure and 'virLogGetOutputs' reports it's own errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index cbc04a3dcd..679709605e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -960,21 +960,14 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, if (virLogGetNbFilters() > 0) { filterstr = virLogGetFilters(); - if (!filterstr) { - virReportOOMError(); - goto error; - } virCommandAddEnvPair(cmd, "LIBVIRT_LOG_FILTERS", filterstr); } if (cfg->log_libvirtd) { if (virLogGetNbOutputs() > 0) { - outputstr = virLogGetOutputs(); - if (!outputstr) { - virReportOOMError(); + if (!(outputstr = virLogGetOutputs())) goto error; - } virCommandAddEnvPair(cmd, "LIBVIRT_LOG_OUTPUTS", outputstr); } -- 2.29.2
Switch to use g_autoptr for 'doc' and 'new' local variables. Additionally report proper error when 'xmlAddChild' fails because OOM is not the only error it can report. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6f4487fcfc..46620d38ed 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30423,15 +30423,14 @@ virDomainDefSetMetadata(virDomainDefPtr def, const char *key, const char *uri) { - xmlDocPtr doc = NULL; + g_autoptr(xmlDoc) doc = NULL; xmlNodePtr old; - xmlNodePtr new = NULL; - int ret = -1; + g_autoptr(xmlNode) new = NULL; if (type >= VIR_DOMAIN_METADATA_LAST) { virReportError(VIR_ERR_INVALID_ARG, _("unknown metadata type '%d'"), type); - goto cleanup; + return -1; } switch ((virDomainMetadataType) type) { @@ -30451,23 +30450,24 @@ virDomainDefSetMetadata(virDomainDefPtr def, case VIR_DOMAIN_METADATA_ELEMENT: if (metadata) { + /* parse and modify the xml from the user */ if (!(doc = virXMLParseString(metadata, _("(metadata_xml)")))) - goto cleanup; + return -1; if (virXMLInjectNamespace(doc->children, uri, key) < 0) - goto cleanup; + return -1; /* create the root node if needed */ if (!def->metadata && !(def->metadata = xmlNewNode(NULL, (unsigned char *)"metadata"))) { virReportOOMError(); - goto cleanup; + return -1; } if (!(new = xmlCopyNode(doc->children, 1))) { virReportOOMError(); - goto cleanup; + return -1; } } @@ -30477,11 +30477,13 @@ virDomainDefSetMetadata(virDomainDefPtr def, xmlFreeNode(old); } - if (new && - !(xmlAddChild(def->metadata, new))) { - xmlFreeNode(new); - virReportOOMError(); - goto cleanup; + if (new) { + if (!(xmlAddChild(def->metadata, new))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to add metadata to XML document")); + return -1; + } + new = NULL; } break; @@ -30490,11 +30492,7 @@ virDomainDefSetMetadata(virDomainDefPtr def, break; } - ret = 0; - - cleanup: - xmlFreeDoc(doc); - return ret; + return 0; } -- 2.29.2
The OOM error handling is dead code nowadays. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 96 +++++++++---------------------------------- 1 file changed, 20 insertions(+), 76 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index f11caf0d6e..1a4b77ea24 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -90,7 +90,7 @@ struct _virCommandSendBuffer { }; struct _virCommand { - int has_error; /* ENOMEM on allocation failure, -1 for anything else. */ + int has_error; /* 0 on success, -1 on error */ char **args; size_t nargs; @@ -198,7 +198,6 @@ virCommandFDIsSet(virCommandPtr cmd, * * Returns: 0 on success, * -1 on usage error, - * ENOMEM on OOM */ static int virCommandFDSet(virCommandPtr cmd, @@ -211,8 +210,7 @@ virCommandFDSet(virCommandPtr cmd, if (virCommandFDIsSet(cmd, fd)) return 0; - if (VIR_EXPAND_N(cmd->passfd, cmd->npassfd, 1) < 0) - return ENOMEM; + ignore_value(VIR_EXPAND_N(cmd->passfd, cmd->npassfd, 1)); cmd->passfd[cmd->npassfd - 1].fd = fd; cmd->passfd[cmd->npassfd - 1].flags = flags; @@ -1344,10 +1342,7 @@ virCommandAddEnv(virCommandPtr cmd, } /* Arg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1)); cmd->env[cmd->nenv++] = g_steal_pointer(&env); } @@ -1474,10 +1469,7 @@ virCommandAddEnvPassCommon(virCommandPtr cmd) if (!cmd || cmd->has_error) return; - if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); virCommandAddEnvPair(cmd, "LC_ALL", "C"); @@ -1497,10 +1489,7 @@ virCommandAddEnvXDG(virCommandPtr cmd, const char *baseDir) if (!cmd || cmd->has_error) return; - if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 3) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 3)); virCommandAddEnvFormat(cmd, "XDG_DATA_HOME=%s/%s", baseDir, ".local/share"); @@ -1530,10 +1519,7 @@ virCommandAddArg(virCommandPtr cmd, const char *val) } /* Arg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1)); cmd->args[cmd->nargs++] = g_strdup(val); } @@ -1559,10 +1545,7 @@ virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf) str = g_strdup(""); /* Arg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1)); cmd->args[cmd->nargs] = g_steal_pointer(&str); cmd->nargs++; @@ -1591,11 +1574,7 @@ virCommandAddArgFormat(virCommandPtr cmd, const char *format, ...) va_end(list); /* Arg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { - VIR_FREE(arg); - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1)); cmd->args[cmd->nargs++] = arg; } @@ -1642,10 +1621,7 @@ virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) narg++; /* narg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1)); narg = 0; while (vals[narg] != NULL) { @@ -1678,10 +1654,7 @@ virCommandAddArgList(virCommandPtr cmd, ...) va_end(list); /* narg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1) < 0) { - cmd->has_error = ENOMEM; - return; - } + ignore_value(VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1)); va_start(list, cmd); while (1) { @@ -1765,10 +1738,7 @@ virCommandSetSendBuffer(virCommandPtr cmd, } i = virCommandGetNumSendBuffers(cmd); - if (VIR_REALLOC_N(cmd->sendBuffers, i + 1) < 0) { - cmd->has_error = ENOMEM; - return -1; - } + ignore_value(VIR_REALLOC_N(cmd->sendBuffers, i + 1)); cmd->sendBuffers[i].fd = fd; cmd->sendBuffers[i].buffer = buffer; @@ -2099,11 +2069,7 @@ virCommandToString(virCommandPtr cmd, bool linebreaks) /* Cannot assume virCommandRun will be called; so report the error * now. If virCommandRun is called, it will report the same error. */ - if (!cmd ||cmd->has_error == ENOMEM) { - virReportOOMError(); - return NULL; - } - if (cmd->has_error) { + if (!cmd || cmd->has_error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return NULL; @@ -2339,11 +2305,7 @@ virCommandProcessIO(virCommandPtr cmd) */ int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) { - if (!cmd ||cmd->has_error == ENOMEM) { - virReportOOMError(); - return -1; - } - if (cmd->has_error) { + if (!cmd || cmd->has_error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2388,11 +2350,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) char *str; int tmpfd; - if (!cmd ||cmd->has_error == ENOMEM) { - virReportOOMError(); - return -1; - } - if (cmd->has_error) { + if (!cmd || cmd->has_error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2539,11 +2497,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) bool synchronous = false; int infd[2] = {-1, -1}; - if (!cmd || cmd->has_error == ENOMEM) { - virReportOOMError(); - return -1; - } - if (cmd->has_error) { + if (!cmd || cmd->has_error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2692,11 +2646,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) int ret; int status = 0; - if (!cmd ||cmd->has_error == ENOMEM) { - virReportOOMError(); - return -1; - } - if (cmd->has_error) { + if (!cmd || cmd->has_error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2835,11 +2785,8 @@ int virCommandHandshakeWait(virCommandPtr cmd) { char c; int rv; - if (!cmd ||cmd->has_error == ENOMEM) { - virReportOOMError(); - return -1; - } - if (cmd->has_error || !cmd->handshake) { + + if (!cmd || cmd->has_error || !cmd->handshake) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2896,11 +2843,8 @@ int virCommandHandshakeWait(virCommandPtr cmd) int virCommandHandshakeNotify(virCommandPtr cmd) { char c = '1'; - if (!cmd ||cmd->has_error == ENOMEM) { - virReportOOMError(); - return -1; - } - if (cmd->has_error || !cmd->handshake) { + + if (!cmd || cmd->has_error || !cmd->handshake) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; -- 2.29.2
VIR_EXPAND_N will abort so we can simplify the hash iterator. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/hypervisor/virclosecallbacks.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/hypervisor/virclosecallbacks.c b/src/hypervisor/virclosecallbacks.c index 2641f45a22..1fd4dd7adf 100644 --- a/src/hypervisor/virclosecallbacks.c +++ b/src/hypervisor/virclosecallbacks.c @@ -241,7 +241,6 @@ struct _virCloseCallbacksList { struct virCloseCallbacksData { virConnectPtr conn; virCloseCallbacksListPtr list; - bool oom; }; static int @@ -263,11 +262,7 @@ virCloseCallbacksGetOne(void *payload, if (data->conn != closeDef->conn || !closeDef->cb) return 0; - if (VIR_EXPAND_N(data->list->entries, - data->list->nentries, 1) < 0) { - data->oom = true; - return 0; - } + ignore_value(VIR_EXPAND_N(data->list->entries, data->list->nentries, 1)); memcpy(data->list->entries[data->list->nentries - 1].uuid, uuid, VIR_UUID_BUFLEN); @@ -286,17 +281,9 @@ virCloseCallbacksGetForConn(virCloseCallbacksPtr closeCallbacks, data.conn = conn; data.list = list; - data.oom = false; virHashForEach(closeCallbacks->list, virCloseCallbacksGetOne, &data); - if (data.oom) { - VIR_FREE(list->entries); - VIR_FREE(list); - virReportOOMError(); - return NULL; - } - return list; } -- 2.29.2
VIR_RESIZE_N can't fail nowadays, adjust the macro. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfirewall.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 6b04a8011f..95962568f5 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -262,10 +262,9 @@ void virFirewallFree(virFirewallPtr firewall) #define ADD_ARG(rule, str) \ do { \ - if (VIR_RESIZE_N(rule->args, \ - rule->argsAlloc, \ - rule->argsLen, 1) < 0) \ - goto no_memory; \ + ignore_value(VIR_RESIZE_N(rule->args, \ + rule->argsAlloc, \ + rule->argsLen, 1)); \ \ rule->args[rule->argsLen++] = g_strdup(str); \ } while (0) @@ -433,9 +432,6 @@ void virFirewallRuleAddArg(virFirewallPtr firewall, ADD_ARG(rule, arg); return; - - no_memory: - firewall->err = ENOMEM; } @@ -455,9 +451,6 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, ADD_ARG(rule, arg); return; - - no_memory: - firewall->err = ENOMEM; } @@ -473,9 +466,6 @@ void virFirewallRuleAddArgSet(virFirewallPtr firewall, } return; - - no_memory: - firewall->err = ENOMEM; } @@ -496,10 +486,6 @@ void virFirewallRuleAddArgList(virFirewallPtr firewall, va_end(list); return; - - no_memory: - firewall->err = ENOMEM; - va_end(list); } -- 2.29.2
Neither virFirewallGroupNew nor VIR_EXPAND_N can fail. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfirewall.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 95962568f5..66b33d4a91 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -517,18 +517,11 @@ void virFirewallStartTransaction(virFirewallPtr firewall, VIR_FIREWALL_RETURN_IF_ERROR(firewall); - if (!(group = virFirewallGroupNew())) { - firewall->err = ENOMEM; - return; - } + group = virFirewallGroupNew(); group->actionFlags = flags; - if (VIR_EXPAND_N(firewall->groups, - firewall->ngroups, 1) < 0) { - firewall->err = ENOMEM; - virFirewallGroupFree(group); - return; - } + ignore_value(VIR_EXPAND_N(firewall->groups, + firewall->ngroups, 1)); firewall->groups[firewall->ngroups - 1] = group; firewall->currentGroup = firewall->ngroups - 1; } -- 2.29.2
VIR_APPEND_ELEMENT_COPY will abort the program on OOM so there's no need to check. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfirewall.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 66b33d4a91..bbeb87e72d 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -315,24 +315,17 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, ADD_ARG(rule, str); if (group->addingRollback) { - if (VIR_APPEND_ELEMENT_COPY(group->rollback, - group->nrollback, - rule) < 0) - goto no_memory; + ignore_value(VIR_APPEND_ELEMENT_COPY(group->rollback, + group->nrollback, + rule)); } else { - if (VIR_APPEND_ELEMENT_COPY(group->action, - group->naction, - rule) < 0) - goto no_memory; + ignore_value(VIR_APPEND_ELEMENT_COPY(group->action, + group->naction, + rule)); } return rule; - - no_memory: - firewall->err = ENOMEM; - virFirewallRuleFree(rule); - return NULL; } -- 2.29.2
There's nothing that would set the 'err' field of virFirewallPtr to ENOMEM so we can remove the checks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfirewall.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bbeb87e72d..c1b7d2268b 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -698,10 +698,6 @@ virFirewallApplyRule(virFirewallPtr firewall, if (rule->queryCB(firewall, rule->layer, (const char *const *)lines, rule->queryOpaque) < 0) return -1; - if (firewall->err == ENOMEM) { - virReportOOMError(); - return -1; - } if (firewall->err) { virReportSystemError(firewall->err, "%s", _("Unable to create rule")); @@ -769,11 +765,7 @@ virFirewallApply(virFirewallPtr firewall) _("Failed to initialize a valid firewall backend")); goto cleanup; } - if (!firewall || firewall->err == ENOMEM) { - virReportOOMError(); - goto cleanup; - } - if (firewall->err) { + if (!firewall || firewall->err) { virReportSystemError(firewall->err, "%s", _("Unable to create rule")); goto cleanup; -- 2.29.2
On a Wednesday in 2021, Peter Krempa wrote:
There's nothing that would set the 'err' field of virFirewallPtr to ENOMEM so we can remove the checks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfirewall.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bbeb87e72d..c1b7d2268b 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -698,10 +698,6 @@ virFirewallApplyRule(virFirewallPtr firewall, if (rule->queryCB(firewall, rule->layer, (const char *const *)lines, rule->queryOpaque) < 0) return -1;
- if (firewall->err == ENOMEM) { - virReportOOMError(); - return -1; - } if (firewall->err) { virReportSystemError(firewall->err, "%s", _("Unable to create rule")); @@ -769,11 +765,7 @@ virFirewallApply(virFirewallPtr firewall) _("Failed to initialize a valid firewall backend")); goto cleanup; } - if (!firewall || firewall->err == ENOMEM) { - virReportOOMError(); - goto cleanup; - } - if (firewall->err) { + if (!firewall || firewall->err) { virReportSystemError(firewall->err, "%s",
Coverity complains about a possible NULL dereference here if firewall == NULL. Jano
_("Unable to create rule")); goto cleanup; -- 2.29.2
The function is used in many places and fails only on allocation failures. Since trying to recover from allocation failure of a small buffer by reporting error doesn't make sense add a wrapper for 'nlmsg_alloc_simple' which will 'abort()' on failure and replace all allocations of netlink message with the new helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virnetdev.c | 18 +++------------ src/util/virnetdevbridge.c | 10 +++------ src/util/virnetdevip.c | 15 ++++--------- src/util/virnetdevvportprofile.c | 6 +---- src/util/virnetlink.c | 38 ++++++++++++++------------------ src/util/virnetlink.h | 4 ++++ 6 files changed, 32 insertions(+), 59 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2b4c8b6280..d0c76ce26c 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1596,11 +1596,7 @@ virNetDevSetVfConfig(const char *ifname, int vf, if (!macaddr && vlanid < 0) return -1; - nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); - if (!nl_msg) { - virReportOOMError(); - return rc; - } + nl_msg = virNetlinkMsgNew(RTM_SETLINK, NLM_F_REQUEST); if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) goto buffer_too_small; @@ -3132,11 +3128,7 @@ virNetDevGetFamilyId(const char *family_name, unsigned int recvbuflen; int ret = -1; - if (!(nl_msg = nlmsg_alloc_simple(GENL_ID_CTRL, - NLM_F_REQUEST | NLM_F_ACK))) { - virReportOOMError(); - goto cleanup; - } + nl_msg = virNetlinkMsgNew(GENL_ID_CTRL, NLM_F_REQUEST | NLM_F_ACK); if (nlmsg_append(nl_msg, &gmsgh, sizeof(gmsgh), NLMSG_ALIGNTO) < 0) goto cleanup; @@ -3220,11 +3212,7 @@ virNetDevSwitchdevFeature(const char *ifname, if ((rv = virNetDevGetFamilyId(DEVLINK_GENL_NAME, &family_id)) <= 0) return rv; - if (!(nl_msg = nlmsg_alloc_simple(family_id, - NLM_F_REQUEST | NLM_F_ACK))) { - virReportOOMError(); - goto cleanup; - } + nl_msg = virNetlinkMsgNew(family_id, NLM_F_REQUEST | NLM_F_ACK); if (nlmsg_append(nl_msg, &gmsgh, sizeof(gmsgh), NLMSG_ALIGNTO) < 0) goto cleanup; diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index d475e4c43d..7b5ea4fe1d 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -1036,13 +1036,9 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname, if (!(ndm.ndm_state & (NUD_PERMANENT | NUD_REACHABLE))) ndm.ndm_state |= NUD_PERMANENT; - nl_msg = nlmsg_alloc_simple(isAdd ? RTM_NEWNEIGH : RTM_DELNEIGH, - NLM_F_REQUEST | - (isAdd ? (NLM_F_CREATE | NLM_F_EXCL) : 0)); - if (!nl_msg) { - virReportOOMError(); - return -1; - } + nl_msg = virNetlinkMsgNew(isAdd ? RTM_NEWNEIGH : RTM_DELNEIGH, + NLM_F_REQUEST | + (isAdd ? (NLM_F_CREATE | NLM_F_EXCL) : 0)); if (nlmsg_append(nl_msg, &ndm, sizeof(ndm), NLMSG_ALIGNTO) < 0) goto buffer_too_small; diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 4eb8ef76d1..83da7bc46d 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -107,11 +107,8 @@ virNetDevCreateNetlinkAddressMessage(int messageType, if ((ifindex = if_nametoindex(ifname)) == 0) return NULL; - if (!(nlmsg = nlmsg_alloc_simple(messageType, - NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL))) { - virReportOOMError(); - return NULL; - } + nlmsg = virNetlinkMsgNew(messageType, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); memset(&ifa, 0, sizeof(ifa)); @@ -323,12 +320,8 @@ virNetDevIPRouteAdd(const char *ifname, if ((ifindex = if_nametoindex(ifname)) == 0) return -1; - if (!(nlmsg = nlmsg_alloc_simple(RTM_NEWROUTE, - NLM_F_REQUEST | NLM_F_CREATE | - NLM_F_EXCL))) { - virReportOOMError(); - return -1; - } + nlmsg = virNetlinkMsgNew(RTM_NEWROUTE, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); memset(&rtmsg, 0, sizeof(rtmsg)); diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 5d6c055b32..c0fc04c699 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -689,11 +689,7 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, ? virUUIDFormat(hostUUID, hostUUIDStr) : "(unspecified)")); - nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); - if (!nl_msg) { - virReportOOMError(); - return rc; - } + nl_msg = virNetlinkMsgNew(RTM_SETLINK, NLM_F_REQUEST); if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) goto buffer_too_small; diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 9bd7339054..a06195bd00 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -133,6 +133,18 @@ static virNetlinkHandle *placeholder_nlhandle; /* Function definitions */ +struct nl_msg * +virNetlinkMsgNew(int nlmsgtype, + int nlmsgflags) +{ + struct nl_msg *ret; + + if (!(ret = nlmsg_alloc_simple(nlmsgtype, nlmsgflags))) + abort(); + + return ret; +} + /** * virNetlinkStartup: * @@ -511,11 +523,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex, ifinfo.ifi_index = ifindex; - nl_msg = nlmsg_alloc_simple(RTM_GETLINK, NLM_F_REQUEST); - if (!nl_msg) { - virReportOOMError(); - return -1; - } + nl_msg = virNetlinkMsgNew(RTM_GETLINK, NLM_F_REQUEST); NETLINK_MSG_APPEND(nl_msg, sizeof(ifinfo), &ifinfo); @@ -595,12 +603,8 @@ virNetlinkNewLink(const char *ifname, return -1; } - nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, - NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); - if (!nl_msg) { - virReportOOMError(); - return -1; - } + nl_msg = virNetlinkMsgNew(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); NETLINK_MSG_APPEND(nl_msg, sizeof(ifinfo), &ifinfo); @@ -684,11 +688,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback) unsigned int resp_len = 0; int error = 0; - nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST); - if (!nl_msg) { - virReportOOMError(); - return -1; - } + nl_msg = virNetlinkMsgNew(RTM_DELLINK, NLM_F_REQUEST); NETLINK_MSG_APPEND(nl_msg, sizeof(ifinfo), &ifinfo); @@ -738,11 +738,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid) unsigned int resp_len = 0; int error = 0; - nl_msg = nlmsg_alloc_simple(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST); - if (!nl_msg) { - virReportOOMError(); - return -1; - } + nl_msg = virNetlinkMsgNew(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST); NETLINK_MSG_APPEND(nl_msg, sizeof(ndinfo), &ndinfo); diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index cab685feea..e05c62e635 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -29,6 +29,10 @@ typedef struct nl_msg virNetlinkMsg; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetlinkMsg, nlmsg_free); +struct nl_msg * +virNetlinkMsgNew(int nlmsgtype, + int nlmsgflags); + #else struct nl_msg; -- 2.29.2
'xmlBufferCreate' returns NULL only on allocation failure. Add a wrapper which will call 'abort()' in such case in a centralised spot. It doesn't make much sense to continue execution from here. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 5 +---- src/conf/network_conf.c | 5 +---- src/libvirt_private.syms | 1 + src/util/virxml.c | 19 +++++++++++++------ src/util/virxml.h | 3 +++ src/vmx/vmx.c | 5 ++--- 6 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 46620d38ed..7a3374b5be 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28692,10 +28692,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, * Thankfully, libxml maps what looks like globals into * thread-local uses, so we are thread-safe. */ xmlIndentTreeOutput = 1; - if (!(xmlbuf = xmlBufferCreate())) { - virReportOOMError(); - return -1; - } + xmlbuf = virXMLBufferCreate(); if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f32710b781..69d99a60e0 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2513,10 +2513,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, * Thankfully, libxml maps what looks like globals into * thread-local uses, so we are thread-safe. */ xmlIndentTreeOutput = 1; - if (!(xmlbuf = xmlBufferCreate())) { - virReportOOMError(); - return -1; - } + xmlbuf = virXMLBufferCreate(); if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6b7261b987..dd54550b60 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3529,6 +3529,7 @@ virVsockSetGuestCid; # util/virxml.h virParseScaledValue; +virXMLBufferCreate; virXMLCheckIllegalChars; virXMLExtractNamespaceXML; virXMLFormatElement; diff --git a/src/util/virxml.c b/src/util/virxml.c index 0354251941..3fed2b2a6e 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -941,12 +941,7 @@ char * virXMLNodeToString(xmlDocPtr doc, xmlNodePtr node) { - g_autoptr(xmlBuffer) xmlbuf = NULL; - - if (!(xmlbuf = xmlBufferCreate())) { - virReportOOMError(); - return NULL; - } + g_autoptr(xmlBuffer) xmlbuf = virXMLBufferCreate(); if (xmlNodeDump(xmlbuf, doc, node, 0, 1) == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1467,3 +1462,15 @@ virParseScaledValue(const char *xpath, *val = bytes; return 1; } + + +xmlBufferPtr +virXMLBufferCreate(void) +{ + xmlBufferPtr ret; + + if (!(ret = xmlBufferCreate())) + abort(); + + return ret; +} diff --git a/src/util/virxml.h b/src/util/virxml.h index e696dd25f5..24a2234506 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -286,3 +286,6 @@ int virParseScaledValue(const char *xpath, unsigned long long scale, unsigned long long max, bool required); + +xmlBufferPtr +virXMLBufferCreate(void); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index db535ba260..e6c0900a65 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -771,7 +771,7 @@ virVMXConvertToUTF8(const char *encoding, const char *string) char *result = NULL; xmlCharEncodingHandlerPtr handler; g_autoptr(xmlBuffer) input = NULL; - g_autoptr(xmlBuffer) utf8 = NULL; + g_autoptr(xmlBuffer) utf8 = virXMLBufferCreate(); handler = xmlFindCharEncodingHandler(encoding); @@ -781,8 +781,7 @@ virVMXConvertToUTF8(const char *encoding, const char *string) return NULL; } - if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) || - !(utf8 = xmlBufferCreate())) { + if (!(input = xmlBufferCreateStatic((char *)string, strlen(string)))) { virReportOOMError(); goto cleanup; } -- 2.29.2
Add a wrapper that will handle the out of memory condition by abort() and also prevents callers from having to typecast the argument. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 7 ++----- src/libvirt_private.syms | 1 + src/util/virxml.c | 13 +++++++++++++ src/util/virxml.h | 4 ++++ src/vbox/vbox_snapshot_conf.c | 34 +++++++++------------------------- tools/virsh-domain.c | 5 +---- 6 files changed, 30 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7a3374b5be..c0881608af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30456,11 +30456,8 @@ virDomainDefSetMetadata(virDomainDefPtr def, return -1; /* create the root node if needed */ - if (!def->metadata && - !(def->metadata = xmlNewNode(NULL, (unsigned char *)"metadata"))) { - virReportOOMError(); - return -1; - } + if (!def->metadata) + def->metadata = virXMLNewNode(NULL, "metadata"); if (!(new = xmlCopyNode(doc->children, 1))) { virReportOOMError(); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd54550b60..48f66daab8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3533,6 +3533,7 @@ virXMLBufferCreate; virXMLCheckIllegalChars; virXMLExtractNamespaceXML; virXMLFormatElement; +virXMLNewNode; virXMLNodeContentString; virXMLNodeNameEqual; virXMLNodeSanitizeNamespaces; diff --git a/src/util/virxml.c b/src/util/virxml.c index 3fed2b2a6e..ebe479f5d3 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1474,3 +1474,16 @@ virXMLBufferCreate(void) return ret; } + + +xmlNodePtr +virXMLNewNode(xmlNsPtr ns, + const char *name) +{ + xmlNodePtr ret; + + if (!(ret = xmlNewNode(ns, BAD_CAST name))) + abort(); + + return ret; +} diff --git a/src/util/virxml.h b/src/util/virxml.h index 24a2234506..d32f77b867 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -289,3 +289,7 @@ int virParseScaledValue(const char *xpath, xmlBufferPtr virXMLBufferCreate(void); + +xmlNodePtr +virXMLNewNode(xmlNsPtr ns, + const char *name); diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index f1cae3039a..5792d3175e 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -328,7 +328,7 @@ virVBoxSnapshotConfCreateHardDiskNode(virVBoxSnapshotConfHardDiskPtr hardDisk) int result = -1; size_t i = 0; char *uuid = NULL; - xmlNodePtr ret = xmlNewNode(NULL, BAD_CAST "HardDisk"); + xmlNodePtr ret = virXMLNewNode(NULL, "HardDisk"); uuid = g_strdup_printf("{%s}", hardDisk->uuid); if (xmlNewProp(ret, BAD_CAST "uuid", BAD_CAST uuid) == NULL) @@ -404,7 +404,7 @@ virVBoxSnapshotConfSerializeSnapshot(xmlNodePtr node, /* node description */ if (snapshot->description != NULL) { - descriptionNode = xmlNewNode(NULL, BAD_CAST "Description"); + descriptionNode = virXMLNewNode(NULL, "Description"); xmlNodeSetContent(descriptionNode, BAD_CAST snapshot->description); xmlAddChild(node, descriptionNode); } @@ -433,10 +433,10 @@ virVBoxSnapshotConfSerializeSnapshot(xmlNodePtr node, xmlAddChild(node, storageControllerNode); if (snapshot->nchildren > 0) { - snapshotsNode = xmlNewNode(NULL, BAD_CAST "Snapshots"); + snapshotsNode = virXMLNewNode(NULL, "Snapshots"); xmlAddChild(node, snapshotsNode); for (i = 0; i < snapshot->nchildren; i++) { - xmlNodePtr child = xmlNewNode(NULL, BAD_CAST "Snapshot"); + xmlNodePtr child = virXMLNewNode(NULL, "Snapshot"); xmlAddChild(snapshotsNode, child); if (virVBoxSnapshotConfSerializeSnapshot(child, snapshot->children[i]) < 0) goto cleanup; @@ -1001,11 +1001,7 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, goto cleanup; } - cur = xmlNewNode(NULL, BAD_CAST "VirtualBox"); - if (!cur) { - virReportOOMError(); - goto cleanup; - } + cur = virXMLNewNode(NULL, "VirtualBox"); if (!xmlNewProp(cur, BAD_CAST "version", BAD_CAST "1.12-linux")) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -1038,11 +1034,7 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, goto cleanup; } - machineNode = xmlNewNode(NULL, BAD_CAST "Machine"); - if (!machineNode) { - virReportOOMError(); - goto cleanup; - } + machineNode = virXMLNewNode(NULL, "Machine"); if (!xmlNewProp(machineNode, BAD_CAST "uuid", BAD_CAST machine->uuid)) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -1101,11 +1093,7 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, } xmlAddChild(xmlDocGetRootElement(xml), machineNode); - mediaRegistryNode = xmlNewNode(NULL, BAD_CAST "MediaRegistry"); - if (!mediaRegistryNode) { - virReportOOMError(); - goto cleanup; - } + mediaRegistryNode = virXMLNewNode(NULL, "MediaRegistry"); xmlAddChild(machineNode, mediaRegistryNode); for (i = 0; i < machine->mediaRegistry->notherMedia; i++) { @@ -1121,11 +1109,7 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, } xmlAddChild(mediaRegistryNode, cur); } - hardDisksNode = xmlNewNode(NULL, BAD_CAST "HardDisks"); - if (!hardDisksNode) { - virReportOOMError(); - goto cleanup; - } + hardDisksNode = virXMLNewNode(NULL, "HardDisks"); for (i = 0; i < machine->mediaRegistry->ndisks; i++) { xmlNodePtr child = virVBoxSnapshotConfCreateHardDiskNode(machine->mediaRegistry->disks[i]); if (child != NULL) @@ -1172,7 +1156,7 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, xmlAddChild(machineNode, cur); if (machine->snapshot != NULL) { - snapshotNode = xmlNewNode(NULL, BAD_CAST "Snapshot"); + snapshotNode = virXMLNewNode(NULL, "Snapshot"); xmlAddChild(machineNode, snapshotNode); if (virVBoxSnapshotConfSerializeSnapshot(snapshotNode, machine->snapshot) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index df33467646..16e0c45f80 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12566,10 +12566,7 @@ virshUpdateDiskXML(xmlNodePtr disk_node, if (new_source) { /* create new source subelement */ - if (!(source = xmlNewNode(NULL, BAD_CAST "source"))) { - vshError(NULL, _("Failed to allocate new source node")); - goto cleanup; - } + source = virXMLNewNode(NULL, "source"); if (source_block) xmlNewProp(source, BAD_CAST "dev", BAD_CAST new_source); -- 2.29.2
Out of memory isn't the only reason the function can fail. Add a message stating that copying of a XML node failed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/test/test_driver.c | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c0881608af..f1fd5320a5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30460,7 +30460,8 @@ virDomainDefSetMetadata(virDomainDefPtr def, def->metadata = virXMLNewNode(NULL, "metadata"); if (!(new = xmlCopyNode(doc->children, 1))) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to copy XML node")); return -1; } } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index bca1297d1d..71ab04aa1a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -224,7 +224,8 @@ testDomainDefNamespaceParse(xmlXPathContextPtr ctxt, for (i = 0; i < n; i++) { xmlNodePtr newnode = xmlCopyNode(nodes[i], 1); if (!newnode) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to copy XML node")); goto error; } @@ -787,7 +788,8 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type) ret = xmlCopyNode(xmlDocGetRootElement(doc), 1); if (!ret) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to copy XML node")); goto error; } xmlReplaceNode(node, ret); -- 2.29.2
On 2/24/21 11:16 AM, Peter Krempa wrote:
Out of memory isn't the only reason the function can fail. Add a message stating that copying of a XML node failed.
Could it still fail due to OOM as well, though? And if so, is there any way of differentiating? (I previously looked into this for some other libxml2 function, ended up asking DV, and I believe I was told that there isn't any way to tell the difference, but as usual IMBES (I May Be Experiencing Senility). At any rate, your changed message is more correct, since it's not making any unsubstantiated assumption about the cause of the error, while virRemoveOOMError() was).
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/test/test_driver.c | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c0881608af..f1fd5320a5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30460,7 +30460,8 @@ virDomainDefSetMetadata(virDomainDefPtr def, def->metadata = virXMLNewNode(NULL, "metadata");
if (!(new = xmlCopyNode(doc->children, 1))) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to copy XML node")); return -1; } } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index bca1297d1d..71ab04aa1a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -224,7 +224,8 @@ testDomainDefNamespaceParse(xmlXPathContextPtr ctxt, for (i = 0; i < n; i++) { xmlNodePtr newnode = xmlCopyNode(nodes[i], 1); if (!newnode) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to copy XML node")); goto error; }
@@ -787,7 +788,8 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type)
ret = xmlCopyNode(xmlDocGetRootElement(doc), 1); if (!ret) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to copy XML node")); goto error; } xmlReplaceNode(node, ret);
On Wed, Feb 24, 2021 at 22:25:56 -0500, Laine Stump wrote:
On 2/24/21 11:16 AM, Peter Krempa wrote:
Out of memory isn't the only reason the function can fail. Add a message stating that copying of a XML node failed.
Could it still fail due to OOM as well, though? And if so, is there any way of differentiating? (I previously looked into this for some other libxml2 function, ended up asking DV, and I believe I was told that there isn't any way to tell the difference, but as usual IMBES (I May Be Experiencing Senility).
Yes, it's one of the options.
At any rate, your changed message is more correct, since it's not making any unsubstantiated assumption about the cause of the error, while virRemoveOOMError() was).
I tried to formulate them such that they will be true even in OOM case by making them as vague as the description of the error reported by the API is. In case of OOM though, if the OOM condition persists, the logging call will abort() anyways right when it's called, so in all but the very rarest cases we'd report the correct error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index ebe479f5d3..62bbafacd6 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -51,10 +51,8 @@ virXMLXPathContextNew(xmlDocPtr xml) { xmlXPathContextPtr ctxt; - if (!(ctxt = xmlXPathNewContext(xml))) { - virReportOOMError(); - return NULL; - } + if (!(ctxt = xmlXPathNewContext(xml))) + abort(); return ctxt; } -- 2.29.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virxml.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index 62bbafacd6..060b7530fc 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -789,10 +789,8 @@ virXMLParseHelper(int domcode, /* Set up a parser context so we can catch the details of XML errors. */ pctxt = xmlNewParserCtxt(); - if (!pctxt || !pctxt->sax) { - virReportOOMError(); - goto error; - } + if (!pctxt || !pctxt->sax) + abort(); private.domcode = domcode; pctxt->_private = &private; -- 2.29.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virprocess.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 41c5678537..69d64e9466 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -462,10 +462,8 @@ int virProcessSetAffinity(pid_t pid, virBitmapPtr map, bool quiet) masklen = CPU_ALLOC_SIZE(numcpus); mask = CPU_ALLOC(numcpus); - if (!mask) { - virReportOOMError(); - return -1; - } + if (!mask) + abort(); CPU_ZERO_S(masklen, mask); for (i = 0; i < virBitmapSize(map); i++) { @@ -509,10 +507,8 @@ virProcessGetAffinity(pid_t pid) masklen = CPU_ALLOC_SIZE(ncpus); mask = CPU_ALLOC(ncpus); - if (!mask) { - virReportOOMError(); - return NULL; - } + if (!mask) + abort(); CPU_ZERO_S(masklen, mask); -- 2.29.2
If the argument of 'xmlSaveUri' is non-NULL the function returns NULL on OOM failure only. Thus we can directly abort rather than try to do the impossible recovery. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viruri.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/util/viruri.c b/src/util/viruri.c index 0aafd49d6d..1e8808ddc6 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -238,11 +238,9 @@ virURIFormat(virURIPtr uri) if (!xmluri.server && !xmluri.port) xmluri.port = -1; - ret = (char *)xmlSaveUri(&xmluri); - if (!ret) { - virReportOOMError(); - return NULL; - } + /* xmlSaveUri can fail only on OOM condition if argument is non-NULL */ + if (!(ret = (char *)xmlSaveUri(&xmluri))) + abort(); return ret; } -- 2.29.2
Similarly to other allocation calls abort() on failure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/iohelper.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 49d939d290..b8810d16d3 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -27,6 +27,7 @@ #include <unistd.h> #include <fcntl.h> +#include <stdlib.h> #include "virthread.h" #include "virfile.h" @@ -57,10 +58,8 @@ runIO(const char *path, int fd, int oflags) off_t end = 0; #if WITH_POSIX_MEMALIGN - if (posix_memalign(&base, alignMask + 1, buflen)) { - virReportOOMError(); - goto cleanup; - } + if (posix_memalign(&base, alignMask + 1, buflen)) + abort(); buf = base; #else buf = g_new0(char, buflen + alignMask); -- 2.29.2
The function just allocates a helper object. Reporting errors would be pointless when we encounter OOM situation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/hyperv/hyperv_wmi.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 8bb6f591f1..2a898cdf03 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -92,12 +92,8 @@ hypervVerifyResponse(WsManClient *client, WsXmlDocH response, } if (wsmc_check_for_fault(response)) { - fault = wsmc_fault_new(); - - if (fault == NULL) { - virReportOOMError(); - return -1; - } + if (!(fault = wsmc_fault_new())) + abort(); wsmc_get_fault_data(response, fault); -- 2.29.2
Trying to report an error on OOM is pointless since error handling allocates memory. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/vbox/vbox_common.c | 20 -------------------- src/vbox/vbox_common.h | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 138403b034..ce9bfbf827 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5465,17 +5465,9 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, } VBOX_UTF8_TO_UTF16(def->parent.name, &name); - if (!name) { - virReportOOMError(); - goto cleanup; - } if (def->parent.description) { VBOX_UTF8_TO_UTF16(def->parent.description, &description); - if (!description) { - virReportOOMError(); - goto cleanup; - } } rc = gVBoxAPI.UIConsole.TakeSnapshot(console, name, description, &progress); @@ -6475,10 +6467,6 @@ vboxDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, goto cleanup; } VBOX_UTF16_TO_UTF8(nameUtf16, &name); - if (!name) { - virReportOOMError(); - goto cleanup; - } ret = virGetDomainSnapshot(dom, name); @@ -6533,10 +6521,6 @@ vboxDomainSnapshotCurrent(virDomainPtr dom, unsigned int flags) } VBOX_UTF16_TO_UTF8(nameUtf16, &name); - if (!name) { - virReportOOMError(); - goto cleanup; - } ret = virGetDomainSnapshot(dom, name); @@ -6593,10 +6577,6 @@ static int vboxDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, } VBOX_UTF16_TO_UTF8(nameUtf16, &name); - if (!name) { - virReportOOMError(); - goto cleanup; - } ret = STREQ(snapshot->name, name); diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h index 8b1fb2ac30..1fb922aaf0 100644 --- a/src/vbox/vbox_common.h +++ b/src/vbox/vbox_common.h @@ -391,8 +391,19 @@ typedef nsISupports IKeyboard; } \ } while (0) -#define VBOX_UTF16_TO_UTF8(arg1, arg2) gVBoxAPI.UPFN.Utf16ToUtf8(data->pFuncs, arg1, arg2) -#define VBOX_UTF8_TO_UTF16(arg1, arg2) gVBoxAPI.UPFN.Utf8ToUtf16(data->pFuncs, arg1, arg2) +#define VBOX_UTF16_TO_UTF8(arg1, arg2) \ + do { \ + gVBoxAPI.UPFN.Utf16ToUtf8(data->pFuncs, arg1, arg2); \ + if (!*(arg2)) \ + abort(); \ + } while (0) + +#define VBOX_UTF8_TO_UTF16(arg1, arg2) \ + do { \ + gVBoxAPI.UPFN.Utf8ToUtf16(data->pFuncs, arg1, arg2); \ + if (!*(arg2)) \ + abort(); \ + } while (0) #define VBOX_ADDREF(arg) gVBoxAPI.nsUISupports.AddRef((void *)(arg)) -- 2.29.2
Attempting to report error in case when we ran out of memory is pointless. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 6 ++---- src/libxl/libxl_driver.c | 7 +++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index f8db118996..f2bcd77a29 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -854,10 +854,8 @@ libxlMakeVnumaList(virDomainDefPtr def, goto cleanup; libxl_bitmap_init(&vcpu_bitmap); - if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) { - virReportOOMError(); - goto cleanup; - } + if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) + abort(); do { libxl_bitmap_set(&vcpu_bitmap, cpu); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 75a8d46af0..434959d694 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4931,10 +4931,9 @@ libxlDomainGetNumaParameters(virDomainPtr dom, if (numnodes <= 0) goto cleanup; - if (libxl_node_bitmap_alloc(cfg->ctx, &nodemap, 0)) { - virReportOOMError(); - goto cleanup; - } + if (libxl_node_bitmap_alloc(cfg->ctx, &nodemap, 0)) + abort(); + nodes = virBitmapNew(numnodes); rc = libxl_domain_get_nodeaffinity(cfg->ctx, -- 2.29.2
'xmlNewDoc' and 'xmlNewDocComment' return NULL only on allocation failure. Attempting to raise an error is pointless. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/vbox/vbox_snapshot_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 5792d3175e..4f12d2ebfa 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -996,10 +996,8 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, goto cleanup; } xml = xmlNewDoc(BAD_CAST "1.0"); - if (!xml) { - virReportOOMError(); - goto cleanup; - } + if (!xml) + abort(); cur = virXMLNewNode(NULL, "VirtualBox"); @@ -1023,10 +1021,8 @@ virVBoxSnapshotConfSaveVboxFile(virVBoxSnapshotConfMachinePtr machine, "OVERWRITTEN AND LOST.\n" "Changes to this xml configuration should be made using Virtualbox\n" "or other application using the libvirt API"); - if (!cur) { - virReportOOMError(); - goto cleanup; - } + if (!cur) + abort(); if (!xmlAddPrevSibling(xmlDocGetRootElement(xml), cur)) { virReportError(VIR_ERR_XML_ERROR, "%s", -- 2.29.2
The yajl library returns a wide range of error codes so reporting OOM on any failure is wrong. In case the error was really based by memory issue the error reporting will probably cause an abort anyways. Change the error message so that we know that it happened in JSON at least. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virjson.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index e74b9fca4f..f2a6024db6 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1943,12 +1943,14 @@ virJSONValueToBuffer(virJSONValuePtr object, yajl_gen_config(g, yajl_gen_validate_utf8, 1); if (virJSONValueToStringOne(object, g) < 0) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to convert virJSONValue to yajl data")); goto cleanup; } if (yajl_gen_get_buf(g, &str, &len) != yajl_gen_status_ok) { - virReportOOMError(); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to format JSON")); goto cleanup; } -- 2.29.2
OOM isn't the only failure glfs_new can encounter. Report an error which might give more insight. libgfapi seems to be setting errno but reporting a system error migt be misleading. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_gluster.c | 3 ++- src/storage_file/storage_file_backend_gluster.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8de0cb8a6b..e673922df6 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -114,7 +114,8 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) /* Actually connect to glfs */ if (!(ret->vol = glfs_new(ret->volname))) { - virReportOOMError(); + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to create glfs object for '%s'"), ret->volname); goto error; } diff --git a/src/storage_file/storage_file_backend_gluster.c b/src/storage_file/storage_file_backend_gluster.c index 9b3b783274..8c7a583886 100644 --- a/src/storage_file/storage_file_backend_gluster.c +++ b/src/storage_file/storage_file_backend_gluster.c @@ -120,7 +120,8 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) (unsigned int)drv->uid, (unsigned int)drv->gid); if (!(priv->vol = glfs_new(src->volume))) { - virReportOOMError(); + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to create glfs object for '%s'"), src->volume); goto error; } -- 2.29.2
The function has also non-OOM failure case when the passed string has 0 length, so reporting OOM error is not correct. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/vmx/vmx.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index e6c0900a65..73bf7c4f3d 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -781,12 +781,8 @@ virVMXConvertToUTF8(const char *encoding, const char *string) return NULL; } - if (!(input = xmlBufferCreateStatic((char *)string, strlen(string)))) { - virReportOOMError(); - goto cleanup; - } - - if (xmlCharEncInFunc(handler, utf8, input) < 0) { + if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) || + xmlCharEncInFunc(handler, utf8, input) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not convert from %s to UTF-8 encoding"), encoding); goto cleanup; -- 2.29.2
On 2/24/21 11:17 AM, Peter Krempa wrote:
The function has also non-OOM failure case when the passed string has 0 length, so reporting OOM error is not correct.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/vmx/vmx.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index e6c0900a65..73bf7c4f3d 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -781,12 +781,8 @@ virVMXConvertToUTF8(const char *encoding, const char *string) return NULL; }
- if (!(input = xmlBufferCreateStatic((char *)string, strlen(string)))) { - virReportOOMError(); - goto cleanup; - } - - if (xmlCharEncInFunc(handler, utf8, input) < 0) { + if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) || + xmlCharEncInFunc(handler, utf8, input) < 0) {
I would have left the error messages for the two functions separate, with adequate wording to tell which of them failed. But this is at worst still better than reporting OOM...
virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not convert from %s to UTF-8 encoding"), encoding); goto cleanup;
Trying to report an OOM error is pointless since our infrastructure to report error needs to allocate memory to report the error. In addition our code mistakenly reported OOM errors even in cases where a function could fail for another reason, which would make issues harder to debug. Remove the virReportOOMError and backend so that programmers are forced to think about what can happen. In case when there's another failure possible a specific error should be reported and otherwise a direct abort() is better since the logger would abort on g_new anyways. This patch also removes the syntas-check which forces use of virReportOOMError instead of using VIR_ERR_NO_MEMORY with other functions. This allows possible future use when we'd end up in a situation where trying to recover from an OOM would make sense, such as when attempting to allocate a massive buffer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/syntax-check.mk | 8 -------- src/libvirt_private.syms | 1 - src/util/virerror.c | 22 ---------------------- src/util/virerror.h | 8 -------- 4 files changed, 39 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index e51877648a..e1ccb74986 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -490,11 +490,6 @@ sc_prohibit_gettext_noop: halt='use N_, not gettext_noop' \ $(_sc_search_regexp) -sc_prohibit_VIR_ERR_NO_MEMORY: - @prohibit='\<VIR_ERR_NO_MEMORY\>' \ - halt='use virReportOOMError, not VIR_ERR_NO_MEMORY' \ - $(_sc_search_regexp) - sc_prohibit_PATH_MAX: @prohibit='\<PATH_MAX\>' \ halt='dynamically allocate paths, do not use PATH_MAX' \ @@ -1895,9 +1890,6 @@ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$) -exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ - ^(build-aux/syntax-check\.mk|include/libvirt/virterror\.h|src/remote/remote_daemon_dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ - exclude_file_name_regexp--sc_prohibit_PATH_MAX = \ ^build-aux/syntax-check\.mk$$ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 48f66daab8..7eb37ed797 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2116,7 +2116,6 @@ virLastErrorPrefixMessage; virRaiseErrorFull; virRaiseErrorObject; virReportErrorHelper; -virReportOOMErrorFull; virReportSystemErrorFull; virSetError; virSetErrorLogPriorityFunc; diff --git a/src/util/virerror.c b/src/util/virerror.c index 708081414a..a503cdefdc 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1358,28 +1358,6 @@ void virReportSystemErrorFull(int domcode, errno = save_errno; } -/** - * virReportOOMErrorFull: - * @domcode: the virErrorDomain indicating where it's coming from - * @filename: filename where error was raised - * @funcname: function name where error was raised - * @linenr: line number where error was raised - * - * Convenience internal routine called when an out of memory error is - * detected - */ -void virReportOOMErrorFull(int domcode, - const char *filename, - const char *funcname, - size_t linenr) -{ - const char *virerr; - - virerr = virErrorMsg(VIR_ERR_NO_MEMORY, NULL); - virRaiseErrorFull(filename, funcname, linenr, - domcode, VIR_ERR_NO_MEMORY, VIR_ERR_ERROR, - virerr, NULL, NULL, -1, -1, virerr, NULL); -} /** * virSetErrorLogPriorityFunc: diff --git a/src/util/virerror.h b/src/util/virerror.h index 9d3e40d65a..da7d7c0afe 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -174,14 +174,6 @@ void virReportSystemErrorFull(int domcode, "Unexpected enum value %d for %s", \ value, sizeof((typname)1) != 0 ? #typname : #typname); -void virReportOOMErrorFull(int domcode, - const char *filename, - const char *funcname, - size_t linenr); - -#define virReportOOMError() \ - virReportOOMErrorFull(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) - #define virReportError(code, ...) \ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) -- 2.29.2
On 2/24/21 11:17 AM, Peter Krempa wrote:
Trying to report an OOM error is pointless since our infrastructure to report error needs to allocate memory to report the error.
In addition our code mistakenly reported OOM errors even in cases where a function could fail for another reason, which would make issues harder to debug.
Remove the virReportOOMError and backend so that programmers are forced to think about what can happen. In case when there's another failure possible a specific error should be reported and otherwise a direct abort() is better since the logger would abort on g_new anyways.
This patch also removes the syntas-check which forces use of virReportOOMError instead of using VIR_ERR_NO_MEMORY with other functions. This allows possible future use when we'd end up in a situation where trying to recover from an OOM would make sense, such as when attempting to allocate a massive buffer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
and good riddance! Reviewed-by: Laine Stump <laine@redhat.com>
--- build-aux/syntax-check.mk | 8 -------- src/libvirt_private.syms | 1 - src/util/virerror.c | 22 ---------------------- src/util/virerror.h | 8 -------- 4 files changed, 39 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index e51877648a..e1ccb74986 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -490,11 +490,6 @@ sc_prohibit_gettext_noop: halt='use N_, not gettext_noop' \ $(_sc_search_regexp)
-sc_prohibit_VIR_ERR_NO_MEMORY: - @prohibit='\<VIR_ERR_NO_MEMORY\>' \ - halt='use virReportOOMError, not VIR_ERR_NO_MEMORY' \ - $(_sc_search_regexp) - sc_prohibit_PATH_MAX: @prohibit='\<PATH_MAX\>' \ halt='dynamically allocate paths, do not use PATH_MAX' \ @@ -1895,9 +1890,6 @@ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \
exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$)
-exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ - ^(build-aux/syntax-check\.mk|include/libvirt/virterror\.h|src/remote/remote_daemon_dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ - exclude_file_name_regexp--sc_prohibit_PATH_MAX = \ ^build-aux/syntax-check\.mk$$
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 48f66daab8..7eb37ed797 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2116,7 +2116,6 @@ virLastErrorPrefixMessage; virRaiseErrorFull; virRaiseErrorObject; virReportErrorHelper; -virReportOOMErrorFull; virReportSystemErrorFull; virSetError; virSetErrorLogPriorityFunc; diff --git a/src/util/virerror.c b/src/util/virerror.c index 708081414a..a503cdefdc 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1358,28 +1358,6 @@ void virReportSystemErrorFull(int domcode, errno = save_errno; }
-/** - * virReportOOMErrorFull: - * @domcode: the virErrorDomain indicating where it's coming from - * @filename: filename where error was raised - * @funcname: function name where error was raised - * @linenr: line number where error was raised - * - * Convenience internal routine called when an out of memory error is - * detected - */ -void virReportOOMErrorFull(int domcode, - const char *filename, - const char *funcname, - size_t linenr) -{ - const char *virerr; - - virerr = virErrorMsg(VIR_ERR_NO_MEMORY, NULL); - virRaiseErrorFull(filename, funcname, linenr, - domcode, VIR_ERR_NO_MEMORY, VIR_ERR_ERROR, - virerr, NULL, NULL, -1, -1, virerr, NULL); -}
/** * virSetErrorLogPriorityFunc: diff --git a/src/util/virerror.h b/src/util/virerror.h index 9d3e40d65a..da7d7c0afe 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -174,14 +174,6 @@ void virReportSystemErrorFull(int domcode, "Unexpected enum value %d for %s", \ value, sizeof((typname)1) != 0 ? #typname : #typname);
-void virReportOOMErrorFull(int domcode, - const char *filename, - const char *funcname, - size_t linenr); - -#define virReportOOMError() \ - virReportOOMErrorFull(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) - #define virReportError(code, ...) \ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__)
On 2/24/21 11:16 AM, Peter Krempa wrote:
'virReportOOMError' is nowadays mostly useless since an OOM error will trigger yet another allocation failure in the logger when attempting to log the error.
Additionally it was also used in few places which can fail with other failures than OOM.
To prevent both errorneous usage types, let's remove it completely.
Yay!! It bugs me every time I see it.
participants (3)
-
Ján Tomko -
Laine Stump -
Peter Krempa