Thanks for your feedback. I have some followup questions. > I think you should initialized this ebtree with EB_ROOT_UNIQUE as value.
I'm not sure I understand what the distinction between EB_ROOT and EB_ROOT_UNIQUE is. I'm happy to add this, but I'd like to understand why it should be EB_ROOT_UNIQUE here (since it looks like it is initialized as EB_ROOT for used_server_id and used_listener_id, and I can't find where it is initialized for used_server_name, which I believe would be equivalent since EB_ROOT is basically just zero initialized). > 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. ok. I was following the pattern used with conf.name in struct server. Would it be better to use strdup, or to not have a separate field for addr_desc, and just use addr_node.key (which is a void*)? If the latter maybe rename addr_node as addr_desc? Additionally: In srv_set_addr_desc, would it be better to hold the lock over the entire operation, or just while we are actually manipulating the tree? Meaning should I release the lock between deleting the old node and inserting the new node? And should this use the existing proxy lock, or should there be a separate dedicated lock for used_server_addr? I'm also a little bit concerned about leaking memory in the server_key_dict. I couldn't find anything that cleans up these keys, although there is a refcount on them. In an environment that uses service discovery and the servers change frequently, this could lead to a memory leak as this dict only grows in size. Thank you, Thayne Thayne McCombs Senior Software Engineer [email protected] Lucid.co On Tue, Dec 1, 2020 at 10:24 AM Frederic Lecaille <[email protected]> wrote: > > 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 <[email protected]> > >> 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.
