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

Prathamesh Chavan pc44800 at gmail.com
Sun Jul 12 21:13:12 UTC 2020


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




More information about the libvir-list mailing list