Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages

[Nicolas Dichtel]

Le 10/05/2016 11:40, Lars Ellenberg a écrit :
> On Tue, May 10, 2016 at 11:09:53AM +0200, Nicolas Dichtel wrote:
>> Le 09/05/2016 15:15, Lars Ellenberg a écrit :
>>> On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
>> [snip]
>>>> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
>>>> could be interesting so that new module won't use it. What is your
>>>> opinion?
>>>
>>> This was supposed to not be DRBD specific.  But it might even still
>>> need some massaging before it was truly generic. And obviously,
>>> it does not meet the taste of genetlink folks, to say the least :(
>> Yes, this file is not generic and netlink APIs are never defined like this.
>> These tons of macro complexifies the code too much. It's overengineering for
>> what purpose?
> 
> If we introduce a new config option,
> we have to add it to the config scanner (one line),
> define min, max, default and scale (four short defines),
> and add it to the netlink definition here (one line).
> Done, rest of the code is generated,
> both on the kernel side,
> and on the drbd-utils side used to talk to the kernel.
> We found that to be very convenient.
Ok.

> 
>> Small examples:
>>  - the drbd netlink API is not exported via uapi (I wonder how apps using this
>>    API get it)
> 
> There used to be a time where there was no "uapi".
> (I wonder how apps ever worked back then).
At that time, include/linux/ was exported ;-)

> 
>>  - v2 of the patch is nacked because adding a new attribute may break existing
> 
> No.
> 
> But because the "new" attributes you chose have not been new,
> but already used (though not yet merged back into mainline yet).
> (Which you did not realize, and had no obvious way of knowing.
>  Could have been fixed.).
Ok.

> 
> And because your patch introduced useless new members to the structs.
> (Could also have been fixed).
> 
> And because I did not see any use defining that many new "padding attributes"
> for no reason, where the obvious (to me) choice was to use 0, and you
> did not even try to explain why that would have been a bad choice.
Because some nl APIs were wrongly use 0 as a valid attribute we make the choice
of always adding a new attribute for padding to be sure to not break existing API.
And yes, in drdb it does not seem to be the case.

> Is this going somewhere?
I'm just trying to understand things.


Regards,
Nicolas