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@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
>
>
> > + ¶ms, &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
>
