Hi Willy,
Thanks for the review. No problem for calling me Stephan, I am totally used to
that, my teachers did that for years.
> At first glance the patches look nice.
Great! 😊
> Yeah I agree, that part is not pretty. And as often in such code, not
> handling errors makes them even more complicated to unroll as you experienced
> in the list loop you added, which could otherwise have been addressed using
> just a goto and a few free().
> Do not hesitate to propose extra patches to improve this, contributions that
> make the code more reliable and maintainable are totally welcome!
Sure, if I have some extra time on my hands. Actually, a colleague of mine is
preparing some patches like this. Since there are many small changes (e.g.,
adding a free), should can we batch them or are individual commits required?
> Are you sure it was not a side effect of a debugging session with some
> temporary code in it ?
Pretty sure, yes. I'll will get back to this at the beginning of next week, if
it's okay. I compiled with a debug flag, but I wouldn't usually expect this to
be an issue.
> So if that's OK for you I can change it now before merging.
Ah, I had used a SMP_VAL_* before, but I was not 100% about the meaning. Then I
fell back to the proxy. Feel free to change it!
> Yes very likely. Originally the code didn't check for allocation errors
> during parsing because it was the boot phase, and we used to consider that a
> memory allocation error would not always be reportable anyway, and given that
> it was at boot time, the result was going to be the same.
> But much later the code began to be a bit more dynamic, and such practices
> are no longer acceptable in parts that can be called at run time (e.g.
> during an "add server" on the CLI). It's particularly difficult to address
> some of these historic remains but from time to time we manage to pass an
> extra pointer to some functions to write an error, and make a function return
> a set of flags among ERR_{WARN,FATAL,ALERT,ABORT}. But it takes a lot of time
> > for little to no perceived value for the vast majority of users :-(
Interesting how that came to be, no accusations. I see that this is not a high
priority. Not sure if I have the time to provide a fix here but I'll keep it
mind.
> 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.
Again, I don't mind if you make the SMP_VAL_*. I tried the
sess_build_logline() and it seems to work perfectly. The tests also still pass,
so looks good. I would like to provide a second patch for this next week, if
this okay.
Best,
Alexander
-----Original Message-----
From: Willy Tarreau <[email protected]>
Sent: Friday, November 3, 2023 8:11 PM
To: Stephan, Alexander <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
Hi Alexander,
(and BTW sorry for having called you Stephan twice in the last thread, each
time I have to make a mental effort due to how your first and last names are
presented in your e-mail address).
On Sat, Oct 28, 2023 at 07:32:20PM +0000, Stephan, Alexander wrote:
> I've just finished the implementation based on the tip with the
> deferred log format parsing so the default-server should be also fully
> supported now. ?
At first glance the patches look nice.
> I guess using the finalize method of the parser is the canonic implementation
> of this.
>
> I have a few remarks about the current state of the code:
> - srv_settings_cpy in server.c has no form of error handling
> available. This is potentially causes trouble due to lack of error handling:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fhaproxy%2Fhaproxy%2Fblob%2F47ed1181f29a56ccb04e5b80ef4f941446
> 6ada7a%2Fsrc%2Fserver.c%23L2370&data=05%7C01%7Calexander.stephan%40sap
> .com%7Cd49f35f800cd493ad76408dbdca0934d%7C42f7676cf455423c82f6dc2d9979
> 1af7%7C0%7C0%7C638346354457583668%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C
> &sdata=I%2F6tjWMJMtBXWqA%2F%2FUmMzO2ZDbuWc7WktP9J0bQPZko%3D&reserved=0
> For now, I did not want to change the signature, but I think, error
> handling would be beneficial here.
Yeah I agree, that part is not pretty. And as often in such code, not handling
errors makes them even more complicated to unroll as you experienced in the
list loop you added, which could otherwise have been addressed using just a
goto and a few free().
Do not hesitate to propose extra patches to improve this, contributions that
make the code more reliable and maintainable are totally welcome!
> - In the new list_for_each in srv_settings_cpy I have to check for
> srv_tlv == NULL as for some reason, the list contains NULL entries. I
> don't see any mistakes with the list handling. Maybe it is just due to
> the structure of the server logic.
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).
> - Please double check that my arguments for the parse_logformat_string
> function are correct. I omit log options for now and use the
> capabilities of the proxy. Seems like the best fit, but I could be wrong.
Ah good point. The argument you're missing and for which use used
px->cap is one of SMP_VAL_*. It indicates at which point(s) the
log-format may be called so that the parser can emit suitable warnings if some
sample fetch functions are not available. Here it will be used during the
server connect() phase, so there is already one perfect for this, which is
SMP_VAL_BE_SRV_CON. It's already used by the source and sni arguments. For the
options I think it's OK that it stays zero, that's how it's used everywhere
else :-)
I could verify that your reg test still works with this change:
--- a/src/server.c
+++ b/src/server.c
@@ -3252,7 +3252,7 @@ static int _srv_parse_finalize(char **args, int cur_arg,
list_for_each_entry(srv_tlv, &srv->pp_tlvs, list) {
LIST_INIT(&srv_tlv->fmt);
if (srv_tlv->fmt_string &&
unlikely(!parse_logformat_string(srv_tlv->fmt_string,
- srv->proxy, &srv_tlv->fmt, 0, px->cap, &errmsg))) {
+ srv->proxy, &srv_tlv->fmt, 0, SMP_VAL_BE_SRV_CON,
&errmsg))) {
if (errmsg) {
ha_alert("%s\n", errmsg);
free(errmsg);
So if that's OK for you I can change it now before merging.
> - I noticed that there are also no checks for strdup in server.c, that
> might need to be addressed in the future. For the srv_settings_cpy I
> cannot give an error, but only try to avoid the memory leak.
Yes very likely. Originally the code didn't check for allocation errors during
parsing because it was the boot phase, and we used to consider that a memory
allocation error would not always be reportable anyway, and given that it was
at boot time, the result was going to be the same.
But much later the code began to be a bit more dynamic, and such practices are
no longer acceptable in parts that can be called at run time (e.g.
during an "add server" on the CLI). It's particularly difficult to address some
of these historic remains but from time to time we manage to pass an extra
pointer to some functions to write an error, and make a function return a set
of flags among ERR_{WARN,FATAL,ALERT,ABORT}. But it takes a lot of time for
little to no perceived value for the vast majority of users :-(
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.
Overall I'm fine with merging this as-is, preferably with the small change
above regarding SMP_VAL_* (otherwise we can fix it later but I guess you'd
prefer a clean patch as well ;-)). Just let me know what you prefer.
Thank you!
Willy
