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@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
>
