On Fri, Jul 10, 2020 at 8:06 PM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
>
> On 7/10/20 9:11 AM, 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>
> > ---
> > src/qemu/qemu_domain.c | 4 +-
> > src/qemu/qemu_domain.h | 10 +++++
> > src/qemu/qemu_domainjob.c | 74 ++++++++++++++++++++++----------
> > src/qemu/qemu_domainjob.h | 32 ++++++++------
> > 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, 119 insertions(+), 56 deletions(-)
>
> Almost.
>
> >
> > +typedef void *(*qemuDomainObjPrivateJobAlloc)(void);
> > +typedef void (*qemuDomainObjPrivateJobFree)(void *);
> > +typedef int (*qemuDomainObjPrivateJobFormat)(virBufferPtr,
> > + virDomainObjPtr);
> > +typedef int (*qemuDomainObjPrivateJobParse)(virDomainObjPtr,
> > + xmlXPathContextPtr);
> > +
> > +typedef struct _qemuDomainObjPrivateJobCallbacks qemuDomainObjPrivateJobCallbacks;
> > +struct _qemuDomainObjPrivateJobCallbacks {
> > + qemuDomainObjPrivateJobAlloc allocJobPrivate;
> > + qemuDomainObjPrivateJobFree freeJobPrivate;
> > + qemuDomainObjPrivateJobFormat formatJob;
> > + qemuDomainObjPrivateJobParse parseJob;
> > +};
> > +
>
> This looks correct.
>
> > typedef struct _qemuDomainJobObj qemuDomainJobObj;
> > typedef qemuDomainJobObj *qemuDomainJobObjPtr;
> > struct _qemuDomainJobObj {
> > @@ -182,14 +197,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 */
> > + qemuDomainObjPrivateJobCallbacks cb;
>
> And so does this. But these callbacks should be passed to
> qemuDomainObjInitJob() which will save them into the cb like this:
>
> int
> qemuDomainObjInitJob(qemuDomainJobObjPtr job,
> qemuDomainObjPrivateJobCallbacks cb)
> {
> memset(job, 0, sizeof(*job));
>
> if (virCondInit(&job->cond) < 0)
> return -1;
>
> if (virCondInit(&job->asyncCond) < 0) {
> virCondDestroy(&job->cond);
> return -1;
> }
>
> job->cb = cb;
>
> if (!(job->privateData = job->cb.allocJobPrivate())) {
> qemuDomainObjFreeJob(job);
> return -1;
> }
>
> return 0;
> }
>
>
> In general, nothing outside qemu_domainjob.c should be calling these
> callbacks. Therefore for instance when formatting private XML it should
> looks something like this:
>
> static int
> qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
> virDomainObjPtr vm)
> {
> ...
> if (qemuDomainJobFormat(buf, priv->job, vm) < 0)
> return -1;
> ...
> }
>
>
> This qemuDomainJobFormat() can look something like this:
>
> int
> qemuDomainObjJobFormat(virBufferPtr buf,
> qemuDomainJobObjPtr job,
> void *opaque)
> {
> g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
> g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>
> if (!qemuDomainTrackJob(job))
> job = QEMU_JOB_NONE;
>
> if (job == QEMU_JOB_NONE &&
> jobObj->asyncJob == QEMU_ASYNC_JOB_NONE)
> return 0;
>
> virBufferAsprintf(&attrBuf, " type='%s' async='%s'",
> qemuDomainJobTypeToString(job),
> qemuDomainAsyncJobTypeToString(jobObj->asyncJob));
>
> if (jobObj->phase) {
> virBufferAsprintf(&attrBuf, " phase='%s'",
> qemuDomainAsyncJobPhaseToString(jobObj->asyncJob,
> jobObj->phase));
> }
>
> if (jobObj->asyncJob != QEMU_ASYNC_JOB_NONE)
> virBufferAsprintf(&attrBuf, " flags='0x%lx'", jobObj->apiFlags);
>
> if (job->cb.formatJob(&childBuf, vm) < 0)
> return -1;
>
> virXMLFormatElement(buf, "job", &attrBuf, &childBuf);
>
> return 0;
> }
>
>
> This looks very close to qemuDomainObjPrivateXMLFormatJob() and that is
> exactly how we want it. Except all "private" data is now formatted using
> callback (migParams for instance). Does this make more sense?
>
Yes, it does. I can see how this will be helpful when we'll be moving
the domainjob functions into a common file.
But I still think that, since the function outside domain job should
be allowed to access this callback function, even the function:
qemuDomainFormatJob() needs to be moved to qemu_domainjob from
qemu_domain.
So it makes this series have 4 patches:
1. Changing the params of qemuDomainObjPrivateXMLFormatJob and ParseJob.
2. Moving these functions to qemu_domainjob
3. Creating privateData and callback functions structure for qemuDomainJob
4. Creating privateData and callback functions structure for qemuDomainJobInfo
Thanks,
Prathamesh Chavan
