Hi Willy, Thanks again for the merge, very glad that it made its way into 2.9.
> I don't see how this is possible: > > list_for_each_entry(srv_tlv, &src->pp_tlvs, list) { > if (srv_tlv == NULL) > break; > For srv_tlv to be NULL, it would mean that you visited (NULL)->list at some > point, and apparently pp_tlvs is always manipulated with > LIST_APPEND() only, so I don't see how you could end up with something like > last->next = (NULL)->list. Are you sure it was not a side effect of a > debugging session with some temporary code in it ? > I'd be interested in knowing if you can reproduce it so that we can find the > root cause (and hopefully address it). I finally got back to this again. You should be able to reproduce it quite easily. After removing the 'break' above, you can run the regression tests. Then, tests which (presumably) use the server settings copy function fail or timeout. Probably an even better method is to use the following configuration 'haproxy.cfg': global defaults log global timeout connect 500ms timeout client 5000ms timeout server 50000ms backend dummy mode tcp default-server send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[fc_pp_tlv(0xE1)] server dummy_server 127.0.0.1:2319 frontend dummy log stdout local0 mode tcp bind *:2320 accept-proxy default_backend dummy Run it with: gdb --args ./haproxy '-f' './haproxy.cfg' It should crash immediately with the following seg fault: Thread 1 "haproxy" received signal SIGSEGV, Segmentation fault. srv_settings_cpy (srv=srv@entry=0xbed7e0, src=src@entry=0xbebce0, srv_tmpl=srv_tmpl@entry=0) at src/server.c:2526 2526 new_srv_tlv->fmt_string = strdup(srv_tlv->fmt_string); Overall, I am not too confident with the order of invocations in the server. Maybe we have some initializations mixed up and unintuitive some dependencies. > Another point, in make_proxy_line_v2() I'm seeing that you've placed > everything under "if (strm)". I understand why, it's because you didn't want > to call build_logline() without a stream. That made > me think "hmmm we > already have the ability to do that", and I found it, you can use > sess_build_logline() instead, that takes the session separately, and supports > an optional stream. That would be useful for health checks for example, since > they don't have streams. But there's a catch: in function > make_proxy_line_v2() we don't have the session at the moment, and > build_logline() extracts it from the stream. Normally during a session > establishment, the session is present in connection->owner, thus > remote->owner in make_proxy_line_v2(). I suggest that you give this a > try to confirm that it works for you, this way we'd be sure that health > checks can also send the ppv2 fields. But that's not critical given that > there's no bad behavior right now, it's just that fields will be ignored, so > this can be done in a subsequent patch. I looked into it again. I did not see an obvious way to retrieve the current or pass the session as parameter. I mean, the easy option, as you said, would be to use remote->owner, which would require replacing if (stream) with if (remote) to prevent the NULL dereference. But we still want to send out TLVs when there is no remote. So maybe I could change it such that it uses remote->owner when available and otherwise, falls back to using the session from the stream? Maybe something along the lines of struct session *sess = (remote != NULL) ? remote->owner : strm->sess; replace->data = sess_build_logline(sess, strm, replace->area, replace->size, &srv_tlv->fmt); ? It seems to achieve the behavior you described regarding the health checks, right? If that's alright, I will send the corresponding patch. Best, Alexander -----Original Message----- From: Willy Tarreau <w...@1wt.eu> Sent: Saturday, November 4, 2023 5:02 AM To: Stephan, Alexander <alexander.step...@sap.com> Cc: haproxy@formilux.org Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options Alexander, I now merged your patch with the SMP_VAL_ change, after verifying that the reg-test is still OK. Thus 2.9-dev9 will contain it. Thanks! Willy