On Tue, Jul 14, 2020 at 7:59 PM Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:
>
> 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.
>
But doing this rightaway would mean we are again moving back
somestuff, which we just brought in this file. (Such as the
PrivateXML[Parse,Format]Job functions).
What I had in mind, was to deal with this segregation, while the
creation of the new file happens. But for more clarity, we can have
the code reside in the same file, and spilt into two sections, where
all the code residing in the beginning of the file (all the structs
and functions) will be the ones, which we plan on moving, and all the
code residing in the lower half, will be the one which will persist.
The other way of dealing this could be to move these things back to
`qemu_domain.c`
> >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`.
>
Yes, thanks for pointing this out. I think while splitting the patch,
I rewrote some parts,
and this slipped in again. I remember Erik had pointed this out earlier.
> [...]
>
> 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?
Yes.
>
> > 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.
>
Yes, even this shall be moved to the common jobs file and the
structures will be renamed accordingly.
> > 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
