[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 10/11] Unmerge attach/update/modify device APIs in drivers



On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> The LXC, QEMU, and LibXL drivers have all merged their handling of
> the attach/update/modify device APIs into one large
> 
>   'xxxxDomainModifyDeviceFlags'
> 
> which then does a 'switch()' based on the actual API being invoked.
> While this saves some lines of code, it is not really all that
> significant in the context of the driver API impls as a whole.
> 
> This merger of the handling of different APIs creates pain when
> wanting to automated analysis of the code and do things which
> are specific to individual APIs. The slight duplication of code
> from unmerged the API impls, is preferrable to allow for easier
> automated analysis.
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  src/libxl/libxl_driver.c | 241 +++++++++++++++++++++++++++++-------
>  src/lxc/lxc_driver.c     | 278 ++++++++++++++++++++++++++++++++---------
>  src/qemu/qemu_driver.c   | 316 ++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 671 insertions(+), 164 deletions(-)

ACK.  Looks huge, but it is mostly copy and paste of the existing code,
followed by flattening out the switch back into the per-API call.  And I
agree that not multiplexing the code will make it easier to add ACL that
allow device updates differently than device unplug.

I still think you could reduce the overall code size by having helper
functions for common tasks (such as flag sanitization of LIVE vs.
CONFIG), instead of verbatim repetition of large preambles, but that can
be separate patches.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]