On Fri, Nov 20, 2020 at 08:21:54PM +0100, Tim Düsterhus wrote: > Am 20.11.20 um 20:13 schrieb Maciej Zdeb: > > Thank you Tim for your help and support! :)
And FWIW I totally support what Tim said about the role of the reviews and offloading developers. Your contributions are welcome, no need to feel sorry for mistakes, we all write multiple versions of our code before it gets merged and that's the normal process. What matters the most is quality. The second point that matters most is efficiency (i.e. that the work is optimally spread over participants): if it takes you one day to figure something right and 5 minutes to someone else, it's probably better that you focus on what takes you less time and that someone else helps you on this last point. So please do not apologize in the future :-) > > I've fixed some coding-style issues. I hope that all trivial mistakes > > (wrong bracing, indention etc) are now fixed. > > > > I have a question about: > > free(px->conf.lfs_file); > > px->conf.lfs_file = strdup(px->conf.args.file); > > px->conf.lfs_line = px->conf.args.line; > > > > It is on various places in http_act.c and so I've also placed that in my > > patch, however I'm not sure if it's really necessary. I just don't > > understand this fragment, why it is there. :( > > > > I might be entirely wrong there, but my understanding that these are > used for error reporting with the parsing of log-format strings and thus > should not be in your code, because you don't use log-format strings. Git grep tells me you're indeed right: $ git grep lfs_file include/ include/haproxy/proxy-t.h: char *lfs_file; /* file name where the logformat string appears (strdup) */ This is indeed used to report late errors in log-format expressions to tell the user where the faulty expression was found. So here it's indeed not needed. > I suggest waiting for an authoritative answer before adjusting anything, > though. This is just a guess based off the comments for the .lfs_file > member of the conf struct. :-) I looked at the patch, it's OK regarding this and most of the other stuff, except one thing, it mixes ACT_HTTP_* and PAT_MATCH_* and there is some breakage there, because the HTTP action being set to PAT_MATCH_STR for example sets it to value 5 which is ACT_HTTP_SET_NICE, etc. So you must only have ACT_* values in your rule->action field. Maciej, there are two possibilities here. Either you duplicate the delete action to have DEL_STR, DEL_BEG, DEL_SUB etc, and you just set the right action based on the matching method you found during parsing, or you extend act_rule.arg.http to add a matching method for the delete action. I tend to consider that the former is cleaner and simpler, given that we're really performing a few different actions there, which are all derivatives of the delete action. Many thanks for working on this one! Willy
