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@xxxxxxxxx> --- 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
Attachment:
signature.asc
Description: PGP signature
