[PATCH 0/4] qemu: Move originalMemlock
As I tried to explain the purpose of virDomainObj to somebody I've came across this very weirdly placed member: originalMemlock. Let's move it into qemuDomainObjPrivate. But after I've done so I've realized the error label is needless. So that's dropped too. Michal Prívozník (4): conf: Move virDomainObj::originalMemlock into qemuDomainObjPrivate qemu_domain: Format qemuDomainObjPrivate::originalMemlock qemu_domain: Drop needless free from qemuDomainObjPrivateXMLParse() qemu_domain: Drop needless 'error' label in qemuDomainObjPrivateXMLParse() src/conf/domain_conf.h | 3 -- src/qemu/qemu_domain.c | 76 +++++++++++++++++++++++------------------- src/qemu/qemu_domain.h | 3 ++ 3 files changed, 45 insertions(+), 37 deletions(-) -- 2.35.1
Since v1.3.0-90-gafbe1d4c56 the original value of memlock limit is stored inside virDomainObj struct directly (under originalMemlock member). This is needless because the value is used only inside QEMU driver and thus can reside in qemuDomainObjPrivate struct. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.h | 3 --- src/qemu/qemu_domain.c | 9 +++++---- src/qemu/qemu_domain.h | 3 +++ 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8a48646160..f5d9df3fab 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3064,9 +3064,6 @@ struct _virDomainObj { int taint; size_t ndeprecations; char **deprecations; - - unsigned long long originalMemlock; /* Original RLIMIT_MEMLOCK, zero if no - * restore will be required later */ }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainObj, virObjectUnref); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7974cdb00b..4edae9cca9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9445,6 +9445,7 @@ int qemuDomainAdjustMaxMemLock(virDomainObj *vm, bool forceVFIO) { + qemuDomainObjPrivate *priv = vm->privateData; unsigned long long currentMemLock = 0; unsigned long long desiredMemLock = 0; @@ -9457,8 +9458,8 @@ qemuDomainAdjustMaxMemLock(virDomainObj *vm, /* If this is the first time adjusting the limit, save the current * value so that we can restore it once memory locking is no longer * required */ - if (vm->originalMemlock == 0) { - vm->originalMemlock = currentMemLock; + if (priv->originalMemlock == 0) { + priv->originalMemlock = currentMemLock; } } else { /* If the limit is already high enough, we can assume @@ -9471,8 +9472,8 @@ qemuDomainAdjustMaxMemLock(virDomainObj *vm, } else { /* Once memory locking is no longer required, we can restore the * original, usually very low, limit */ - desiredMemLock = vm->originalMemlock; - vm->originalMemlock = 0; + desiredMemLock = priv->originalMemlock; + priv->originalMemlock = 0; } if (desiredMemLock > 0 && diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c7125722e0..248af92649 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -241,6 +241,9 @@ struct _qemuDomainObjPrivate { GSList *dbusVMStateIds; /* true if -object dbus-vmstate was added */ bool dbusVMState; + + unsigned long long originalMemlock; /* Original RLIMIT_MEMLOCK, zero if no + * restore will be required later */ }; #define QEMU_DOMAIN_PRIVATE(vm) \ -- 2.35.1
Now that qemuDomainObjPrivate struct gained new member format it into XML and parse it so that the value is preserved across daemon restarts. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4edae9cca9..9602a42196 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2386,6 +2386,12 @@ qemuDomainObjPrivateXMLFormat(virBuffer *buf, if (qemuDomainObjPrivateXMLFormatBackups(buf, vm) < 0) return -1; + if (priv->originalMemlock > 0) { + virBufferAsprintf(buf, + "<originalMemlock>%llu</originalMemlock>\n", + priv->originalMemlock); + } + return 0; } @@ -3102,6 +3108,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->memPrealloc = virXPathBoolean("boolean(./memPrealloc)", ctxt) == 1; + if (virXPathULongLong("string(./originalMemlock)", + ctxt, &priv->originalMemlock) == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse agent timeout")); + goto error; + } + return 0; error: -- 2.35.1
On Wed, May 11, 2022 at 17:03:17 +0200, Michal Privoznik wrote:
Now that qemuDomainObjPrivate struct gained new member format it into XML and parse it so that the value is preserved across daemon restarts.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4edae9cca9..9602a42196 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2386,6 +2386,12 @@ qemuDomainObjPrivateXMLFormat(virBuffer *buf, if (qemuDomainObjPrivateXMLFormatBackups(buf, vm) < 0) return -1;
+ if (priv->originalMemlock > 0) { + virBufferAsprintf(buf, + "<originalMemlock>%llu</originalMemlock>\n", + priv->originalMemlock); + } + return 0; }
@@ -3102,6 +3108,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
priv->memPrealloc = virXPathBoolean("boolean(./memPrealloc)", ctxt) == 1;
+ if (virXPathULongLong("string(./originalMemlock)", + ctxt, &priv->originalMemlock) == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse agent timeout"));
A bit too much copypasta ;)
+ goto error; + } + return 0;
error: -- 2.35.1
The qemuDomainObjPrivateXMLParse() is responsible for parsing given XML into qemuDomainObjPrivate struct. As it does so, memory might be allocated for some members. If an error occurs during parsing the control jumps onto 'error' label where only some of previously allocated memory is freed. The reason there's no memory leak is simple: the only caller (virDomainObjParseXML()) unrefs freshly created virDomainObj which in turn causes qemuDomainObjPrivateFree() to be called. Therefore, these partial, selective frees are needless and should be just dropped. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9602a42196..4de4840c8e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3118,9 +3118,6 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, return 0; error: - g_clear_pointer(&priv->namespaces, virBitmapFree); - g_clear_pointer(&priv->monConfig, virObjectUnref); - g_clear_pointer(&priv->qemuDevices, g_strfreev); return -1; } -- 2.35.1
After previous cleanup the 'error' label in qemuDomainObjPrivateXMLParse() contains nothing but a return statement. Well, the label can be dropped and all 'goto'-s can be replaced with the return statement directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 53 ++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4de4840c8e..a86bba93d1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2912,13 +2912,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, g_autoptr(virQEMUCaps) qemuCaps = NULL; if (!(priv->monConfig = virDomainChrSourceDefNew(NULL))) - goto error; + return -1; if (!(monitorpath = virXPathString("string(./monitor[1]/@path)", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no monitor path")); - goto error; + return -1; } tmp = virXPathString("string(./monitor[1]/@type)", ctxt); @@ -2940,13 +2940,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported monitor type '%s'"), virDomainChrTypeToString(priv->monConfig->type)); - goto error; + return -1; } if (virXPathInt("string(./agentTimeout)", ctxt, &priv->agentTimeout) == -2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse agent timeout")); - goto error; + return -1; } priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 0; @@ -2963,11 +2963,11 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed namespace name: %s"), next->name); - goto error; + return -1; } if (qemuDomainEnableNamespace(vm, ns) < 0) - goto error; + return -1; } } @@ -2979,22 +2979,22 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->rememberOwner = virXPathBoolean("count(./rememberOwner) > 0", ctxt); if ((n = virXPathNodeSet("./vcpus/vcpu", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { if (qemuDomainObjPrivateXMLParseVcpu(nodes[i], i, vm->def) < 0) - goto error; + return -1; } VIR_FREE(nodes); if ((n = virXPathNodeSet("./qemuCaps/flag", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu capabilities flags")); - goto error; + return -1; } if (n > 0) { if (!(qemuCaps = virQEMUCapsNew())) - goto error; + return -1; for (i = 0; i < n; i++) { g_autofree char *str = virXMLPropString(nodes[i], "name"); @@ -3003,7 +3003,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (flag < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown qemu capabilities flag %s"), str); - goto error; + return -1; } virQEMUCapsSet(qemuCaps, flag); } @@ -3016,14 +3016,14 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->lockState = virXPathString("string(./lockstate)", ctxt); if (qemuDomainObjPrivateXMLParseJob(vm, ctxt) < 0) - goto error; + return -1; priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; if ((n = virXPathNodeSet("./devices/device", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu device list")); - goto error; + return -1; } if (n > 0) { /* NULL-terminated list */ @@ -3034,7 +3034,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (!priv->qemuDevices[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu device list")); - goto error; + return -1; } } } @@ -3043,7 +3043,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if ((n = virXPathNodeSet("./slirp/helper", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse slirp helper list")); - goto error; + return -1; } for (i = 0; i < n; i++) { g_autofree char *alias = virXMLPropString(nodes[i], "alias"); @@ -3055,22 +3055,22 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virStrToLong_i(pid, NULL, 10, &slirp->pid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse slirp helper list")); - goto error; + return -1; } if (virDomainDefFindDevice(vm->def, alias, &dev, true) < 0 || dev.type != VIR_DOMAIN_DEVICE_NET) - goto error; + return -1; if (qemuDomainObjPrivateXMLParseSlirpFeatures(nodes[i], ctxt, slirp) < 0) - goto error; + return -1; QEMU_DOMAIN_NETWORK_PRIVATE(dev.data.net)->slirp = g_steal_pointer(&slirp); } VIR_FREE(nodes); if (qemuDomainObjPrivateXMLParseAutomaticPlacement(ctxt, priv) < 0) - goto error; + return -1; if ((tmp = virXPathString("string(./libDir/@path)", ctxt))) priv->libDir = tmp; @@ -3082,28 +3082,28 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (virCPUDefParseXML(ctxt, "./cpu", VIR_CPU_TYPE_GUEST, &priv->origCPU, false) < 0) - goto error; + return -1; priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)", ctxt) == 1; if (qemuDomainObjPrivateXMLParseAllowReboot(ctxt, &priv->allowReboot) < 0) - goto error; + return -1; qemuDomainObjPrivateXMLParsePR(ctxt, &priv->prDaemonRunning); if (qemuDomainObjPrivateXMLParseBlockjobs(vm, priv, ctxt) < 0) - goto error; + return -1; if (qemuDomainObjPrivateXMLParseBackups(priv, ctxt) < 0) - goto error; + return -1; qemuDomainStorageIDReset(priv); if (virXPathULongLong("string(./nodename/@index)", ctxt, &priv->nodenameindex) == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to parse node name index")); - goto error; + return -1; } priv->memPrealloc = virXPathBoolean("boolean(./memPrealloc)", ctxt) == 1; @@ -3112,13 +3112,10 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, ctxt, &priv->originalMemlock) == -2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse agent timeout")); - goto error; + return -1; } return 0; - - error: - return -1; } -- 2.35.1
On Wed, May 11, 2022 at 17:03:15 +0200, Michal Privoznik wrote:
As I tried to explain the purpose of virDomainObj to somebody I've came across this very weirdly placed member: originalMemlock. Let's move it into qemuDomainObjPrivate. But after I've done so I've realized the error label is needless. So that's dropped too.
Michal Prívozník (4): conf: Move virDomainObj::originalMemlock into qemuDomainObjPrivate qemu_domain: Format qemuDomainObjPrivate::originalMemlock qemu_domain: Drop needless free from qemuDomainObjPrivateXMLParse() qemu_domain: Drop needless 'error' label in qemuDomainObjPrivateXMLParse()
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa