Dear Willy, First of all, thanks a lot for your comments. Please find our comments inline.
On Fri, Sep 5, 2014 at 6:23 PM, Willy Tarreau <[email protected]> wrote: > Hi László, > > On Fri, Sep 05, 2014 at 10:18:25AM +0200, Sárközi, László wrote: > > Dear haproxy list, > > > > We've been working on a project that involves using haproxy to proxy > > between multiple networks represented as multiple network namespaces on > the > > proxy host. > > (...) > That's really great. I'm having some comments about this patch series > however, please find them inline with the patches : > > > diff --git a/include/types/server.h b/include/types/server.h > > index 94f9a0f..9d5c6ed 100644 > > --- a/include/types/server.h > > +++ b/include/types/server.h > > @@ -85,6 +85,8 @@ enum srv_admin { > > #define SRV_F_BACKUP 0x0001 /* this server is a backup > server */ > > #define SRV_F_MAPPORTS 0x0002 /* this server uses mapped > ports */ > > #define SRV_F_NON_STICK 0x0004 /* never add connections > allocated to this server to a stick table */ > > +#define SRV_F_USE_CONFIGURED_NAMESPACE 0x0010 > > +#define SRV_F_USE_NAMESPACE_FROM_PROXY_PROTOCOL 0x0020 > > Are you sure you couldn't find shorter names for these macros ? I'm > generally > not that much picky about this, but they're *really* large. Eg, please try > something along SRV_F_USE_CONF_NS and SRV_F_USE_NS_FROM_PP or something > like > this that sounds good to you. > Ok. Both of these seem OK for us. > > diff --git a/src/backend.c b/src/backend.c > > index a96b767..9b10cd3 100644 > > --- a/src/backend.c > > +++ b/src/backend.c > > @@ -719,6 +719,45 @@ int assign_server(struct session *s) > > return err; > > } > > > > +/* > > + * This function assigns a server connection network namespace. > > + * The network namespace is taken from the configuration file. > > + * If flag SRV_F_USE_NAMESPACE_FROM_PROXY_PROTOCOL is set server > connection > > + * relay the client network namespace. If flag > SRV_F_USE_CONFIGURED_NAMESPACE > > + * is set server connection namespace comes form configuration. > > + * > > + * It may return : > > + * SRV_STATUS_OK if everything is OK. > > + * SRV_STATUS_INTERNAL for other unrecoverable errors. > > + * > > + * Upon successful return, the connection flag CO_FL_NAMESPACE_SET and > > + * CO_FL_NAMESPACE_RECV and network namespace are set. > > + * > > + */ > > +int set_srv_conn_network_namespace(struct session *s, struct connection > *cli_conn, struct connection *srv_conn) > > +{ > > + if ((objt_server(s->target)->flags & > SRV_F_USE_NAMESPACE_FROM_PROXY_PROTOCOL) && > > + (objt_server(s->target)->flags & > SRV_F_USE_CONFIGURED_NAMESPACE) ) > > + return SRV_STATUS_INTERNAL; > > + > > + if ((objt_server(s->target)->flags & > SRV_F_USE_NAMESPACE_FROM_PROXY_PROTOCOL) && > > + (cli_conn->flags & CO_FL_NAMESPACE_SET)){ > > + > > + conn_clear_network_namespace(srv_conn); > > + srv_conn->network_namespace = > strdup(cli_conn->network_namespace); > > + srv_conn->flags |= CO_FL_NAMESPACE_SET; > > + srv_conn->flags |= CO_FL_NAMESPACE_RECV; > > + } > > + > > + if (objt_server(s->target)->flags & > SRV_F_USE_CONFIGURED_NAMESPACE){ > > + conn_clear_network_namespace(srv_conn); > > + > > + srv_conn->network_namespace = > (objt_server(s->target)->network_namespace); > > + srv_conn->flags |= CO_FL_NAMESPACE_SET; > > Please, use a single indent level above. > Ok. > > + } > > + > > + return SRV_STATUS_OK; > > +} > > > > /* > > * This function assigns a server address to a session, and sets > SN_ADDR_SET. > > @@ -802,6 +841,10 @@ int assign_server_address(struct session *s) > > return SRV_STATUS_INTERNAL; > > } > > > > + /* Configure network namespace */ > > + if(set_srv_conn_network_namespace(s,cli_conn,srv_conn) == > SRV_STATUS_INTERNAL) > > And similarly for the name, probably "set_srv_conn_netns" should be > enough for this function. > Ok. > > + return SRV_STATUS_INTERNAL; > > + > > s->flags |= SN_ADDR_SET; > > return SRV_STATUS_OK; > > } > > diff --git a/src/connection.c b/src/connection.c > > index 3af6d9a..e0b6172 100644 > > --- a/src/connection.c > > +++ b/src/connection.c > > @@ -217,6 +217,16 @@ void conn_update_sock_polling(struct connection *c) > > c->flags = f; > > } > > > > +void conn_clear_network_namespace(struct connection* conn) > > +{ > > + if((conn->flags & CO_FL_NAMESPACE_RECV) && > (conn->network_namespace != NULL)) > > + { > > + free(conn->network_namespace); > > + conn->network_namespace = NULL; > > + conn->flags &= ~CO_FL_NAMESPACE_RECV; > > + } > > It seems to me that this test is unnecessary and that the code could > be performed unconditionally. We'd better do it in this case, as history > has shown that many bugs arise from conditions that are improperly > converted when flags change later. > Well, there's a reason for that if here: there are two possible ways a namespace name could be set for a connection. The first is that there's a namespace configured for the bind stanza and the connection 'inherits' that namespace from the listener it arrived on. In this case, we store a pointer to the string in the bind structure. This obviously must not be freed, because it is part of the config. The other way is that a namespace TLV is received in the proxy protocol. In this case the connection structure *owns* the string it has a pointer to: basically this is what the CO_FL_NAMESPACE_RECV flag means. > > +} > > + > > /* This handshake handler waits a PROXY protocol header at the > beginning of the > > * raw data stream. The header looks like this : > > * > > diff --git a/src/haproxy.c b/src/haproxy.c > > index 13c3d26..df02401 100644 > > --- a/src/haproxy.c > > +++ b/src/haproxy.c > > @@ -1176,6 +1176,9 @@ void deinit(void) > > free(s->agent.bi); > > free(s->agent.bo); > > free((char*)s->conf.file); > > + if (s->network_namespace != NULL) { > > + free(s->network_namespace); > > + } > > The test is undesired and not needed, please simply call free(), it > performs the test itself. > Ok. > > free(s); > > s = s_next; > > }/* end while(s) */ > > @@ -1187,6 +1190,8 @@ void deinit(void) > > LIST_DEL(&l->by_bind); > > free(l->name); > > free(l->counters); > > + if(l->network_namespace != NULL) > > + free(l->network_namespace); > > same here. > Ok. > > free(l); > > } > > > > diff --git a/src/namespace.c b/src/namespace.c > > new file mode 100644 > > index 0000000..20d027d > > --- /dev/null > > +++ b/src/namespace.c > > @@ -0,0 +1,74 @@ > > +#include <common/namespace.h> > > + > > +#define _GNU_SOURCE > > + > > +#include <stdio.h> > > +#include <sys/stat.h> > > +#include <fcntl.h> > > +#include <sys/types.h> > > +#include <unistd.h> > > +#include <sys/socket.h> > > +#include <sched.h> > > (...) > > I have not found a way to prevent this file from being built since > namespace.o is unconditionally added to the Makefile's OBJS, but it > specifically relies on Linux. The best way generally consists in > adding a new option to the makefile just like for CTTPROXY, which > references the new file. For example : > > ifneq ($(USE_NS),) > OPTIONS_CFLAGS += -DCONFIG_HAP_NS ### only if needed > OPTIONS_OBJS += src/namespace.o > BUILD_OPTIONS += $(call ignore_implicit,USE_NS) > endif > Ok, we'll do so. > > + > > +static int open_named_namespace(const char *ns_name) { > > Please indicate the expected return values above so that new users do > proper error checking. I think that would be enough : > > /* Opens the namespace <ns_name> and returns the FD or -1 in case of > * error (check errno). > */ > Sure. > > + char buf[512]; > > + int fd; > > + > > + snprintf(buf, sizeof(buf), "/var/run/netns/%s", ns_name); > > Just out of curiosity, is it the location defined by LSB/FHS/whatever > standard > or is it on a specific distro ? We're seeing haproxy embedded into various > products and non-standard locations rarely follow the ones of "fat" > distros. > Well, there's generally very little documentation on this. The '/var/run/netns' directory is used by iproute, so the path above came from the need to be able to open namespaces created by 'ip netns add'. We'll check whether or not it can be changed during compilation. > Also, a test on the snprintf() result would be desired, since a user could > craft too large a string to cause it to fail and have open() try to open > anything (think about shared hosting environments where APIs are used to > let users add config sections for example). > Indeed, that's a good point. > > + fd = open(buf, O_RDONLY); > > + > > + return fd; > > Also, just a matter of style, above you changed to spaces for indents, > please > stick to doc/coding-style.txt to ensure consistency and optimal > participation > across contributors. > Ok, will try to do so. ;) > > > +} > > + > > Same below about the return value. > > > +static int open_current_namespace() { > > + char buf[512]; > > + int fd; > > + > > + snprintf(buf, sizeof(buf), "/proc/%d/ns/net", getpid()); > > + fd = open(buf, O_RDONLY); > > + > > + return fd; > > +} > > + > > return value etc. > > > +static int socketat(int default_ns, int target_ns, int domain, int > type, int protocol) { > > + int sock; > > + > > + if (setns(target_ns, CLONE_NEWNET) == -1) > > + return -1; > > + > > + sock = socket(domain, type, protocol); > > + > > + if (setns(default_ns, CLONE_NEWNET) == -1) { > > + close(sock); > > + return -1; > > + } > > + > > + return sock; > > +} > > + > > +int namespace_socket(const char *ns_name, int domain, int type, int > protocol) { > > + int default_ns, target_ns, sock; > > + > > + if (ns_name) { > > + default_ns = open_current_namespace(); > > + if (default_ns == -1) { > > + return -1; > > + } > > + > > + target_ns = open_named_namespace(ns_name); > > + if (target_ns == -1) { > > + close(default_ns); > > + return -1; > > + } > > + > > + sock = socketat(default_ns, target_ns, domain, type, protocol); > > + > > + close(target_ns); > > + close(default_ns); > > + } > > + else { > > + sock = socket(domain, type, protocol); > > + } > > Given that the function above aims at replacing a call to socket(), > some changes will be needed for performance. First, it should be > inlined and an unlikely() put around if (ns_name) so that we don't > hurt too much the most common case. > > Second, it becomes critically important that the target_ns remains > open and assigned to each ns so that we don't have to perform that > very expensive open()/close() dance for every socket. Also, haproxy > is designed to run chrooted, so that won't work above because /proc > or /var will not exist in the chroot. And open() is blocking, so > that can become catastrophically slow on a machine with moderate I/O > load. > > The proper solution in my opinion is to have a list of (namespace name, > namespace fd) built at startup time, and to store the namespace fd in > the server/listener structs once they're resolved after parsing. Then > you can don't need namespace_socket() anymore and can directly use > socketat(). > Ok, we'll think this through. > > + return sock; > > +} > > diff --git a/src/proto_tcp.c b/src/proto_tcp.c > > index 72dc92b..94cb091 100644 > > --- a/src/proto_tcp.c > > +++ b/src/proto_tcp.c > > @@ -33,6 +33,7 @@ > > #include <common/errors.h> > > #include <common/mini-clist.h> > > #include <common/standard.h> > > +#include <common/namespace.h> > > > > #include <types/global.h> > > #include <types/capture.h> > > @@ -284,6 +285,7 @@ int tcp_connect_server(struct connection *conn, int > data, int delack) > > struct server *srv; > > struct proxy *be; > > struct conn_src *src; > > + int namespace_set = conn->flags & CO_FL_NAMESPACE_SET; > > Isn't the namespace a property of the server instead of the connection, > given that you configure it on the server line ? That makes me think that > this flag (or at least this test) is not needed and that the server's NS > will be enough. > Well, in certain cases it's the property of the server. However, when recvproxy was enabled for a connection it is possible that a namespace name was received via the proxy protocol, and that can also be used to determine which namespace the server is in. > > conn->flags = CO_FL_WAIT_L4_CONN; /* connection in progress */ > > > > @@ -301,7 +303,13 @@ int tcp_connect_server(struct connection *conn, int > data, int delack) > > return SN_ERR_INTERNAL; > > } > > > > - if ((fd = conn->t.sock.fd = socket(conn->addr.to.ss_family, > SOCK_STREAM, IPPROTO_TCP)) == -1) { > > + if (namespace_set) { > > + fd = conn->t.sock.fd = > namespace_socket(conn->network_namespace, conn->addr.to.ss_family, > SOCK_STREAM, IPPROTO_TCP); > > + } else { > > + fd = conn->t.sock.fd = socket(conn->addr.to.ss_family, > SOCK_STREAM, IPPROTO_TCP); > > + } > > These ones will be fused into a common socketat() call. > Yeah, that would be possibly better. > > + if (fd == -1) { > > qfprintf(stderr, "Cannot get a server socket.\n"); > > > > if (errno == ENFILE) { > > @@ -732,10 +740,18 @@ int tcp_bind_listener(struct listener *listener, > char *errmsg, int errlen) > > fd = listener->fd; > > ext = (fd >= 0); > > > > - if (!ext && (fd = socket(listener->addr.ss_family, SOCK_STREAM, > IPPROTO_TCP)) == -1) { > > - err |= ERR_RETRYABLE | ERR_ALERT; > > - msg = "cannot create listening socket"; > > - goto tcp_return; > > + if (!ext) { > > + if (listener->options & LI_O_NAMESPACE_SET) { > > + fd = namespace_socket(listener->network_namespace, > listener->addr.ss_family, SOCK_STREAM, IPPROTO_TCP); > > + } else { > > + fd = socket(listener->addr.ss_family, SOCK_STREAM, > IPPROTO_TCP); > > + } > > These ones will be fused into a common socketat() call as well. > Ok. > > + > > + if (fd == -1) { > > + err |= ERR_RETRYABLE | ERR_ALERT; > > + msg = "cannot create listening socket"; > > + goto tcp_return; > > + } > > } > > > > if (fd >= global.maxsock) { > > @@ -1994,6 +2010,29 @@ static int bind_parse_interface(char **args, int > cur_arg, struct proxy *px, stru > > } > > #endif > > > > +/* parse the "namespace" bind keyword */ > > +static int bind_parse_namespace(char **args, int cur_arg, struct proxy > *px, struct bind_conf *conf, char **err) > > +{ > > + struct listener *l; > > + char *namespace = NULL; > > + > > + if (!*args[cur_arg + 1]) { > > + memprintf(err, "'%s' : missing namespace id", > args[cur_arg]); > > + return ERR_ALERT | ERR_FATAL; > > + } > > + namespace = args[cur_arg + 1]; > > + > > + list_for_each_entry(l, &conf->listeners, by_bind) { > > + l->network_namespace = strdup(namespace); > > + if(l->network_namespace == NULL){ > > + Alert("Cannot allocate memory for network > namespace: '%s'.\n",args[cur_arg + 1]); > > + return ERR_ALERT | ERR_FATAL; > > + } > > + l->options |= LI_O_NAMESPACE_SET; > > + } > > + return 0; > > +} > > + > > static struct cfg_kw_list cfg_kws = {ILH, { > > { CFG_LISTEN, "tcp-request", tcp_parse_tcp_req }, > > { CFG_LISTEN, "tcp-response", tcp_parse_tcp_rep }, > > @@ -2053,6 +2092,7 @@ static struct bind_kw_list bind_kws = { "TCP", { > }, { > > { "v4v6", bind_parse_v4v6, 0 }, /* force socket > to bind to IPv4+IPv6 */ > > { "v6only", bind_parse_v6only, 0 }, /* force socket > to bind to IPv6 only */ > > #endif > > + { "namespace", bind_parse_namespace, 1 }, > > /* the versions with the NULL parse function*/ > > { "defer-accept", NULL, 0 }, > > { "interface", NULL, 1 }, > > diff --git a/src/server.c b/src/server.c > > index fdb63cc..7fab99a 100644 > > --- a/src/server.c > > +++ b/src/server.c > > @@ -1501,6 +1501,23 @@ int parse_server(const char *file, int linenum, > char **args, struct proxy *curpr > > err_code |= ERR_ALERT | ERR_FATAL; > > goto out; > > } > > + else if (!defsrv && !strcmp(args[cur_arg], > "namespace")) { > > + // FIXME: set a flag indicating that the > namespace has been set > > I think your flag is present below, it simply is an or() of the two SRV_F_* > flags that are set. > > > + char *arg = args[cur_arg + 1]; > > + if (!strcmp(arg, "fromproxy")) { > > + newsrv->flags |= > SRV_F_USE_NAMESPACE_FROM_PROXY_PROTOCOL; > > + } else { > > + newsrv->network_namespace = > strdup(args[cur_arg + 1]); > > + if( newsrv->network_namespace == > NULL) > > + { > > + Alert("Cannot allocate > memory for network namespace: '%s'.\n",args[cur_arg + 1]); > > + err_code |= ERR_ALERT | > ERR_FATAL; > > + goto out; > > + } > > + newsrv->flags |= > SRV_F_USE_CONFIGURED_NAMESPACE; > > + } > > + cur_arg += 2; > > + } > > else { > > static int srv_dumped; > > struct srv_kw *kw; > > diff --git a/src/session.c b/src/session.c > > index aeaa7e1..10532ae 100644 > > --- a/src/session.c > > +++ b/src/session.c > > @@ -91,6 +91,11 @@ int session_accept(struct listener *l, int cfd, > struct sockaddr_storage *addr) > > cli_conn->flags |= CO_FL_ADDR_FROM_SET; > > cli_conn->target = &l->obj_type; > > > > + if (l->options & LI_O_NAMESPACE_SET) { > > + cli_conn->flags |= CO_FL_NAMESPACE_SET; > > + cli_conn->network_namespace = l->network_namespace; > > Maybe once we have a list of (ns, fd) we'll be able to store the fd only > instead of the ns here, because we try to limit the connection struct to > the smallest possible size. > Yeah, this is the namespace fd caching thing you've suggested above. > > + } > > + > > if (unlikely((s = pool_alloc2(pool2_session)) == NULL)) > > goto out_free_conn; > > > > -- > > 1.9.1 > > > > Second patch : > > > diff --git a/include/types/connection.h b/include/types/connection.h > > index 89f4f38..a75d45d 100644 > > --- a/include/types/connection.h > > +++ b/include/types/connection.h > > @@ -334,6 +334,7 @@ struct proxy_hdr_v2 { > > #define PP2_TYPE_SSL 0x20 > > #define PP2_TYPE_SSL_VERSION 0x21 > > #define PP2_TYPE_SSL_CN 0x22 > > +#define PP2_TYPE_NETWORK_NAMESPACE 0x30 > > Could you please avoid to fragment the values here, I fear the day we need > a contiguous array and we can't find one anymore. Let's start at 0x23 for > now. Note, since we're in development, we can change these numbers later, > so let's not consider the PPv2 spec as "official" whenever it comes to what > we stuff in haproxy-dev. > Ok. So which ID should we use? I must admit that at some point I lost track of the discussion about SSL related IDs... ;) > > +/* > > + * Get tlv data from tlv struct. Function return value is length of data > > + */ > > I don't want to be picky here but apparently you return a string, not > data, since you stop on the first zero. > Yep. > > +static int get_tlv_data(struct tlv * src,char *dst, int dst_length) > > +{ > > + int length = get_tlv_length(src); > > + if(dst_length < length) > > + return -1; > > + strncpy(dst, (char *)src->value, length); > > + dst[length] = '\0'; > > + return length; > > Also, you should use strlen(dst) for the return value, because "length" > might > be wrong if a zero is present in the TLV entry. > > > +static int search_tlv(int type, unsigned int len, char *tlv_packets) > > +{ > > + int parsed_length = 0; > > + > > + while (parsed_length <= len && type != tlv_packets[parsed_length]) > > + parsed_length += get_tlv_length((struct tlv*) > &tlv_packets[parsed_length]); > > + > > + return parsed_length; > > +} > > It's a good idea to have provided this function, I'm sure it will meet many > new usages! > > > /* This handshake handler waits a PROXY protocol header at the > beginning of the > > * raw data stream. The header looks like this : > > * > > @@ -255,6 +286,7 @@ int conn_recv_proxy(struct connection *conn, int > flag) > > char *line, *end; > > struct proxy_hdr_v2 *hdr_v2; > > const char v2sig[] = PP2_SIGNATURE; > > + int tlv_length = 0; > > > > /* we might have been called just after an asynchronous shutr */ > > if (conn->flags & CO_FL_SOCK_RD_SH) > > @@ -432,6 +464,7 @@ int conn_recv_proxy(struct connection *conn, int > flag) > > > > switch (hdr_v2->ver_cmd & PP2_CMD_MASK) { > > case 0x01: /* PROXY command */ > > + > > switch (hdr_v2->fam) { > > case 0x11: /* TCPv4 */ > > ((struct sockaddr_in > *)&conn->addr.from)->sin_family = AF_INET; > > Extra empty line above. Please try to avoid adding unnecessary newlines > out of the scope you're working in, they add more context to the patch > and make its backports and fixes a little bit more painful. I'm sure it's > a left-over from a debugging session though, I'm not complaining, just > reporting :-) > Ok, we'll remove it. > > @@ -441,6 +474,7 @@ int conn_recv_proxy(struct connection *conn, int > flag) > > ((struct sockaddr_in > > *)&conn->addr.to)->sin_addr.s_addr > = hdr_v2->addr.ip4.dst_addr; > > ((struct sockaddr_in *)&conn->addr.to)->sin_port > = hdr_v2->addr.ip4.dst_port; > > conn->flags |= CO_FL_ADDR_FROM_SET | > CO_FL_ADDR_TO_SET; > > + tlv_length = ntohs(hdr_v2->len) - > PP2_ADDR_LEN_INET; > > break; > > case 0x21: /* TCPv6 */ > > ((struct sockaddr_in6 > *)&conn->addr.from)->sin6_family = AF_INET6; > > @@ -450,8 +484,40 @@ int conn_recv_proxy(struct connection *conn, int > flag) > > memcpy(&((struct sockaddr_in6 > > *)&conn->addr.to)->sin6_addr, > hdr_v2->addr.ip6.dst_addr, 16); > > ((struct sockaddr_in6 *)&conn->addr.to)->sin6_port > = hdr_v2->addr.ip6.dst_port; > > conn->flags |= CO_FL_ADDR_FROM_SET | > CO_FL_ADDR_TO_SET; > > + tlv_length = ntohs(hdr_v2->len) - > PP2_ADDR_LEN_INET6; > > break; > > } > > + > > + /*tlv parsing*/ > > + if(tlv_length) > > + { > > + // data from: > > + char * tlv_packets = > &trash.str[trash.len-tlv_length]; > > + > > + int tlv_first_byte = 0; > > + > > + /*search tlv data, we interested in*/ > > + tlv_first_byte = > search_tlv(PP2_TYPE_NETWORK_NAMESPACE, tlv_length, tlv_packets); > > + > > + /*if tlv_lenght*/ > > + if(tlv_first_byte < tlv_length){ > > + int network_namspace_leght = 0; > > + network_namspace_leght = > get_tlv_length((struct tlv*) &tlv_packets[tlv_first_byte]) + 1; > > + /*if the buffer is not enough for > namespace*/ > > + conn_clear_network_namespace(conn); > > + conn->network_namespace = (char*) > malloc(network_namspace_leght * sizeof(char)); > > + conn->flags |= CO_FL_NAMESPACE_RECV; > > + > > + if(-1 == get_tlv_data((struct tlv*) > &tlv_packets[tlv_first_byte], conn->network_namespace, > network_namspace_leght)){ > > + goto bad_header; > > + } > > + else > > + { > > + conn->flags |= CO_FL_NAMESPACE_SET; > > + } > > + } > > + } > > + > > Please clean up the block above, it's highly out of style compared to the > rest, and even contains some typos ("lenght" and "leght" instead of > "length", > "namspace"). Just a few hints, spaces around language keywords/operators/ > braces/comments, isolated braces, disturbing "if(result==subject)", etc. > I'm sure your code deserves a bit more love. > Sure. > /* unsupported protocol, keep local connection address */ > > break; > > case 0x00: /* LOCAL command */ > > @@ -601,7 +667,6 @@ int make_proxy_line_v1(char *buf, int buf_len, > struct sockaddr_storage *src, str > > return ret; > > } > > > > -#ifdef USE_OPENSSL > > static int make_tlv(char *dest, int dest_len, char type, uint16_t > length, char *value) > > { > > struct tlv *tlv; > > @@ -617,18 +682,18 @@ static int make_tlv(char *dest, int dest_len, char > type, uint16_t length, char * > > memcpy(tlv->value, value, length); > > return length + sizeof(*tlv); > > } > > -#endif > > > > int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, > struct connection *remote) > > { > > const char pp2_signature[] = PP2_SIGNATURE; > > int ret = 0; > > + int tlv_len = 0; > > struct proxy_hdr_v2 *hdr = (struct proxy_hdr_v2 *)buf; > > struct sockaddr_storage null_addr = {0}; > > struct sockaddr_storage *src = &null_addr; > > struct sockaddr_storage *dst = &null_addr; > > + > > #ifdef USE_OPENSSL > > - int tlv_len = 0; > > char *value = NULL; > > struct tlv_ssl *tlv; > > int ssl_tlv_len = 0; > > @@ -643,6 +708,7 @@ int make_proxy_line_v2(char *buf, int buf_len, > struct server *srv, struct connec > > src = &remote->addr.from; > > dst = &remote->addr.to; > > } > > + > > if (src && dst && src->ss_family == dst->ss_family && > src->ss_family == AF_INET) { > > if (buf_len < PP2_HDR_LEN_INET) > > return 0; > > @@ -707,6 +773,15 @@ int make_proxy_line_v2(char *buf, int buf_len, > struct server *srv, struct connec > > ret += ssl_tlv_len; > > } > > #endif > > + //PP2_TYPE_NETWORK_NAMESPACE > > + > > + if( remote && (remote->flags & CO_FL_NAMESPACE_SET) ){ > > + if ((buf_len - ret) < sizeof(struct tlv)) > > + return 0; > > Disturbing indents/spaces. > > > + > > + tlv_len = make_tlv(&buf[ret], buf_len, > PP2_TYPE_NETWORK_NAMESPACE, strlen(remote->network_namespace), > remote->network_namespace); > > + ret += tlv_len; > > + } > > Possibly it will make sense to have both the namespace name and its length > in the structures so that you don't need to run strlen() over the name for > every PPv2 line you build. That could probably be done as part of the > (namespace,fd) list design I think, because maybe you'll only need > approximately this for each already opened namespace : > > struct open_ns { > char *namespace_str; > char *namespace_len; > int namespace_fd; > }; > > Then providing a pointer to such a struct into each listener/server should > work reasonably well for all your uses. You can also consider that by > design > the default one is inserted first into the list. > Ok. Will consider this as part of the namespace/fd cache. > Overall, that's great, I'm seeing this as a really useful feature of 1.6 > that will gain some traction from users. We always need to have good stuff > at the beginning to incite users to test development releases and detect > the new bugs we add, this is a great opportunity to please everyone :-) > Thanks a lot for your comments and feedback. We're a bit overwhelmed at the moment, but will come back with an updated patchset sooner-or-later. -- KOVACS Krisztian
