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

On Tue, Aug 04, 2020 at 08:06:45PM +0530, Prathamesh Chavan wrote: 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 ... 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