[GSoC][PATCH 1/7] qemu_domain: Added `qemuDomainJobInfo` to domainJob's `privateData`

[Prathamesh Chavan]

Sorry for not noticing this earlier, but the movement needs to happen
together with changes in codes as the moved code is no longer
available to be directly accessed by `qemu_domainjob`. (If we include
`qemu_domain.h` in `qemu_domainjob.h`, a cyclic dependency will get
created).

Thanks,
Prathamesh Chavan

On Mon, Aug 10, 2020 at 6:36 PM Erik Skultety <eskultet at redhat.com> wrote:
>
> On Tue, Aug 04, 2020 at 08:06:43PM +0530, Prathamesh Chavan wrote:
> > As `qemuDomainJobInfo` had attributes specific to qemu hypervisor's
> > jobs, we moved the attribute `current` and `completed` from
> > `qemuDomainJobObj` to its `privateData` structure.
> >
> > In this process, two callback functions: `setJobInfoOperation`
> > and `currentJobInfoInit` were introduced to qemuDomainJob's
> > callback structure.
> >
> > Signed-off-by: Prathamesh Chavan <pc44800 at gmail.com>
> > ---
> >  src/qemu/qemu_backup.c           |  22 +-
> >  src/qemu/qemu_domain.c           | 498 +++++++++++++++++++++++++++++++
> >  src/qemu/qemu_domain.h           |  74 +++++
> >  src/qemu/qemu_domainjob.c        | 483 +-----------------------------
> >  src/qemu/qemu_domainjob.h        |  81 +----
> >  src/qemu/qemu_driver.c           |  49 +--
> >  src/qemu/qemu_migration.c        |  62 ++--
> >  src/qemu/qemu_migration_cookie.c |   8 +-
> >  src/qemu/qemu_process.c          |  32 +-
> >  9 files changed, 680 insertions(+), 629 deletions(-)
>
> This patch does IMO too much, moving qemuDomainJobInfo struct to qemu_domain.h
> moving a bunch of functions that depend on the qemuDomainJobInfo structure to
> qemu_domain.c, moving attributes "current" and "completely" to a different
> structure, and introducing new callbacks. This caused the moved code to be
> changed in the same step in order to reflect the attribute movement.
> To illustrate this:
>
> ...
> > +void
> > +qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
> > +                                virDomainObjPtr vm)
> > +{
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +    qemuDomainJobPrivatePtr jobPriv = priv->job.privateData;
>
> ^This line was changed during the code movement
>
> > +    virObjectEventPtr event;
> > +    virTypedParameterPtr params = NULL;
> > +    int nparams = 0;
> > +    int type;
> > +
> > +    if (!jobPriv->completed)
> > +        return;
>
> ^These 2 as well...
>
> > +
> > +    if (qemuDomainJobInfoToParams(jobPriv->completed, &type,
>
> ^This one too...
>
> When doing code movements, it's a better idea to first move the code and then
> perform the changes, it's easier for the reviewer as well as the one looking at
> the commit history.
> You should be able to move the affected functions that need the
> qemuDomainJobInfo structure along with the structure in one patch and then in
> another patch move the attributes "current" and "completely" to a different
> place and adjust the code accordingly.
> If for some reason moving the qemuDomainJobInfo structure in the first patch
> caused issues for the follow-up patch moving the attributes, then since
> qemu_domain.h includes qemu_domainjob.h you could leave the qemuDomainJobInfo
> structure movement out of the first patch and move in the second one.
>
>
> > +                                  &params, &nparams) < 0) {
> > +        VIR_WARN("Could not get stats for completed job; domain %s",
> > +                 vm->def->name);
> > +    }
> > +
> > +    event = virDomainEventJobCompletedNewFromObj(vm, params, nparams);
> > +    virObjectEventStateQueue(driver->domainEventState, event);
> > +}
>
> ...
>
>
>
> >  static int
> >  qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
> > @@ -140,6 +636,8 @@ static qemuDomainObjPrivateJobCallbacks qemuPrivateJobCallbacks = {
> >      .resetJobPrivate = qemuJobResetPrivate,
> >      .formatJob = qemuDomainFormatJobPrivate,
> >      .parseJob = qemuDomainParseJobPrivate,
> > +    .setJobInfoOperation = qemuDomainJobInfoSetOperation,
>
> ^This would probably be better called jobInfoSetOperation
>
> > +    .currentJobInfoInit = qemuDomainCurrentJobInfoInit,
>
> As you've established in the commit message itself "current" and "completed"
> are QEMU specific, so the callback should therefore be called jobInfoInit or
> maybe jobInfoNew.
>
> Erik
>