On Wed, Jul 27, 2016 at 01:08:17AM +0200, Tomasz Flendrich wrote:
+int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn,+ virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *xml, + unsigned int flags); + +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xml, + unsigned int flags); + #endif /* __QEMU_DRIVER_H__ */This should not be exposed in qemu_driver.h, instead the functions should be moved to qemu_domain.h if possible, but they now fit there. If that's not possible, we need to do similar thing as we now have with qemu_processpriv.h, so this could be called qemu_domainpriv.h I guessDid you mean qemu_driverpriv.h?
I really meant 'domain' as I was thinking these are qemu_domain related, but since they live in qemu_driver.c let's do qemu_driverpriv.h in order not to complicate things.
As you predicted, it's not easily done. qemuDomainAttachDeviceLiveAndConfig() indirectly uses a few functions from qemu_hotplug. It would be pointless to move them all to qemu_domain.h, because then we would have to drag the whole qemu_hotplug.[hc] along. What can be done is moving these to qemu_hotplug.h: - qemuDomainAttachDeviceLive() - qemuDomainDetachDeviceLive(), and these to qemu_domain.h: - qemuDomainAttachDeviceConfig() - qemuDomainDetachDeviceConfig() - qemuDomainChrPreInsert() - qemuDomainChrInsertPreAlloced() - qemuDomainChrInsertPreAllocCleanup() - qemuDomainChrInsert() - qemuDomainChrRemove(). But then where should qemuDomainAttachDeviceLiveAndConfig() and qemuDomainDetachDeviceLiveAndConfig() be? In that qemu_domainpriv/qemu_driverpriv.h? And should their definitions still be in qemu_driver.c, or in a brand new .c file?
Hoinestly? I think the code should be split in all the drivers a bit differently. There should be xxx_driver.h as is, then xxx_driver.c with minimum functions (ideally just the xxxRegister one) and then all static methods from the xxx_driver.c should be moved to a different file, e.g. xxx_apis.c and those that are needed in the driver or tests would be made non-static without problems as it as its own header file. But that's not what was done or will happen, so let's not complicate it and leave it as is with just the private header for those functions needed in tests.
Btw qemuDomainUpdateDeviceFlags() is still unsplit and it deserves the same treatment as the other two *DeviceFlags functions. It will be done in v3.
Cool, I can't wait!
Thank you again for such thorough reviews! I really appreciate it. Tomasz
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list