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

[Prathamesh Chavan]

I have decided to go with the nested callback structure. I'll be
posting a patch related to the same soon.

On Wed, Aug 12, 2020 at 5:35 PM Erik Skultety <eskultet at redhat.com> wrote:
>
> 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
>