Re: [GSoC][PATCH v4 3/4] qemu_domainjob: introduce `privateData` for `qemuDomainJob`

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(a)gmail.com&gt; >--- >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