[GSoC][PATCH v4 3/4] qemu_domainjob: introduce `privateData` for `qemuDomainJob`
Michal Privoznik
mprivozn at redhat.com
Tue Jul 14 14:31:35 UTC 2020
On 7/13/20 8:03 PM, Prathamesh Chavan wrote:
> To remove dependecy of `qemuDomainJob` on job specific
> paramters, a `privateData` pointer is introduced.
> To handle it, structure of callback functions is
> also introduced.
>
> Signed-off-by: Prathamesh Chavan <pc44800 at gmail.com>
> ---
> The static callback functions structure was declared in `qemu_domain.c`
> Due to this, all callback functions (present in `qemu_domainobj.c`)
> were exposed using the .h file to allow their access by the
> `qemu_domain.c` file.
>
Yeah, well callbacks should be static thus should live in the
qemu_domain.c too. Also, the private data struct is defined in
qemu_domain.h and thus callbacks which allocate/free/format/parse should
live in the corresponding .c file.
> src/qemu/qemu_domain.c | 9 ++-
> src/qemu/qemu_domain.h | 10 ++++
> src/qemu/qemu_domainjob.c | 94 ++++++++++++++++++++++++--------
> src/qemu/qemu_domainjob.h | 44 ++++++++++++---
> src/qemu/qemu_driver.c | 3 +-
> src/qemu/qemu_migration.c | 28 +++++++---
> src/qemu/qemu_migration_params.c | 9 ++-
> src/qemu/qemu_process.c | 15 +++--
> 8 files changed, 165 insertions(+), 47 deletions(-)
This looks almost perfect.
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 10d2033db1..a02865dc59 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -88,6 +88,13 @@ VIR_ENUM_IMPL(qemuDomainNamespace,
> "mount",
> );
>
> +static qemuDomainObjPrivateJobCallbacks qemuPrivateJobCallbacks = {
> + .allocJobPrivate = &qemuJobAllocPrivate,
> + .freeJobPrivate = &qemuJobFreePrivate,
> + .formatJob = &qemuDomainFormatJobPrivate,
> + .parseJob = &qemuDomainParseJobPrivate,
> +};
I think we will need one callback more - resetJobPrivate. More below.
Also, there is no need to put ampersand before functions. Compilers are
wise enough to tell that we need the address of the functions.
> +
> /**
> * qemuDomainObjFromDomain:
> * @domain: Domain pointer that has to be looked up
> @@ -1585,7 +1592,7 @@ qemuDomainObjPrivateAlloc(void *opaque)
> if (VIR_ALLOC(priv) < 0)
> return NULL;
>
> - if (qemuDomainObjInitJob(&priv->job) < 0) {
> + if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to init qemu driver mutexes"));
> goto error;
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index e524fd0002..bb9b414a46 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -492,6 +492,16 @@ struct _qemuDomainXmlNsDef {
> char **capsdel;
> };
>
> +typedef struct _qemuDomainJobPrivate qemuDomainJobPrivate;
> +typedef qemuDomainJobPrivate *qemuDomainJobPrivatePtr;
> +struct _qemuDomainJobPrivate {
> + bool spiceMigration; /* we asked for spice migration and we
> + * should wait for it to finish */
> + bool spiceMigrated; /* spice migration completed */
> + bool dumpCompleted; /* dump completed */
> + qemuMigrationParamsPtr migParams;
> +};
> +
> int qemuDomainObjStartWorker(virDomainObjPtr dom);
> void qemuDomainObjStopWorker(virDomainObjPtr dom);
>
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index d96d5334a3..fe160afe79 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
> @@ -159,23 +159,32 @@ qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
> virObjectEventStateQueue(driver->domainEventState, event);
> }
>
> -
> -int
> -qemuDomainObjInitJob(qemuDomainJobObjPtr job)
> +void *
> +qemuJobAllocPrivate(void)
> {
> - memset(job, 0, sizeof(*job));
> -
> - if (virCondInit(&job->cond) < 0)
> - return -1;
> + qemuDomainJobPrivatePtr priv;
> + if (VIR_ALLOC(priv) < 0)
> + return NULL;
> + return (void *)priv;
Or plain: g_new0(qemuDomainJobPrivate, 1)
> +}
>
> - if (virCondInit(&job->asyncCond) < 0) {
> - virCondDestroy(&job->cond);
> - return -1;
> - }
>
> - return 0;
> +static void
> +qemuJobFreePrivateData(qemuDomainJobPrivatePtr priv)
> +{
> + priv->spiceMigration = false;
> + priv->spiceMigrated = false;
> + priv->dumpCompleted = false;
> + qemuMigrationParamsFree(priv->migParams);
> + priv->migParams = NULL;
See, this is what previously qemuDomainObjResetAsyncJob() was doing. And
while qemuDomainObjResetAsyncJob() is called from qemuDomainObjFreeJob()
it is also called from qemuDomainObjBeginJobInternal() and other places.
Therefore, naming this Free() is probably not the best idea. It should
be named qemuJobResetPrivate() or something, esp. beacuse it doesn't
really free the private data (it doesn't call free(priv)), it just
clears/resets them.
> }
>
> +void
> +qemuJobFreePrivate(void *opaque)
> +{
> + qemuDomainJobObjPtr job = (qemuDomainJobObjPtr) opaque;
> + qemuJobFreePrivateData(job->privateData);
And this will actually free the data.
> +}
>
> static void
> qemuDomainObjResetJob(qemuDomainJobObjPtr job)
> @@ -207,14 +216,11 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObjPtr job)
> job->phase = 0;
> job->mask = QEMU_JOB_DEFAULT_MASK;
> job->abortJob = false;
> - job->spiceMigration = false;
> - job->spiceMigrated = false;
> - job->dumpCompleted = false;
> VIR_FREE(job->error);
> g_clear_pointer(&job->current, qemuDomainJobInfoFree);
> - qemuMigrationParamsFree(job->migParams);
> - job->migParams = NULL;
> + job->cb->freeJobPrivate(job);
This needs to call the resetJobPrivate() callback then.
> job->apiFlags = 0;
> + job->cb = NULL;
Ooops, we definitely do not want to this. Because if we did, then the
second time this is called we would dereference a NULL pointer when
trying to call any of the callbacks (resetJobPrivate() for instance).
> }
>
> void
> @@ -229,7 +235,7 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj,
> job->asyncJob = priv->job.asyncJob;
> job->asyncOwner = priv->job.asyncOwner;
> job->phase = priv->job.phase;
> - job->migParams = g_steal_pointer(&priv->job.migParams);
> + job->privateData = g_steal_pointer(&priv->job.privateData);
Okay, I have to admit, this took me a while to figure out. And it is
sort of corect. BUT, after this the priv->job.privateData is going to be
NULL (because that's how g_steal_pointer() works). And I don't think the
code is prepared for that.
So what we need to do is to allocate new private data here.
> job->apiFlags = priv->job.apiFlags;
>
> qemuDomainObjResetJob(&priv->job);
> @@ -1239,14 +1245,24 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf,
> return 0;
> }
>
> +int
> +qemuDomainFormatJobPrivate(virBufferPtr buf,
> + qemuDomainJobObjPtr job)
> +{
> + qemuDomainJobPrivatePtr priv = job->privateData;
> + if (priv->migParams)
> + qemuMigrationParamsFormat(buf, priv->migParams);
> + return 0;
> +}
>
> int
> qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
> virDomainObjPtr vm)
> {
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + qemuDomainJobObjPtr jobObj = &priv->job;
> g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
> g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
> - qemuDomainObjPrivatePtr priv = vm->privateData;
> qemuDomainJob job = priv->job.active;
>
> if (!qemuDomainTrackJob(job))
> @@ -1273,8 +1289,8 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
> qemuDomainObjPrivateXMLFormatNBDMigration(&childBuf, vm) < 0)
> return -1;
>
> - if (priv->job.migParams)
> - qemuMigrationParamsFormat(&childBuf, priv->job.migParams);
> + if (jobObj->cb->formatJob(&childBuf, jobObj) < 0)
> + return -1;
>
> virXMLFormatElement(buf, "job", &attrBuf, &childBuf);
>
> @@ -1366,12 +1382,22 @@ qemuDomainObjPrivateXMLParseJobNBD(virDomainObjPtr vm,
> return 0;
> }
>
> +int
> +qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
> + qemuDomainJobObjPtr job)
> +{
> + qemuDomainJobPrivatePtr jobPriv = job->privateData;
> + if (qemuMigrationParamsParse(ctxt, &jobPriv->migParams) < 0)
> + return -1;
> + return 0;
> +}
>
> int
> qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm,
> xmlXPathContextPtr ctxt)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> + qemuDomainJobObjPtr job = &priv->job;
> VIR_XPATH_NODE_AUTORESTORE(ctxt);
> g_autofree char *tmp = NULL;
>
> @@ -1420,8 +1446,32 @@ qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm,
> if (qemuDomainObjPrivateXMLParseJobNBD(vm, ctxt) < 0)
> return -1;
>
> - if (qemuMigrationParamsParse(ctxt, &priv->job.migParams) < 0)
> + if (job->cb->parseJob(ctxt, job) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +int
> +qemuDomainObjInitJob(qemuDomainJobObjPtr job,
> + qemuDomainObjPrivateJobCallbacksPtr cb)
> +{
> + memset(job, 0, sizeof(*job));
> + job->cb = cb;
> +
> + if (!(job->privateData = job->cb->allocJobPrivate())) {
> + qemuDomainObjFreeJob(job);
This call is needless - alloc failed there is nothing to free.
> + return -1;
> + }
> +
> + if (virCondInit(&job->cond) < 0)
> + return -1;
> +
> + if (virCondInit(&job->asyncCond) < 0) {
> + virCondDestroy(&job->cond);
> return -1;
But if any of these calls fail, then we have allocated data the needs
freeing.
I think we want this to be squashed in:
diff --git c/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
index a02865dc59..b0ed562d25 100644
--- c/src/qemu/qemu_domain.c
+++ w/src/qemu/qemu_domain.c
@@ -88,11 +88,72 @@ VIR_ENUM_IMPL(qemuDomainNamespace,
"mount",
);
+
+static void *
+qemuJobAllocPrivate(void)
+{
+ return g_new0(qemuDomainJobPrivate, 1);
+}
+
+
+static void
+qemuJobFreePrivate(void *opaque)
+{
+ qemuDomainJobPrivatePtr priv = opaque;
+
+ if (!priv)
+ return;
+
+ qemuMigrationParamsFree(priv->migParams);
+ VIR_FREE(priv);
+}
+
+
+static void
+qemuJobResetPrivate(void *opaque)
+{
+ qemuDomainJobPrivatePtr priv = opaque;
+
+ priv->spiceMigration = false;
+ priv->spiceMigrated = false;
+ priv->dumpCompleted = false;
+ qemuMigrationParamsFree(priv->migParams);
+ priv->migParams = NULL;
+}
+
+
+static int
+qemuDomainFormatJobPrivate(virBufferPtr buf,
+ qemuDomainJobObjPtr job)
+{
+ qemuDomainJobPrivatePtr priv = job->privateData;
+
+ if (priv->migParams)
+ qemuMigrationParamsFormat(buf, priv->migParams);
+
+ return 0;
+}
+
+
+static int
+qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
+ qemuDomainJobObjPtr job)
+{
+ qemuDomainJobPrivatePtr priv = job->privateData;
+
+ if (qemuMigrationParamsParse(ctxt, &priv->migParams) < 0)
+ return -1;
+
+ return 0;
+}
+
+
static qemuDomainObjPrivateJobCallbacks qemuPrivateJobCallbacks = {
- .allocJobPrivate = &qemuJobAllocPrivate,
- .freeJobPrivate = &qemuJobFreePrivate,
- .formatJob = &qemuDomainFormatJobPrivate,
- .parseJob = &qemuDomainParseJobPrivate,
+ .allocJobPrivate = qemuJobAllocPrivate,
+ .freeJobPrivate = qemuJobFreePrivate,
+ .resetJobPrivate = qemuJobResetPrivate,
+ .formatJob = qemuDomainFormatJobPrivate,
+ .parseJob = qemuDomainParseJobPrivate,
};
/**
diff --git c/src/qemu/qemu_domainjob.c w/src/qemu/qemu_domainjob.c
index fe160afe79..64381d8f78 100644
--- c/src/qemu/qemu_domainjob.c
+++ w/src/qemu/qemu_domainjob.c
@@ -159,33 +159,6 @@ qemuDomainEventEmitJobCompleted(virQEMUDriverPtr
driver,
virObjectEventStateQueue(driver->domainEventState, event);
}
-void *
-qemuJobAllocPrivate(void)
-{
- qemuDomainJobPrivatePtr priv;
- if (VIR_ALLOC(priv) < 0)
- return NULL;
- return (void *)priv;
-}
-
-
-static void
-qemuJobFreePrivateData(qemuDomainJobPrivatePtr priv)
-{
- priv->spiceMigration = false;
- priv->spiceMigrated = false;
- priv->dumpCompleted = false;
- qemuMigrationParamsFree(priv->migParams);
- priv->migParams = NULL;
-}
-
-void
-qemuJobFreePrivate(void *opaque)
-{
- qemuDomainJobObjPtr job = (qemuDomainJobObjPtr) opaque;
- qemuJobFreePrivateData(job->privateData);
-}
-
static void
qemuDomainObjResetJob(qemuDomainJobObjPtr job)
{
@@ -218,12 +191,12 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObjPtr job)
job->abortJob = false;
VIR_FREE(job->error);
g_clear_pointer(&job->current, qemuDomainJobInfoFree);
- job->cb->freeJobPrivate(job);
+ job->cb->resetJobPrivate(job->privateData);
job->apiFlags = 0;
- job->cb = NULL;
}
-void
+
+int
qemuDomainObjRestoreJob(virDomainObjPtr obj,
qemuDomainJobObjPtr job)
{
@@ -238,8 +211,13 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj,
job->privateData = g_steal_pointer(&priv->job.privateData);
job->apiFlags = priv->job.apiFlags;
+ if (!(priv->job.privateData = priv->job.cb->allocJobPrivate()))
+ return -1;
+ job->cb = priv->job.cb;
+
qemuDomainObjResetJob(&priv->job);
qemuDomainObjResetAsyncJob(&priv->job);
+ return 0;
}
void
@@ -247,6 +225,7 @@ qemuDomainObjFreeJob(qemuDomainJobObjPtr job)
{
qemuDomainObjResetJob(job);
qemuDomainObjResetAsyncJob(job);
+ job->cb->freeJobPrivate(job->privateData);
g_clear_pointer(&job->current, qemuDomainJobInfoFree);
g_clear_pointer(&job->completed, qemuDomainJobInfoFree);
virCondDestroy(&job->cond);
@@ -1245,15 +1224,6 @@
qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf,
return 0;
}
-int
-qemuDomainFormatJobPrivate(virBufferPtr buf,
- qemuDomainJobObjPtr job)
-{
- qemuDomainJobPrivatePtr priv = job->privateData;
- if (priv->migParams)
- qemuMigrationParamsFormat(buf, priv->migParams);
- return 0;
-}
int
qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
@@ -1382,16 +1352,6 @@
qemuDomainObjPrivateXMLParseJobNBD(virDomainObjPtr vm,
return 0;
}
-int
-qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
- qemuDomainJobObjPtr job)
-{
- qemuDomainJobPrivatePtr jobPriv = job->privateData;
- if (qemuMigrationParamsParse(ctxt, &jobPriv->migParams) < 0)
- return -1;
- return 0;
-}
-
int
qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm,
xmlXPathContextPtr ctxt)
@@ -1460,15 +1420,16 @@ qemuDomainObjInitJob(qemuDomainJobObjPtr job,
memset(job, 0, sizeof(*job));
job->cb = cb;
- if (!(job->privateData = job->cb->allocJobPrivate())) {
- qemuDomainObjFreeJob(job);
+ if (!(job->privateData = job->cb->allocJobPrivate()))
return -1;
- }
- if (virCondInit(&job->cond) < 0)
+ if (virCondInit(&job->cond) < 0) {
+ job->cb->freeJobPrivate(job->privateData);
return -1;
+ }
if (virCondInit(&job->asyncCond) < 0) {
+ job->cb->freeJobPrivate(job->privateData);
virCondDestroy(&job->cond);
return -1;
}
diff --git c/src/qemu/qemu_domainjob.h w/src/qemu/qemu_domainjob.h
index b0e840adc8..d2c599cd68 100644
--- c/src/qemu/qemu_domainjob.h
+++ w/src/qemu/qemu_domainjob.h
@@ -159,6 +159,7 @@ typedef qemuDomainJobObj *qemuDomainJobObjPtr;
typedef void *(*qemuDomainObjPrivateJobAlloc)(void);
typedef void (*qemuDomainObjPrivateJobFree)(void *);
+typedef void (*qemuDomainObjPrivateJobReset)(void *);
typedef int (*qemuDomainObjPrivateJobFormat)(virBufferPtr,
qemuDomainJobObjPtr);
typedef int (*qemuDomainObjPrivateJobParse)(xmlXPathContextPtr,
@@ -169,6 +170,7 @@ typedef qemuDomainObjPrivateJobCallbacks
*qemuDomainObjPrivateJobCallbacksPtr;
struct _qemuDomainObjPrivateJobCallbacks {
qemuDomainObjPrivateJobAlloc allocJobPrivate;
qemuDomainObjPrivateJobFree freeJobPrivate;
+ qemuDomainObjPrivateJobReset resetJobPrivate;
qemuDomainObjPrivateJobFormat formatJob;
qemuDomainObjPrivateJobParse parseJob;
};
@@ -248,8 +250,8 @@ void qemuDomainObjSetJobPhase(virQEMUDriverPtr driver,
int phase);
void qemuDomainObjSetAsyncJobMask(virDomainObjPtr obj,
unsigned long long allowedJobs);
-void qemuDomainObjRestoreJob(virDomainObjPtr obj,
- qemuDomainJobObjPtr job);
+int qemuDomainObjRestoreJob(virDomainObjPtr obj,
+ qemuDomainJobObjPtr job);
void qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver,
virDomainObjPtr obj);
void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj);
@@ -291,17 +293,3 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
int
qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm,
xmlXPathContextPtr ctxt);
-
-void *
-qemuJobAllocPrivate(void);
-
-void
-qemuJobFreePrivate(void *opaque);
-
-int
-qemuDomainFormatJobPrivate(virBufferPtr buf,
- qemuDomainJobObjPtr job);
-
-int
-qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
- qemuDomainJobObjPtr job);
Michal
More information about the libvir-list
mailing list