LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

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

To: Julius Volz <juliusv@xxxxxxxxxx>
Subject: Re: [PATCH 00/26] IPVS: Add first IPv6 support to IPVS.
Cc: Simon Horman <horms@xxxxxxxxxxxx>, Vince Busam <vbusam@xxxxxxxxxx>, Ben Greear <greearb@xxxxxxxxxxxxxxx>, lvs-devel@xxxxxxxxxxxxxxx, netdev@xxxxxxxxxxxxxxx
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Tue, 17 Jun 2008 22:08:08 +0200
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.
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>