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

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