[libvirt] [PATCH v2 06/10] Make qemu attach/detach functions public

[Martin Kletzander]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160727/5f71046c/attachment-0001.sig>