On Mon, Sep 05, 2022 at 03:57:03PM +0200, Kristina Hanicova wrote:
This patch adds the generalized job object into the domain object
so that it can be used by all drivers without the need to extract
it from the private data.
Because of this, the job object needs to be created and set
during the creation of the domain object. This patch also extends
xmlopt with possible job config containing virDomainJobObj
callbacks, its private data callbacks and one variable
(maxQueuedJobs).
This patch includes:
* addition of virDomainJobObj into virDomainObj (used in the
following patches)
* extending xmlopt with job config structure
* new function for freeing the virDomainJobObj
Since this series was pushed, virsh unit tests are failing on
some platforms:
# ./build/tools/virsh -c test:///default uri
test:///default
Segmentation fault (core dumped)
In fact after checking with valgrind, they should be failing on all
platforms, but most aren't crashing on the use-after-free
$ valgrind ./build/tools/virsh -c test:///default uri
test:///default
==2099625== Invalid read of size 8
==2099625== at 0x4A1FCCB: virDomainObjResetAsyncJob (virdomainjob.c:184)
==2099625== by 0x4A1FDF0: virDomainObjClearJob (virdomainjob.c:224)
==2099625== by 0x4A1FE8D: virDomainJobObjFree (virdomainjob.c:240)
==2099625== by 0x499017D: vir_object_finalize (virobject.c:323)
==2099625== by 0x52F8D31: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4F357D2: ??? (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F39772: g_hash_table_unref (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x499017D: vir_object_finalize (virobject.c:323)
==2099625== by 0x52F8D31: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4B6F18A: testDriverDispose (test_driver.c:158)
==2099625== Address 0x69aa1e8 is 392 bytes inside a block of size 464 free'd
==2099625== at 0x48480E4: free (vg_replace_malloc.c:872)
==2099625== by 0x4F4BB8C: g_free (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F67013: g_slice_free1 (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x530D7D3: g_type_free_instance (in
/usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4B6F17E: testDriverDispose (test_driver.c:157)
==2099625== by 0x499017D: vir_object_finalize (virobject.c:323)
==2099625== by 0x52F8D31: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x49907D7: virObjectUnref (virobject.c:377)
==2099625== by 0x4B6EE62: testDriverCloseInternal (test_driver.c:1498)
==2099625== by 0x4B6EEE2: testConnectClose (test_driver.c:1543)
==2099625== by 0x4BBE495: virConnectDispose (datatypes.c:174)
==2099625== Block was alloc'd at
==2099625== at 0x484586F: malloc (vg_replace_malloc.c:381)
==2099625== by 0x4F4F278: g_malloc (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F67BA5: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x4F69BCC: g_slice_alloc0 (in /usr/lib64/libglib-2.0.so.0.7200.3)
==2099625== by 0x5313016: g_type_create_instance (in
/usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x52FAE37: ??? (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x52FC080: g_object_new_with_properties (in
/usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x52FCB20: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==2099625== by 0x4990723: virObjectNew (virobject.c:252)
==2099625== by 0x49E0DD6: virDomainXMLOptionNew (domain_conf.c:1616)
==2099625== by 0x4B6F098: testDriverNew (test_driver.c:463)
==2099625== by 0x4B7709C: testOpenDefault (test_driver.c:1397)
==2099625== by 0x4B7709C: testConnectOpen (test_driver.c:1522)
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
src/bhyve/bhyve_domain.c | 2 +-
src/ch/ch_conf.c | 2 +-
src/conf/domain_conf.c | 13 ++++++++++++-
src/conf/domain_conf.h | 16 +++++++++++++++-
src/conf/virconftypes.h | 2 ++
src/conf/virdomainjob.c | 11 +++++++++++
src/conf/virdomainjob.h | 5 ++++-
src/hyperv/hyperv_driver.c | 2 +-
src/libvirt_private.syms | 1 +
src/libxl/libxl_conf.c | 2 +-
src/lxc/lxc_conf.c | 2 +-
src/openvz/openvz_conf.c | 2 +-
src/qemu/qemu_conf.c | 3 ++-
src/qemu/qemu_process.c | 2 +-
src/security/virt-aa-helper.c | 2 +-
src/test/test_driver.c | 2 +-
src/vbox/vbox_common.c | 2 +-
src/vmware/vmware_driver.c | 2 +-
src/vmx/vmx.c | 2 +-
src/vz/vz_driver.c | 2 +-
tests/bhyveargv2xmltest.c | 2 +-
tests/testutils.c | 2 +-
22 files changed, 62 insertions(+), 19 deletions(-)
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
index 69555a3efc..b7b2db57b8 100644
--- a/src/bhyve/bhyve_domain.c
+++ b/src/bhyve/bhyve_domain.c
@@ -221,7 +221,7 @@ virBhyveDriverCreateXMLConf(struct _bhyveConn *driver)
return virDomainXMLOptionNew(&virBhyveDriverDomainDefParserConfig,
&virBhyveDriverPrivateDataCallbacks,
&virBhyveDriverDomainXMLNamespace,
- NULL, NULL);
+ NULL, NULL, NULL);
}
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
index 775596e9f5..0d07fa270c 100644
--- a/src/ch/ch_conf.c
+++ b/src/ch/ch_conf.c
@@ -110,7 +110,7 @@ chDomainXMLConfInit(virCHDriver *driver)
virCHDriverDomainDefParserConfig.priv = driver;
return virDomainXMLOptionNew(&virCHDriverDomainDefParserConfig,
&virCHDriverPrivateDataCallbacks,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
virCHDriverConfig *
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 237f1d6835..35ac2bb1c2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1604,7 +1604,8 @@ virDomainXMLOptionNew(virDomainDefParserConfig *config,
virDomainXMLPrivateDataCallbacks *priv,
virXMLNamespace *xmlns,
virDomainABIStability *abi,
- virSaveCookieCallbacks *saveCookie)
+ virSaveCookieCallbacks *saveCookie,
+ virDomainJobObjConfig *jobConfig)
{
virDomainXMLOption *xmlopt;
@@ -1629,6 +1630,9 @@ virDomainXMLOptionNew(virDomainDefParserConfig *config,
if (saveCookie)
xmlopt->saveCookie = *saveCookie;
+ if (jobConfig)
+ xmlopt->jobObjConfig = *jobConfig;
+
/* Technically this forbids to use one of Xerox's MAC address prefixes in
* our hypervisor drivers. This shouldn't ever be a problem.
*
@@ -3857,6 +3861,7 @@ static void virDomainObjDispose(void *obj)
virDomainObjDeprecationFree(dom);
virDomainSnapshotObjListFree(dom->snapshots);
virDomainCheckpointObjListFree(dom->checkpoints);
+ virDomainJobObjFree(dom->job);
}
virDomainObj *
@@ -3889,6 +3894,12 @@ virDomainObjNew(virDomainXMLOption *xmlopt)
if (!(domain->checkpoints = virDomainCheckpointObjListNew()))
goto error;
+ domain->job = g_new0(virDomainJobObj, 1);
+ if (virDomainObjInitJob(domain->job,
+ &xmlopt->jobObjConfig.cb,
+ &xmlopt->jobObjConfig.jobDataPrivateCb) < 0)
+ goto error;
+
virObjectLock(domain);
virDomainObjSetState(domain, VIR_DOMAIN_SHUTOFF,
VIR_DOMAIN_SHUTOFF_UNKNOWN);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2b1497d78d..42fa2a4400 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -56,6 +56,7 @@
#include "virsavecookie.h"
#include "virresctrl.h"
#include "virenum.h"
+#include "virdomainjob.h"
/* Flags for the 'type' field in virDomainDeviceDef */
typedef enum {
@@ -3093,6 +3094,8 @@ struct _virDomainObj {
virObjectLockable parent;
virCond cond;
+ virDomainJobObj *job;
+
pid_t pid; /* 0 for no PID, avoid negative values like -1 */
virDomainStateReason state;
@@ -3277,11 +3280,19 @@ struct _virDomainABIStability {
virDomainABIStabilityDomain domain;
};
+
+struct _virDomainJobObjConfig {
+ virDomainObjPrivateJobCallbacks cb;
+ virDomainJobDataPrivateDataCallbacks jobDataPrivateCb;
+ unsigned int maxQueuedJobs;
+};
+
virDomainXMLOption *virDomainXMLOptionNew(virDomainDefParserConfig *config,
virDomainXMLPrivateDataCallbacks *priv,
virXMLNamespace *xmlns,
virDomainABIStability *abi,
- virSaveCookieCallbacks *saveCookie);
+ virSaveCookieCallbacks *saveCookie,
+ virDomainJobObjConfig *jobConfig);
virSaveCookieCallbacks *
virDomainXMLOptionGetSaveCookie(virDomainXMLOption *xmlopt);
@@ -3321,6 +3332,9 @@ struct _virDomainXMLOption {
/* Snapshot postparse callbacks */
virDomainMomentPostParseCallback momentPostParse;
+
+ /* virDomainJobObj callbacks, private data callbacks and defaults */
+ virDomainJobObjConfig jobObjConfig;
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainXMLOption, virObjectUnref);
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index c3f1c5fa01..154805091a 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -150,6 +150,8 @@ typedef struct _virDomainIdMapEntry virDomainIdMapEntry;
typedef struct _virDomainInputDef virDomainInputDef;
+typedef struct _virDomainJobObjConfig virDomainJobObjConfig;
+
typedef struct _virDomainKeyWrapDef virDomainKeyWrapDef;
typedef struct _virDomainLeaseDef virDomainLeaseDef;
diff --git a/src/conf/virdomainjob.c b/src/conf/virdomainjob.c
index 5ab4bdc18b..a073ff08cd 100644
--- a/src/conf/virdomainjob.c
+++ b/src/conf/virdomainjob.c
@@ -13,6 +13,7 @@
#include "virthreadjob.h"
#include "virlog.h"
#include "virtime.h"
+#include "domain_conf.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -230,6 +231,16 @@ virDomainObjClearJob(virDomainJobObj *job)
g_clear_pointer(&job->privateData, job->cb->freeJobPrivate);
}
+void
+virDomainJobObjFree(virDomainJobObj *job)
+{
+ if (!job)
+ return;
+
+ virDomainObjClearJob(job);
+ g_free(job);
+}
+
bool
virDomainTrackJob(virDomainJob job)
{
diff --git a/src/conf/virdomainjob.h b/src/conf/virdomainjob.h
index bdfdc91935..091d951aa6 100644
--- a/src/conf/virdomainjob.h
+++ b/src/conf/virdomainjob.h
@@ -11,7 +11,8 @@
#include "virenum.h"
#include "virthread.h"
#include "virbuffer.h"
-#include "domain_conf.h"
+#include "virconftypes.h"
+#include "virxml.h"
#define JOB_MASK(job) (job == 0 ? 0 : 1 << (job - 1))
#define VIR_JOB_DEFAULT_MASK \
@@ -227,6 +228,8 @@ int virDomainObjPreserveJob(virDomainJobObj *currJob,
void virDomainObjClearJob(virDomainJobObj *job);
G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virDomainJobObj, virDomainObjClearJob);
+void virDomainJobObjFree(virDomainJobObj *job);
+
bool virDomainTrackJob(virDomainJob job);
bool virDomainNestedJobAllowed(virDomainJobObj *jobs, virDomainJob newJob);
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 288c01ad14..3929e27e09 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -1766,7 +1766,7 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
goto cleanup;
/* init xmlopt for domain XML */
- priv->xmlopt = virDomainXMLOptionNew(&hypervDomainDefParserConfig, NULL,
NULL, NULL, NULL);
+ priv->xmlopt = virDomainXMLOptionNew(&hypervDomainDefParserConfig, NULL,
NULL, NULL, NULL, NULL);
if (hypervGetOperatingSystem(priv, &os) < 0)
goto cleanup;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5077db9c6b..cd0b94297c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1182,6 +1182,7 @@ virDomainAsyncJobTypeToString;
virDomainJobDataCopy;
virDomainJobDataFree;
virDomainJobDataInit;
+virDomainJobObjFree;
virDomainJobStatusToType;
virDomainJobTypeFromString;
virDomainJobTypeToString;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 06f338ef90..918303c8d0 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -2486,5 +2486,5 @@ libxlCreateXMLConf(libxlDriverPrivate *driver)
return virDomainXMLOptionNew(&libxlDomainDefParserConfig,
&libxlDomainXMLPrivateDataCallbacks,
&libxlDriverDomainXMLNamespace,
- NULL, NULL);
+ NULL, NULL, NULL);
}
diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
index 7834801fb5..fefe63bf20 100644
--- a/src/lxc/lxc_conf.c
+++ b/src/lxc/lxc_conf.c
@@ -189,7 +189,7 @@ lxcDomainXMLConfInit(virLXCDriver *driver, const char *defsecmodel)
return virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig,
&virLXCDriverPrivateDataCallbacks,
&virLXCDriverDomainXMLNamespace,
- NULL, NULL);
+ NULL, NULL, NULL);
}
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index c94f9b8577..c28d0e9f43 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -1062,5 +1062,5 @@ virDomainXMLOption *openvzXMLOption(struct openvz_driver *driver)
{
openvzDomainDefParserConfig.priv = driver;
return virDomainXMLOptionNew(&openvzDomainDefParserConfig,
- NULL, NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL, NULL);
}
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3b75cdeb95..2809ae53b9 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1278,7 +1278,8 @@ virQEMUDriverCreateXMLConf(virQEMUDriver *driver,
&virQEMUDriverPrivateDataCallbacks,
&virQEMUDriverDomainXMLNamespace,
&virQEMUDriverDomainABIStability,
- &virQEMUDriverDomainSaveCookie);
+ &virQEMUDriverDomainSaveCookie,
+ NULL);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 32f03ff79a..251bca4bdf 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -9295,7 +9295,7 @@ qemuProcessQMPConnectMonitor(qemuProcessQMP *proc)
monConfig.data.nix.path = proc->monpath;
monConfig.data.nix.listen = false;
- if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) ||
+ if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL, NULL)) ||
!(proc->vm = virDomainObjNew(xmlopt)) ||
!(proc->vm->def = virDomainDefNew(xmlopt)))
return -1;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 2d0bc99c73..f338488da3 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -632,7 +632,7 @@ get_definition(vahControl * ctl, const char *xmlStr)
}
if (!(ctl->xmlopt = virDomainXMLOptionNew(&virAAHelperDomainDefParserConfig,
- NULL, NULL, NULL, NULL))) {
+ NULL, NULL, NULL, NULL, NULL))) {
vah_error(ctl, 0, _("Failed to create XML config object"));
return -1;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 641a141b6a..686ff051a8 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -460,7 +460,7 @@ testDriverNew(void)
if (!(ret = virObjectLockableNew(testDriverClass)))
return NULL;
- if (!(ret->xmlopt = virDomainXMLOptionNew(&config, &privatecb, &ns,
NULL, NULL)) ||
+ if (!(ret->xmlopt = virDomainXMLOptionNew(&config, &privatecb, &ns,
NULL, NULL, NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
!(ret->ifaces = virInterfaceObjListNew()) ||
!(ret->domains = virDomainObjListNew()) ||
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index e249980195..bd77641d39 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -143,7 +143,7 @@ vboxDriverObjNew(void)
if (!(driver->caps = vboxCapsInit()) ||
!(driver->xmlopt = virDomainXMLOptionNew(&vboxDomainDefParserConfig,
- NULL, NULL, NULL, NULL)))
+ NULL, NULL, NULL, NULL, NULL)))
goto cleanup;
return driver;
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 2a18d73988..3c578434f3 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -139,7 +139,7 @@ vmwareDomainXMLConfigInit(struct vmware_driver *driver)
.free = vmwareDataFreeFunc };
vmwareDomainDefParserConfig.priv = driver;
return virDomainXMLOptionNew(&vmwareDomainDefParserConfig, &priv,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
static virDrvOpenStatus
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index fa5cc56146..bf0dba17d8 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -696,7 +696,7 @@ virVMXDomainXMLConfInit(virCaps *caps)
{
virVMXDomainDefParserConfig.priv = caps;
return virDomainXMLOptionNew(&virVMXDomainDefParserConfig, NULL,
- &virVMXDomainXMLNamespace, NULL, NULL);
+ &virVMXDomainXMLNamespace, NULL, NULL, NULL);
}
char *
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 017c084ede..571d895167 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -331,7 +331,7 @@ vzDriverObjNew(void)
if (!(driver->caps = vzBuildCapabilities()) ||
!(driver->xmlopt = virDomainXMLOptionNew(&vzDomainDefParserConfig,
&vzDomainXMLPrivateDataCallbacksPtr,
- NULL, NULL, NULL)) ||
+ NULL, NULL, NULL, NULL)) ||
!(driver->domains = virDomainObjListNew()) ||
!(driver->domainEventState = virObjectEventStateNew()) ||
(vzInitVersion(driver) < 0) ||
diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c
index 2ccc1379b9..92189a2e58 100644
--- a/tests/bhyveargv2xmltest.c
+++ b/tests/bhyveargv2xmltest.c
@@ -113,7 +113,7 @@ mymain(void)
if ((driver.caps = virBhyveCapsBuild()) == NULL)
return EXIT_FAILURE;
- if ((driver.xmlopt = virDomainXMLOptionNew(NULL, NULL,
+ if ((driver.xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL,
NULL, NULL, NULL)) == NULL)
return EXIT_FAILURE;
diff --git a/tests/testutils.c b/tests/testutils.c
index 30f91dcd28..8fe624f029 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -989,7 +989,7 @@ static virDomainDefParserConfig virTestGenericDomainDefParserConfig =
{
virDomainXMLOption *virTestGenericDomainXMLConfInit(void)
{
return virDomainXMLOptionNew(&virTestGenericDomainDefParserConfig,
- NULL, NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL, NULL);
}
--
2.37.2
With regards,
Daniel
--
|:
https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|:
https://libvirt.org -o-
https://fstop138.berrange.com :|
|:
https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|