Re: [PATCH 00/26] IPVS: Add first IPv6 support to IPVS.

[Patrick McHardy <kaber@trash.net>]

Julius Volz wrote:
> Ok, so this is my draft version of the IPVS Generic Netlink interface
> definition. I'm posting this to see if anyone notices general problems
> with it right away.

I'm not familiar with the ipvs interface itself, so I'll
stick to netlink related comments.

> Arrays of the same attribute type are always put into a nested container
> so that it is easy to add new attributes which are parallel to the array
> later on.

That makes sense.

> Perhaps integer flag fields should also be split up into
> NLA_FLAG attributes, haven't done that yet.

I personally don't find NLA_FLAG very useful since for
flags use usually want the flag and a mask, otherwise
you can't unset it without the convention that userspace
always includes it, even for change requests when it
doesn't want to change it. And that is unusual for netlink
and also needlessly complicated and racy in userspace since
you'd have to query the current value before sending a change
request.

> First is a text listing attribute types and how they occur and nest in
> all of the commands and their replies. After that are the corresponding
> source excerpts (no patch material yet).
> 
> Julius
> 
> 
> ======================================
> |    IPVS NETLINK ATTRIBUTE TYPES    |
> |          (grouped as enums)        |
> ======================================
> 
> IPVS_ENTRY_ATTR_SERVICE		- NLA_NESTED
> IPVS_ENTRY_ATTR_SERVICES	- NLA_NESTED
> IPVS_ENTRY_ATTR_DEST		- NLA_NESTED
> IPVS_ENTRY_ATTR_DESTS		- NLA_NESTED
> IPVS_ENTRY_ATTR_DAEMON		- NLA_NESTED
> IPVS_ENTRY_ATTR_DAEMONS		- NLA_NESTED

So these are lists I assume. I don't think we have any examples
of lists of nested attributes in the mainline kernel, but in
some similar (unsubmitted) code of mine I used (names adjusted):

IPVS_SERVICE_LIST		- NLA_NESTED
IPCS_DEST_LIST			- NLA_NESTED
IPVS_DAEMON_LIST		- NLA_NESTED

and

IPVS_LIST_ELEM			- NLA_NESTED

for list elements of every kind. Since you can only put one
kind of element in the lists anyway (I think), different
types don't allow any increased flexibility and the LIST
naming is more clear in my opinion.

> IPVS_SVC_ATTR_AF		- NLA_U32
> IPVS_SVC_ATTR_PROTOCOL		- NLA_U32
> IPVS_SVC_ATTR_ADDR		- union nf_inet_addr

This should probably use NLA_BINARY, which allows addresses
of any kind.

> IPVS_SVC_ATTR_PORT		- NLA_U16
> IPVS_SVC_ATTR_FWMARK		- NLA_U32
> IPVS_SVC_ATTR_SCHED_NAME	- NLA_STRING

NLA_NUL_STRING (at least for validation purposes)?

> IPVS_SVC_ATTR_FLAGS		- NLA_U32

As I mentioned above, you usually want a MASK in combination
with flags to allow to unset them. This is best done using
a structure.

> IPVS_SVC_ATTR_TIMEOUT		- NLA_U32
> IPVS_SVC_ATTR_NETMASK		- NLA_U32

Shouldn't this also be able to carry IPv6 masks?

> IPVS_SVC_ATTR_NUM_DESTS		- NLA_U32

Is this number related to the IPVS_ENTRY_ATTR_DESTS list?
If so, it shouldn't be contained as seperate attribute,
that just allows for potential inconsistency.

> IPVS_SVC_ATTR_STATS		- NLA_NESTED
> 
> IPVS_DEST_ATTR_AF		- NLA_U32

Doesn't the family have to be equal for service and dest?
If so, having it specified only once avoids potential
inconsistencies.

> ========================== include/net/ip_vs.h ==========================

Please put this under include/linux, this doesn't belong
here as its a public header.