[GSoC][PATCH 3/7] qemu_domainjob: add `saveDomainStatus` as a callback function to jobs

[Erik Skultety]

On Tue, Aug 04, 2020 at 08:06:45PM +0530, Prathamesh Chavan wrote:
> The function `qemuDomainObjSaveStatus` required an access to
> `virQEMUDriverPtr`.
> To make jobs hypervisor-agnostic we remove this funciton
> and replace it with a callback function from `qemuDomainJob`
> 
> Removal of `virQEMUDriverPtr` as parameter resulted in
> removal of the same from function, where it was pass.
> All of such references were removed as the variable
> was no longer required.
> 
> Signed-off-by: Prathamesh Chavan <pc44800 at gmail.com>
> ---
>  src/qemu/qemu_backup.c           |  41 +-
>  src/qemu/qemu_backup.h           |   3 +-
>  src/qemu/qemu_block.c            |  45 +-
>  src/qemu/qemu_block.h            |   6 +-
>  src/qemu/qemu_blockjob.c         |  45 +-
>  src/qemu/qemu_blockjob.h         |   3 +-
>  src/qemu/qemu_checkpoint.c       |  29 +-
>  src/qemu/qemu_domain.c           |  78 ++-
>  src/qemu/qemu_domain.h           |  24 +-
>  src/qemu/qemu_domainjob.c        |  50 +-
>  src/qemu/qemu_domainjob.h        |  29 +-
>  src/qemu/qemu_driver.c           | 848 ++++++++++++++-----------------
>  src/qemu/qemu_hotplug.c          | 319 ++++++------
>  src/qemu/qemu_hotplug.h          |  30 +-
>  src/qemu/qemu_migration.c        | 315 +++++-------
>  src/qemu/qemu_migration.h        |  12 +-
>  src/qemu/qemu_migration_cookie.c |   7 +-
>  src/qemu/qemu_migration_params.c |  48 +-
>  src/qemu/qemu_migration_params.h |  15 +-
>  src/qemu/qemu_process.c          | 258 +++++-----
>  src/qemu/qemu_process.h          |  15 +-
>  tests/qemuhotplugtest.c          |   2 +-
>  22 files changed, 986 insertions(+), 1236 deletions(-)
>

Hi, I'm sorry for the delay, but I spent a while thinking about other
approaches to achieve the same what I'm commenting on below. I had to verify
every single idea by debugging libvirt so that I would not propose something
that was impossible to do and by doing that, I realized a very interesting
circular data reference we have:
    (QEMU)driver->xmlopt->config.priv->(QEMU)driver

... 

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 677fa7ea91..d7a944a886 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -634,6 +634,7 @@ static qemuDomainObjPrivateJobCallbacks qemuPrivateJobCallbacks = {
>      .allocJobPrivate = qemuJobAllocPrivate,
>      .freeJobPrivate = qemuJobFreePrivate,
>      .resetJobPrivate = qemuJobResetPrivate,
> +    .saveStatus = qemuDomainSaveStatus,
>      .formatJob = qemuDomainFormatJobPrivate,
>      .parseJob = qemuDomainParseJobPrivate,
>      .setJobInfoOperation =qemuDomainJobInfoSetOperation,

Okay, ^this does the job, it works, but I would call it the easy way out.
The qemuPrivateJobCallbacks structure hints that it contains callbacks specific
to job handling to which qemuDomainSaveStatus is simply not related at all.
It just so happens, that we have to save the domain status basically every time
we're doing something to the VM.
Structurally, I see 2 ways to achieve the same code extraction properly.
First, having another structure for callbacks which would nest the existing
qemuPrivateJobCallbacks, IOW:

struct _qemuDomainObjPrivateCallbacks {
    /* generic callbacks that we can't really categorize */
    qemuDomainObjPrivateSaveStatus saveStatus;

    /* Job related callbacks */
   qemuDomainObjPrivateJobCallbacks jobcb;
}

We'd then pass ^this structure instead of the qemuDomainObjPrivateJobCallbacks
one at the relevant places.

I don't like the ^this solution that much either, but I wanted to mention it.

I think what we need to do instead is to look what qemuDomainSaveStatus or
qemuDomainObjSaveStatus really need. They need to access the driver and its
config, that's it. In that perspective it relates to the virDomainObj's private
data.
Specifically for qemuDomainObjSaveStatus:

...
if (virDomainObjIsActive(obj))
...

^This check can easily be extracted to the virDomainObjSave function, there's
not reason why it should be specific to QEMU only.

...
if (virDomainObjSave(obj, driver->xmlopt, cfg->stateDir) < 0)
...

^This is the thing we need to call from the hypervisor-agnostic code, except we
don't have @driver (note that for example libxl doesn't have @driver as
part of the virDomainObj's private data).

Considering the above, we need a generic wrapper over virDomainObjSave, let's
call it virDomainDriverObjSave:

void virDomainDriverObjSave(virDomainObjPtr obj) {
    return obj->privateDataCallbacks.saveStatus(obj);
}

struct _virDomainObj {
    ...
    void *privateData;
    void (*privateDataFreeFunc) (void *);
    virDomainObjSaveStatusCallbackPtr saveStatus; <<<<<<<<<
    ...
};


The saveStatus callback would then have to live inside xmlopt callbacks and
be copied over in virDomainObjNew (just like we copy the free callback).
This is far from ideal, as it involves @xmlopt as we should not be interacting
with it, but we're already abusing the @xmlopt on so many places that it's such
an integral part of libvirt that refactoring how and where we use @xmlopt (see
the circular referencing above) is IMO beyond even a standalone GSoC project.

Alternatively in:
struct _virDomainObj {
    ...
    void *privateData;
    virDomainObjPrivateDataCallbacks cb;
    ...
}

and then

struct _virDomainObjPrivateDataCallbacks {
    void (*free) (void *);
    void (*saveStatus) (virDomainObjPtr);
}

However, ^this would break the consistency we use for freeing privateData in
object Dispose functions for example for StoragePools, Volumes, Domains, etc.
And since I am a fan of consistency, I would not favour ^this alternative.

Erik