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://github.com/haproxy/haproxy/blob/47ed1181f29a56ccb04e5b80ef4f9414466ada7a/src/server.c#L2370 > 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
