[GSoC][PATCH v4 3/4] qemu_domainjob: introduce `privateData` for `qemuDomainJob`
Martin Kletzander
mkletzan at redhat.com
Tue Jul 14 14:28:53 UTC 2020
On Mon, Jul 13, 2020 at 11:33:40PM +0530, 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.
>
> 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(-)
>
So the biggest issue I have with this is that it is impossible to see which of
these functions are going to end up in `src/hypervisor/virjob.c` and which of
them will stay qemu-specific in `src/qemu/qemu_domainjob.c`.
I would suggest either:
1) marking them as such in some understandable way or
2) leaving qemu-specific code in `qemu_driver.c`
3) creating yet another file for what is going to become `virjob.c`
Otherwise I'm getting confused with some of the things, why are they there etc.
>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,
>+};
>+
> /**
> * 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;
>+}
>
>- 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;
> }
>
>+void
>+qemuJobFreePrivate(void *opaque)
>+{
>+ qemuDomainJobObjPtr job = (qemuDomainJobObjPtr) opaque;
>+ qemuJobFreePrivateData(job->privateData);
>+}
>
> 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);
If this function is supposed to be in `virjob.c`, then the callback function
should be just called with `job->privateData` which will help you drop one level
of indirection by skipping qemuJobFreePrivate() and calling
qemuJobFreePrivateData() directly. The function would take `void *opaque`.
[...]
Skipping rest of the diff from `qemu_domainjob.c` due to the confusion described
above, I'll illustrate it on the header file below.
>diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h
>index 9d2ee14584..b0e840adc8 100644
>--- a/src/qemu/qemu_domainjob.h
>+++ b/src/qemu/qemu_domainjob.h
>@@ -156,6 +156,23 @@ qemuDomainJobInfoCopy(qemuDomainJobInfoPtr info);
>
> typedef struct _qemuDomainJobObj qemuDomainJobObj;
> typedef qemuDomainJobObj *qemuDomainJobObjPtr;
>+
>+typedef void *(*qemuDomainObjPrivateJobAlloc)(void);
>+typedef void (*qemuDomainObjPrivateJobFree)(void *);
>+typedef int (*qemuDomainObjPrivateJobFormat)(virBufferPtr,
>+ qemuDomainJobObjPtr);
>+typedef int (*qemuDomainObjPrivateJobParse)(xmlXPathContextPtr,
>+ qemuDomainJobObjPtr);
>+
>+typedef struct _qemuDomainObjPrivateJobCallbacks qemuDomainObjPrivateJobCallbacks;
>+typedef qemuDomainObjPrivateJobCallbacks *qemuDomainObjPrivateJobCallbacksPtr;
>+struct _qemuDomainObjPrivateJobCallbacks {
>+ qemuDomainObjPrivateJobAlloc allocJobPrivate;
>+ qemuDomainObjPrivateJobFree freeJobPrivate;
>+ qemuDomainObjPrivateJobFormat formatJob;
>+ qemuDomainObjPrivateJobParse parseJob;
>+};
>+
So this is going to be part of `virjob.c`, right?
> struct _qemuDomainJobObj {
> virCond cond; /* Use to coordinate jobs */
>
>@@ -182,14 +199,11 @@ struct _qemuDomainJobObj {
> qemuDomainJobInfoPtr current; /* async job progress data */
> qemuDomainJobInfoPtr completed; /* statistics data of a recently completed job */
> bool abortJob; /* abort of the job requested */
>- bool spiceMigration; /* we asked for spice migration and we
>- * should wait for it to finish */
>- bool spiceMigrated; /* spice migration completed */
> char *error; /* job event completion error */
>- bool dumpCompleted; /* dump completed */
>-
>- qemuMigrationParamsPtr migParams;
> unsigned long apiFlags; /* flags passed to the API which started the async job */
>+
>+ void *privateData; /* job specific collection of data */
>+ qemuDomainObjPrivateJobCallbacksPtr cb;
> };
>
Same as this.
> const char *qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
>@@ -264,7 +278,9 @@ bool qemuDomainTrackJob(qemuDomainJob job);
>
> void qemuDomainObjFreeJob(qemuDomainJobObjPtr job);
>
>-int qemuDomainObjInitJob(qemuDomainJobObjPtr job);
>+int
>+qemuDomainObjInitJob(qemuDomainJobObjPtr job,
>+ qemuDomainObjPrivateJobCallbacksPtr cb);
>
> bool qemuDomainJobAllowed(qemuDomainJobObjPtr jobs, qemuDomainJob newJob);
>
>@@ -275,3 +291,17 @@ 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);
But these are going to stay in the qemu code, IIRC. See why it is confusing?
The rest looks fine from a quick look. Let's see what others think.
Have a nice day,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200714/5026fce5/attachment-0001.sig>
More information about the libvir-list
mailing list