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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

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?

Michal