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 <w...@1wt.eu> Sent: Friday, November 3, 2023 8:11 PM 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 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