10:16 a.m.
'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
10:16 a.m.
New subject: [PATCH 01/33] Remove useless comments for VIR_FROM_THIS definition
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 02/33] util: xml: Introduce autoptr cleanup support for 'xmlNode'
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 03/33] virDomainDefSetMetadata: Avoid temporary variable for string copy
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(a)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
10:16 a.m.
New subject: [PATCH 04/33] virCommandAddEnv: Make stealing of argument more obvious
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(a)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
10:16 a.m.
New subject: [PATCH 05/33] virCommandAddArgBuffer: Simplify clearing of @buf
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(a)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
10:16 a.m.
New subject: [PATCH 06/33] virCPUx86DataParse: Don't check error from x86FeatureNames
x86FeatureNames uses virBuffer and thus can't fail nowadays.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 07/33] virhostcputest: linuxCPUStatsCompareFiles: Don't check return value of virBufferContentAndReset
The buffer won't encounter OOM condition nowadays
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 08/33] virBuildPath: Remove return value
The function can't fail nowadays, remove the return value and adjust
callers.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 09/33] lxc_process: Remove OOM handling from logging setup
'virLogGetFilters' doesn't return failure and 'virLogGetOutputs'
reports
it's own errors.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 10/33] virDomainDefSetMetadata: Rework memory handling
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(a)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
10:16 a.m.
New subject: [PATCH 11/33] util: vircommand: Remove OOM handling
The OOM error handling is dead code nowadays.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 12/33] virCloseCallbacksGetForConn: Remove OOM handling
VIR_EXPAND_N will abort so we can simplify the hash iterator.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 13/33] virfirewall: Don't check OOM in ADD_ARG macro
VIR_RESIZE_N can't fail nowadays, adjust the macro.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 14/33] virfirewall: Remove OOM checks from virFirewallStartTransaction
Neither virFirewallGroupNew nor VIR_EXPAND_N can fail.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 15/33] virfirewall: virFirewallAddRuleFullV: Remove OOM check from VIR_APPEND_ELEMENT
VIR_APPEND_ELEMENT_COPY will abort the program on OOM so there's no need
to check.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 16/33] virfirewall: Remove impossible OOM error reporting
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(a)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
12:14 p.m.
New subject: [PATCH 16/33] virfirewall: Remove impossible OOM error reporting
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(a)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
10:16 a.m.
New subject: [PATCH 17/33] util: virnetlink: Add wrapper for 'nlmsg_alloc_simple'
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(a)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
10:16 a.m.
New subject: [PATCH 18/33] util: xml: Add virXMLBufferCreate wrapper
'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(a)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
10:16 a.m.
New subject: [PATCH 19/33] util: xml: Add wrapper for 'xmlNewNode'
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(a)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
10:16 a.m.
New subject: [PATCH 20/33] Don't report OOM error on xmlCopyNode failure
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(a)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
9:25 p.m.
New subject: [PATCH 20/33] Don't report OOM error on xmlCopyNode failure
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(a)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);
9:16 a.m.
New subject: [PATCH 20/33] Don't report OOM error on xmlCopyNode failure
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.
10:16 a.m.
New subject: [PATCH 21/33] virXMLXPathContextNew: abort() on allocation failure
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 22/33] virXMLParseHelper: abort() on allocation failure
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 23/33] util: virprocess: abort() on CPU_ALLOC failure
Signed-off-by: Peter Krempa <pkrempa(a)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
10:16 a.m.
New subject: [PATCH 24/33] virURIFormat: abort() on failure
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(a)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
10:17 a.m.
New subject: [PATCH 25/33] util: iohelper: Don't handle OOM from posix_memalign
Similarly to other allocation calls abort() on failure.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:17 a.m.
New subject: [PATCH 26/33] hyperv: abort() failure of wsmc_fault_new()
The function just allocates a helper object. Reporting errors would be
pointless when we encounter OOM situation.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:17 a.m.
New subject: [PATCH 27/33] vbox: abort() on allocation failure in UTF8<->UTF16 conversion
Trying to report an error on OOM is pointless since error handling
allocates memory.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:17 a.m.
New subject: [PATCH 28/33] libxl: abort() on failure of libxl_cpu_bitmap_alloc()
Attempting to report error in case when we ran out of memory is
pointless.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:17 a.m.
New subject: [PATCH 29/33] virVBoxSnapshotConfSaveVboxFile: abort() on failure to allocate xmlDoc and comment
'xmlNewDoc' and 'xmlNewDocComment' return NULL only on allocation
failure. Attempting to raise an error is pointless.
Signed-off-by: Peter Krempa <pkrempa(a)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
10:17 a.m.
New subject: [PATCH 30/33] util: json: Report non-OOM error on yajl failure
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(a)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
10:17 a.m.
New subject: [PATCH 31/33] storage: Don't report OOM error on failure of glfs_new
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(a)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
10:17 a.m.
New subject: [PATCH 32/33] virVMXConvertToUTF8: Report non-OOM error on failure of xmlBufferCreateStatic
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(a)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
9:33 p.m.
New subject: [PATCH 32/33] virVMXConvertToUTF8: Report non-OOM error on failure of xmlBufferCreateStatic
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(a)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;
10:17 a.m.
New subject: [PATCH 33/33] util: virerror: Remove virReportOOMError
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(a)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
9:37 p.m.
New subject: [PATCH 33/33] util: virerror: Remove virReportOOMError
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(a)redhat.com>
and good riddance!
Reviewed-by: Laine Stump <laine(a)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__)
6:32 p.m.
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.
1358
days inactive
1366
days old
39 comments
3 participants
participants (3)
-
Ján Tomko -
Laine Stump -
Peter Krempa