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

Re: [libvirt] [PATCH] Removed function virDomainDefNewFull.



On 05/30/2016 04:21 AM, Ján Tomko wrote:
> On Fri, May 27, 2016 at 04:20:09PM +0200, Tomáš Ryšavý wrote:
>> The function virDomainDefNewFull() in src/conf/domain_conf.c was a thin
>> wrapper around virDomainDefNew() that was only used in a few places in
>> the code. The function was removed and the callers were re-implemented.
>> ---
> 
> What is the motivation for this change?
> 

I'm guessing it came from:

http://wiki.libvirt.org/page/BiteSizedTasks#Remove_virDomainDefNewFull.28.29

I added that entry. My motivation was that the function doesn't add much: just
saves a small amount of code total, while adding to the already quite large
domain_conf.c internal API. The name is DefNewFull is a little weird... it's
not all that 'Full', just tracks the metadata bits.

Plus outside of the vz usage it's largely about facilitating a pattern in the
xen code to use the virDomainDefPtr as a structure to pass around (name, uuid,
id) and nothing else, which I find weird. grep for 'minidef' in xen_driver.c
for examples of how the code tries to document that distinction. I'd rather
see that pattern go away, or the function made private to xen code, than to
facilitate it with common code.

All that that said this is minor and it was largely just an idea for the
BiteSizedTasks page, so if the above doesn't convince feel free to remove that
task item from the wiki :)

Thanks,
Cole

> Personally, I would rather keep the thin wrapper than open code it
> everywhere.
> 
> Jan
> 
>>  src/conf/domain_conf.c   | 22 ----------------------
>>  src/conf/domain_conf.h   |  3 ---
>>  src/libvirt_private.syms |  1 -
>>  src/vz/vz_utils.c        |  8 +++++++-
>>  src/xen/xen_hypervisor.c | 26 ++++++++++++++++++--------
>>  src/xen/xend_internal.c  | 23 +++++++++++++++++++----
>>  src/xen/xm_internal.c    | 24 ++++++++++++++++++++++--
>>  7 files changed, 66 insertions(+), 41 deletions(-)
>>
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


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