Re: [libvirt] [PATCH v2 06/10] Make qemu attach/detach functions public
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 guess
Did 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
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)