[libvirt] [PATCH 09/10] conf: Provide missing virDomain*DefNew() functions

Ján Tomko jtomko at redhat.com
Fri Jun 30 12:22:54 UTC 2017


On Fri, Jun 30, 2017 at 12:34:15PM +0200, Andrea Bolognani wrote:
>On Fri, 2017-06-30 at 10:18 +0200, Ján Tomko wrote:
>> > Same as virDomainDeviceInfo itself, any struct that
>> > embeds it needs to be initialized properly before use;
>> > however, none of the structs in question even had a
>> > proper allocation function defined.
>>>> > Implement an allocation function for all structs
>> > embedding a virDomainDeviceInfo and use them instead
>> > of plain VIR_ALLOC() everywhere.
>> 
>> NACK
>> 
>> This is a pointless obfuscation
>
>Would you mind spending a few words to explain why you feel
>that's the case?
>
>Having a function where you perform both allocation and
>initialization of your data structure is a pretty common

As of now, nothing special is initialized in the structure.
If you are going to need initialize something in the future,
please mention it also in the commit message, not just the
cover letter. This makes the obfuscation pointful.

>pattern both outside and inside libvirt, and while it adds
>one layer of indirection it also improves flexibility and
>encapsulation, which makes it IMHO well worth it.

I am not a fan of adding code just in case we might need it
in the future, which is what this patch looks like to a lazy
reviewer that does not read cover letters for cleanups/refactors.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170630/338ea837/attachment-0001.sig>


More information about the libvir-list mailing list