On 12/1/20 11:50 AM, Emeric Brun wrote:
Hi,
Hi Thayne,
See my answers below.
On 11/30/20 10:23 AM, PR Bot wrote:
Dear list!
Author: Thayne McCombs <tha...@lucidchart.com>
Number of patches: 2
This is an automated relay of the Github pull request:
Add srvkey option to stick-table
Patch title(s):
Add srvkey option to stick-table
Harden sa2str agains 107-byte-long abstract unix domain path
Link:
https://github.com/haproxy/haproxy/pull/979
Edit locally:
wget https://github.com/haproxy/haproxy/pull/979.patch && vi 979.patch
Apply locally:
curl https://github.com/haproxy/haproxy/pull/979.patch | git am -
Description:
This allows using the address of the server rather than the name of
the
server for keeping track of servers in a backend for
stickiness.
Fixes #814
I haven't tested this at all
yet, and it still needs some polish, but here is a draft of how to fix
#814.
This is my first significant contribution to haproxy,
so I would not be surprised if I'm doing something terribly wrong, and
I'm sure there are at least some small mistakes in it. Initial
feedback would be very welcome.
Thank you for this first contribution which looks almost good!!! We all
do wrong things ;) Even if it not easy to comment your code because it
is not included in this mail, let's giving a try.
I have noticed two places where initializations are missing:
diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
index 998e210f6..e62b79765 100644
--- a/include/haproxy/proxy-t.h
+++ b/include/haproxy/proxy-t.h
@@ -424,6 +424,7 @@ struct proxy {
char *lfsd_file; /* file name where the structured-data logformat
string for RFC5424 appears (strdup) */
int lfsd_line; /* file name where the structured-data logformat
string for RFC5424 appears */
} conf; /* config information */
+ struct eb_root used_server_addr; /* list of server addresses in
use */
< used_server_addr> ebtree is not initialized. This would make haproxy
crash. I think you should initialized this ebtree with EB_ROOT_UNIQUE as
value.
You should comment all the new functions (see srv_set_addr_desc() and
perhaps others).
The following function should be commente as this done for
srv_set_dyncookie() espcially for the locks we need even if this
function is only used at parsing time, as far as I see. What if we want
to use it at runing time? This function should be called with a lock on
<s> server (not necessary at parsing time).
+static void srv_set_addr_desc(struct server *s)
+{
+ struct proxy *p = s->proxy;
+ char *key;
+
+ key = sa2str(&s->addr, s->svc_port, s->flags & SRV_F_MAPPORTS);
+
+ if (strcmp(key, s->addr_desc) == 0) {
->addr_desc is initialized only by srv_set_addr_desc(). So the first
time we enter this function ->addr_desc has NULL as value.
+ free(key);
+ return;
+ }
+
+ HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock);
+ ebpt_delete(&s->addr_node);
+ HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock);
+
+ free(s->addr_desc);
+ s->addr_desc = key;
+ s->addr_node.key = key;
I think this is dangerous to initialize two objects with the same value
as a string. I would strdup() <key> to initialize s->addr_node.key.
+ // TODO: should this use a dedicated lock?
+ HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock);
+ ebis_insert(&p->used_server_addr, &s->addr_node);
+ HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock);
+}
+
There are two typos in the documentation:
+ a template). If "addr" is given, then the server is idntified
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
+ by it's current network address
~~~~~~~~~~~~^
You should split your patch to provide such cleanup code in seperated patch:
@@ -1453,7 +1497,7 @@ int url2sa(const char *url, int ulen, struct
sockaddr_storage *addr, struct spli
/* Secondly, if :// pattern is found, verify parsed stuff
* before pattern is matching our http pattern.
* If so parse ip address and port in uri.
- *
+ *
I am not sure at all about this modification, but it semms
implementation dependent to me:
diff --git a/src/tools.c b/src/tools.c
index c51f542c6..21e3f06ef 100644
--- a/src/tools.c
+++ b/src/tools.c
@@ -1228,7 +1228,7 @@ char * sa2str(const struct sockaddr_storage *addr,
int port, int map_ports)
case AF_UNIX:
path = ((struct sockaddr_un *)addr)->sun_path;
if (path[0] == '\0') {
- return memprintf(&out, "abns@%s", path+1);
+ return memprintf(&out, "abns@%107s", path+1);
} else {
return strdup(path);
}
I would not implement this function:
+static void srv_addr_updated(struct server *srv)
+{
+ srv_set_dyncookie(srv);
+ srv_set_addr_desc(srv);
+}
Note something important: there are regression tests which can be run.
See reg-tests directory.
