Le 10/05/2016 11:40, Lars Ellenberg a écrit :

On Tue, May 10, 2016 at 11:09:53AM +0200, Nicolas Dichtel wrote:

quoted

Le 09/05/2016 15:15, Lars Ellenberg a écrit :

quoted

On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:

[snip]

quoted

quoted

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.

quoted

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 ;-)

quoted

 - 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