On Fri, Jul 10, 2020 at 8:06 PM Michal Privoznik <mprivozn@redhat.com> 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@gmail.com> --- 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