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

[Erik Skultety]

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