Re: [PATCH 1/1] MINOR: build: force CC to set a return code when probing options
Hello Bertrand, On Sun, 7 Mar 2021 at 00:53, Bertrand Jacquin wrote: > I am not proposing haproxy build-system to use -Werror here, I'm only > proposing to use -Werror when probing for options supported by the > compiler, as effectively clang return a code if 0 even if an option is > not supported. The fact haproxy does not use -Wno-clobbered today is > with clang build because of an internal bug in clang with how haproxy is > using stdin/stderr indirection. > > With the proposal above, Werror is only use to probe options, it is not > reflected in SPEC_CFLAGS. Got it, thanks for clarifying. Lukas
Re: [PATCH 1/1] MINOR: build: force CC to set a return code when probing options
Hi Lukas, On Saturday, March 06 2021 at 23:48:52 +0100, Lukas Tribus wrote: > Hello, > > On Sat, 6 Mar 2021 at 21:25, Bertrand Jacquin wrote: > > > > gcc returns non zero code if an option is not supported (tested > > from 6.5 to 10.2). > > > > $ gcc -Wfoobar -E -xc - -o /dev/null < /dev/null > /dev/null 2>&1 ; echo > > $? > > 1 > > > > clang always return 0 if an option in not recognized unless > > -Werror is also passed, preventing a correct probing of options > > supported by the compiler (tested with clang 6.0.1 to 11.1.0) > > -Werror is more than just "-Wunknown-warning-option" on clang. > -Werror/ERR is specifically optional in the Makefile. > > If we want to change the haproxy build-system to do -Werror from now > on we need a) discuss it and b) fix it all up. We can't hardcode > -Werror and at the same time claim that it's an optional parameter. I am not proposing haproxy build-system to use -Werror here, I'm only proposing to use -Werror when probing for options supported by the compiler, as effectively clang return a code if 0 even if an option is not supported. The fact haproxy does not use -Wno-clobbered today is with clang build because of an internal bug in clang with how haproxy is using stdin/stderr indirection. With the proposal above, Werror is only use to probe options, it is not reflected in SPEC_CFLAGS. -- Bertrand
Re: [PATCH 1/1] MINOR: build: force CC to set a return code when probing options
Hello, On Sat, 6 Mar 2021 at 21:25, Bertrand Jacquin wrote: > > gcc returns non zero code if an option is not supported (tested > from 6.5 to 10.2). > > $ gcc -Wfoobar -E -xc - -o /dev/null < /dev/null > /dev/null 2>&1 ; echo $? > 1 > > clang always return 0 if an option in not recognized unless > -Werror is also passed, preventing a correct probing of options > supported by the compiler (tested with clang 6.0.1 to 11.1.0) -Werror is more than just "-Wunknown-warning-option" on clang. -Werror/ERR is specifically optional in the Makefile. If we want to change the haproxy build-system to do -Werror from now on we need a) discuss it and b) fix it all up. We can't hardcode -Werror and at the same time claim that it's an optional parameter. Lukas
[PATCH 1/1] MINOR: build: force CC to set a return code when probing options
gcc returns non zero code if an option is not supported (tested from 6.5 to 10.2). $ gcc -Wfoobar -E -xc - -o /dev/null < /dev/null > /dev/null 2>&1 ; echo $? 1 clang always return 0 if an option in not recognized unless -Werror is also passed, preventing a correct probing of options supported by the compiler (tested with clang 6.0.1 to 11.1.0). $ clang -Wfoobar -E -xc - -o /dev/null < /dev/null > /dev/null 2>&1 ; echo $? 0 $ clang -Werror -Wfoobar -E -xc - -o /dev/null < /dev/null > /dev/null 2>&1 ; echo $? 1 Please note today this is not visible since clang 11 exit with SIGABRT or with return code 1 on older version due to bad file descriptor from file descriptor handling $ clang -Wfoobar -E -xc - -o /dev/null < /dev/null 2>&0 ; echo $? Aborted (core dumped) 134 $ clang -Wfoobar -E -xc - -o /dev/null < /dev/null ; echo $? warning: unknown warning option '-Wfoobar'; did you mean '-Wformat'? [-Wunknown-warning-option] 1 warning generated. 0 $ clang-11 -Werror -Wfoobar -E -xc - -o /dev/null < /dev/null ; echo $? error: unknown warning option '-Wfoobar'; did you mean '-Wformat'? [-Werror,-Wunknown-warning-option] 1 This specific issue is being tracked with clang upstream in https://bugs.llvm.org/show_bug.cgi?id=49463 --- Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 77960ba4cac5..0867047fdc69 100644 --- a/Makefile +++ b/Makefile @@ -126,16 +126,16 @@ endif # Usage: CFLAGS += $(call cc-opt,option). Eg: $(call cc-opt,-fwrapv) # Note: ensure the referencing variable is assigned using ":=" and not "=" to # call it only once. -cc-opt = $(shell set -e; if $(CC) $(1) -E -xc - -o /dev/null &0 2>&0; then echo "$(1)"; fi;) +cc-opt = $(shell set -e; if $(CC) -Werror $(1) -E -xc - -o /dev/null &0 2>&0; then echo "$(1)"; fi;) # same but emits $2 if $1 is not supported -cc-opt-alt = $(shell set -e; if $(CC) $(1) -E -xc - -o /dev/null &0 2>&0; then echo "$(1)"; else echo "$(2)"; fi;) +cc-opt-alt = $(shell set -e; if $(CC) -Werror $(1) -E -xc - -o /dev/null &0 2>&0; then echo "$(1)"; else echo "$(2)"; fi;) # Disable a warning when supported by the compiler. Don't put spaces around the # warning! And don't use cc-opt which doesn't always report an error until # another one is also returned. # Usage: CFLAGS += $(call cc-nowarn,warning). Eg: $(call cc-opt,format-truncation) -cc-nowarn = $(shell set -e; if $(CC) -W$(1) -E -xc - -o /dev/null &0 2>&0; then echo "-Wno-$(1)"; fi;) +cc-nowarn = $(shell set -e; if $(CC) -Werror -W$(1) -E -xc - -o /dev/null &0 2>&0; then echo "-Wno-$(1)"; fi;) Installation options. DESTDIR =
[PATCH 4/5] MINOR: connection: Use a `struct ist` to store proxy_authority
This makes the code cleaner, because proxy_authority can be handled like
proxy_unique_id.
---
include/haproxy/connection-t.h | 5 ++---
include/haproxy/connection.h | 6 +++---
src/connection.c | 28 +---
3 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/include/haproxy/connection-t.h b/include/haproxy/connection-t.h
index 0bcceacff..0c4a6649c 100644
--- a/include/haproxy/connection-t.h
+++ b/include/haproxy/connection-t.h
@@ -533,9 +533,8 @@ struct connection {
void (*destroy_cb)(struct connection *conn); /* callback to notify of
imminent death of the connection */
struct sockaddr_storage *src; /* source address (pool), when known,
otherwise NULL */
struct sockaddr_storage *dst; /* destination address (pool), when
known, otherwise NULL */
- char *proxy_authority;/* Value of authority TLV received via
PROXYv2 */
- uint8_t proxy_authority_len; /* Length of authority TLV received via
PROXYv2 */
- struct ist proxy_unique_id; /* Value of the unique ID TLV received via
PROXYv2 */
+ struct ist proxy_authority; /* Value of the authority TLV received
via PROXYv2 */
+ struct ist proxy_unique_id; /* Value of the unique ID TLV received
via PROXYv2 */
struct quic_conn *qc; /* Only present if this connection is a
QUIC one */
/* used to identify a backend connection for http-reuse,
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 33c1380cc..46a521e01 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -355,7 +355,7 @@ static inline void conn_init(struct connection *conn, void
*target)
conn->subs = NULL;
conn->src = NULL;
conn->dst = NULL;
- conn->proxy_authority = NULL;
+ conn->proxy_authority = IST_NULL;
conn->proxy_unique_id = IST_NULL;
conn->hash_node = NULL;
}
@@ -553,8 +553,8 @@ static inline void conn_free(struct connection *conn)
sockaddr_free(&conn->src);
sockaddr_free(&conn->dst);
- pool_free(pool_head_authority, conn->proxy_authority);
- conn->proxy_authority = NULL;
+ pool_free(pool_head_authority, istptr(conn->proxy_authority));
+ conn->proxy_authority = IST_NULL;
pool_free(pool_head_uniqueid, istptr(conn->proxy_unique_id));
conn->proxy_unique_id = IST_NULL;
diff --git a/src/connection.c b/src/connection.c
index 10bc6712f..716de51f8 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -487,13 +487,19 @@ int conn_recv_proxy(struct connection *conn, int flag)
}
#endif
case PP2_TYPE_AUTHORITY: {
- if (tlv_len > PP2_AUTHORITY_MAX)
+ const struct ist tlv = ist2((const char
*)tlv_packet->value, tlv_len);
+
+ if (istlen(tlv) > PP2_AUTHORITY_MAX)
goto bad_header;
- conn->proxy_authority =
pool_alloc(pool_head_authority);
- if (conn->proxy_authority == NULL)
+ conn->proxy_authority =
ist2(pool_alloc(pool_head_authority), 0);
+ if (!isttest(conn->proxy_authority))
goto fail;
- memcpy(conn->proxy_authority, (const char
*)tlv_packet->value, tlv_len);
- conn->proxy_authority_len = tlv_len;
+ if (istcpy(&conn->proxy_authority, tlv,
PP2_AUTHORITY_MAX) < 0) {
+ /* This is technically unreachable,
because we verified above
+* that the TLV value fits.
+*/
+ goto fail;
+ }
break;
}
case PP2_TYPE_UNIQUE_ID: {
@@ -1188,9 +1194,9 @@ int make_proxy_line_v2(char *buf, int buf_len, struct
server *srv, struct connec
if (srv->pp_opts & SRV_PP_V2_AUTHORITY) {
value = NULL;
- if (remote && remote->proxy_authority) {
- value = remote->proxy_authority;
- value_len = remote->proxy_authority_len;
+ if (remote && isttest(remote->proxy_authority)) {
+ value = istptr(remote->proxy_authority);
+ value_len = istlen(remote->proxy_authority);
}
#ifdef USE_OPENSSL
else {
@@ -1354,13 +1360,13 @@ int smp_fetch_fc_pp_authority(const struct arg *args,
struct sample *smp, const
return 0;
}
- if (conn->proxy_authority == NULL)
+ if (!isttest(conn->proxy_authority))
return 0;
smp->flags = 0;
s
[PATCH 2/5] CLEANUP: connection: Remove useless test for NULL before calling `pool_free()`
`pool_free()` is a noop when the given pointer is NULL. No need to test.
---
include/haproxy/connection.h | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index e00e6f820..58b0d0e7b 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -553,18 +553,14 @@ static inline void conn_free(struct connection *conn)
sockaddr_free(&conn->src);
sockaddr_free(&conn->dst);
- if (conn->proxy_authority != NULL) {
- pool_free(pool_head_authority, conn->proxy_authority);
- conn->proxy_authority = NULL;
- }
- if (isttest(conn->proxy_unique_id)) {
- pool_free(pool_head_uniqueid, conn->proxy_unique_id.ptr);
- conn->proxy_unique_id = IST_NULL;
- }
- if (conn->hash_node) {
- pool_free(pool_head_conn_hash_node, conn->hash_node);
- conn->hash_node = NULL;
- }
+ pool_free(pool_head_authority, conn->proxy_authority);
+ conn->proxy_authority = NULL;
+
+ pool_free(pool_head_uniqueid, conn->proxy_unique_id.ptr);
+ conn->proxy_unique_id = IST_NULL;
+
+ pool_free(pool_head_conn_hash_node, conn->hash_node);
+ conn->hash_node = NULL;
/* By convention we always place a NULL where the ctx points to if the
* mux is null. It may have been used to store the connection as a
--
2.30.1
[PATCH 5/5] CLEANUP: connection: Consistently use `struct ist` to process all TLV types
Instead of directly poking around within the `struct tlv tlv_packet` the actual
value will be consumed using a `struct ist`.
---
src/connection.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/connection.c b/src/connection.c
index 716de51f8..b1d643b9d 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -447,15 +447,15 @@ int conn_recv_proxy(struct connection *conn, int flag)
/* TLV parsing */
while (tlv_offset < total_v2_len) {
struct tlv *tlv_packet;
- size_t tlv_len;
+ struct ist tlv;
/* Verify that we have at least TLV_HEADER_SIZE bytes
left */
if (tlv_offset + TLV_HEADER_SIZE > total_v2_len)
goto bad_header;
tlv_packet = (struct tlv *) &trash.area[tlv_offset];
- tlv_len = get_tlv_length(tlv_packet);
- tlv_offset += tlv_len + TLV_HEADER_SIZE;
+ tlv = ist2((const char *)tlv_packet->value,
get_tlv_length(tlv_packet));
+ tlv_offset += istlen(tlv) + TLV_HEADER_SIZE;
/* Verify that the TLV length does not exceed the total
PROXYv2 length */
if (tlv_offset > total_v2_len)
@@ -466,11 +466,11 @@ int conn_recv_proxy(struct connection *conn, int flag)
uint32_t n_crc32c;
/* Verify that this TLV is exactly 4 bytes long
*/
- if (tlv_len != 4)
+ if (istlen(tlv) != 4)
goto bad_header;
- n_crc32c = read_n32(tlv_packet->value);
- write_n32(tlv_packet->value, 0); // compute
with CRC==0
+ n_crc32c = read_n32(istptr(tlv));
+ write_n32(istptr(tlv), 0); // compute with
CRC==0
if (hash_crc32c(trash.area, total_v2_len) !=
n_crc32c)
goto bad_header;
@@ -480,15 +480,13 @@ int conn_recv_proxy(struct connection *conn, int flag)
case PP2_TYPE_NETNS: {
const struct netns_entry *ns;
- ns =
netns_store_lookup((char*)tlv_packet->value, tlv_len);
+ ns = netns_store_lookup(istptr(tlv),
istlen(tlv));
if (ns)
conn->proxy_netns = ns;
break;
}
#endif
case PP2_TYPE_AUTHORITY: {
- const struct ist tlv = ist2((const char
*)tlv_packet->value, tlv_len);
-
if (istlen(tlv) > PP2_AUTHORITY_MAX)
goto bad_header;
conn->proxy_authority =
ist2(pool_alloc(pool_head_authority), 0);
@@ -503,8 +501,6 @@ int conn_recv_proxy(struct connection *conn, int flag)
break;
}
case PP2_TYPE_UNIQUE_ID: {
- const struct ist tlv = ist2((const char
*)tlv_packet->value, tlv_len);
-
if (istlen(tlv) > UNIQUEID_LEN)
goto bad_header;
conn->proxy_unique_id =
ist2(pool_alloc(pool_head_uniqueid), 0);
--
2.30.1
[PATCH 3/5] CLEANUP: connection: Use istptr / istlen for proxy_unique_id
Don't access the ist's fields directly, use the helper functions instead.
---
include/haproxy/connection.h | 2 +-
src/connection.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 58b0d0e7b..33c1380cc 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -556,7 +556,7 @@ static inline void conn_free(struct connection *conn)
pool_free(pool_head_authority, conn->proxy_authority);
conn->proxy_authority = NULL;
- pool_free(pool_head_uniqueid, conn->proxy_unique_id.ptr);
+ pool_free(pool_head_uniqueid, istptr(conn->proxy_unique_id));
conn->proxy_unique_id = IST_NULL;
pool_free(pool_head_conn_hash_node, conn->hash_node);
diff --git a/src/connection.c b/src/connection.c
index c394d0f84..10bc6712f 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -499,7 +499,7 @@ int conn_recv_proxy(struct connection *conn, int flag)
case PP2_TYPE_UNIQUE_ID: {
const struct ist tlv = ist2((const char
*)tlv_packet->value, tlv_len);
- if (tlv.len > UNIQUEID_LEN)
+ if (istlen(tlv) > UNIQUEID_LEN)
goto bad_header;
conn->proxy_unique_id =
ist2(pool_alloc(pool_head_uniqueid), 0);
if (!isttest(conn->proxy_unique_id))
@@ -1384,8 +1384,8 @@ int smp_fetch_fc_pp_unique_id(const struct arg *args,
struct sample *smp, const
smp->flags = 0;
smp->data.type = SMP_T_STR;
- smp->data.u.str.area = conn->proxy_unique_id.ptr;
- smp->data.u.str.data = conn->proxy_unique_id.len;
+ smp->data.u.str.area = istptr(conn->proxy_unique_id);
+ smp->data.u.str.data = istlen(conn->proxy_unique_id);
return 1;
}
--
2.30.1
[PATCH 1/5] CLEANUP: connection: Use `VAR_ARRAY` in `struct tlv` definition
This is for consistency with `struct tlv_ssl`.
---
include/haproxy/connection-t.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/haproxy/connection-t.h b/include/haproxy/connection-t.h
index 9c2af7b8f..0bcceacff 100644
--- a/include/haproxy/connection-t.h
+++ b/include/haproxy/connection-t.h
@@ -649,7 +649,7 @@ struct tlv {
uint8_t type;
uint8_t length_hi;
uint8_t length_lo;
- uint8_t value[0];
+ uint8_t value[VAR_ARRAY];
}__attribute__((packed));
struct tlv_ssl {
--
2.30.1
Re: [Bug: 2.2.10] Regression in SRV TTL Handling + Unexpected Record Timeouts
Hi Tim, Ah, good eye! It does appear to be the same, but in my case it's due to short TTLs vs large responses. Thanks. I'll link this over there. Best, Luke — Luke Seelenbinder Stadia Maps | Founder stadiamaps.com | (864) 735-8533 > On 6 Mar 2021, at 19:03, Tim Düsterhus wrote: > > Luke, > > Am 06.03.21 um 18:57 schrieb Luke Seelenbinder: >> I just upgraded one of our HAProxy installations to 2.2.10 (on Debian using >> the from the HAProxy maintained apt repo). It appears the changes made to >> how SRV records are expired is causing issues, at least with short-lived >> TTLs in the SRV records. > > I believe this might be a duplicate of this existing issue: > https://github.com/haproxy/haproxy/issues/1160 > > Best regards > Tim Düsterhus
Re: [Bug: 2.2.10] Regression in SRV TTL Handling + Unexpected Record Timeouts
Luke, Am 06.03.21 um 18:57 schrieb Luke Seelenbinder: > I just upgraded one of our HAProxy installations to 2.2.10 (on Debian using > the from the HAProxy maintained apt repo). It appears the changes made to how > SRV records are expired is causing issues, at least with short-lived TTLs in > the SRV records. I believe this might be a duplicate of this existing issue: https://github.com/haproxy/haproxy/issues/1160 Best regards Tim Düsterhus
[Bug: 2.2.10] Regression in SRV TTL Handling + Unexpected Record Timeouts
Hi List, I just upgraded one of our HAProxy installations to 2.2.10 (on Debian using the from the HAProxy maintained apt repo). It appears the changes made to how SRV records are expired is causing issues, at least with short-lived TTLs in the SRV records. The issue I'm seeing is the record resolves, the servers stay properly set (and serving requests) until the SRV TTL expires (which in our case could be any value between 0 and 60), at which point the servers are set to no address, but this happens *before* a new record is fetched to reset the TTLs since this timeout is based on the values defined in resolves. I can play with the timeout section of resolvers to improve this situation, but it never completely fixes it, since the TTL on the SRV record could be quite low since I'm not fetching directly from our origin NS. Code snippet below: --- resolvers default nameserver … accepted_payload_size 8192 resolve_retries 4 hold valid 30s< Adjusting this to really low helps, but adds undue load on DNS, and may end up still being expired by the new "watchdog". hold obsolete 60s< Adjusting this higher means it stays up longer, but still fails to load the new record set in time hold timeout1s timeout resolve 5s timeout retry 1s --- If the TTL for the SRV records return is less than valid or obsolete, the servers will lose their address before it is updated. I would consider this a serious regression for short-lived SRV records. Thanks! Happy to provide more details if this isn't easily reproducible. Luke — Luke Seelenbinder Stadia Maps | Founder stadiamaps.com
