Hi Willy, Thanks for the clarifications. I've implemented your requested changes and split my changes in 6 consecutive patches.
I was not able to use a check function for authority and unique_id without modifying sample.c or allowing an argument. As far as I can tell, the check function is only executed for sample fetches that have not specified 0 for the arg function. I think it's okay this way since, semantically, those function do not handle arguments, so doing the argument setup internally makes sense IMO. If there is way to adjust this without any hacks, I will of course apply it. For now, I have appended them to this message to not cause to much spam on the list while also keeping the previous, related discussion. If I should send them individually, please tell me and I will do so. Restructuring, especially in regard to pooling lead to quite a bit of changes, but the overall logic still applies and should hopefully be relatively straight forward. Actually, the code even got simpler in many places! :) BTW, I also added some TLV type constants in the 6th patch, feel free to merge it or ignore it. I think it helps with readability and is not really risky. If there should be a nit that you quickly want to change, feel free to. I am not upset about it at all. Best, Alexander -----Original Message----- From: Willy Tarreau <w...@1wt.eu> Sent: Sunday, August 13, 2023 10:01 AM To: Stephan, Alexander <alexander.step...@sap.com> Cc: haproxy@formilux.org Subject: Re: [PATCH] MEDIUM: sample: Implement sample fetch for arbitrary PROXY protocol v2 TLV values [You don't often get email from w...@1wt.eu. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Hi Alexander, On Fri, Aug 11, 2023 at 02:08:37PM +0000, Stephan, Alexander wrote: > Hi Willy, > > Thanks for the nice, detailed feedback. > Overall, I agree with all of your listed points, so no need for further > discussions. ? > I will hopefully send the separated patches at the beginning of next week. OK! No rush anyway, such changes having a low impact can be merged late in the cycle if needed, so take your time. > Just a comment and two small questions to make sure I got things correct: > > > As such I'd like them to be renamed to remove this confusion. > > Maybe just call them HA_PP2_* ? > > Yes, such a prefix will clean it up quite nicely IMO. Will add that to > the first patch of the series. Thanks. > > [...] > > It may even encourage us to create > > smaller pools if ever deemed really useful (e.g. a 4- or 8- byte > > load balancing hash key for end-to-end consistency would only take a > > total of 32 or 40, malloc included). > > Just to make sure: Right now, we don't want to optimize further for > small TLVs other than removing the second pool for the values and > using the new struct with the VLA to reduce the overhead. > For normal use, a pool with 160 B could be used now to accommodate > for the new conn_tlv_list struct and 128 B TLVs (e.g., UNIQUE_ID)? > For the authority type, it would then be a 255 + 32 B sized pool? > Maybe that could be used for the value range 128 <= x <= 255, and > then, for > 255, malloc? Yes I think that will do the job (more like 128 < x < 256 BTW). I think you'd rather just focus on the type size (like you did) and not on the type itself, so that pools will automatically fit (i.e. have 128+sizeof(conn_tlv_list) and 256+sizeof() and the rest is for malloc(). > Other, smaller pools are future work? Yes, as I'm not sure they're really that much used, and it will be easy to create smaller pools if we figure they're needed. > > struct conn_tlv_list { > > struct list list; > > unsigned short len; // 65535 should be more than enough! > > unsigned char type; > > char contents[0]; > > }; > > I am also a bit confused about the second struct variant of this. IMO > this is the optimal struct layout in regards to size, that I would like to > use. > What is the other struct for, where `len` and `type` are switched? Ah, at first I didn't know what you were talking about, I remember having added the type while writing the message, it's just that I poorly pasted it the second time :-) It's better to keep it as above, where types are better aligned and leave no hole. Hmmm BTW the struct will be padded, so you should use offsetof(conn_tlv_list, contents) rather than sizeof(conn_tlv_list) for the size calculations. Or you can mark the struct with __attribute__((packed)), it will do the job as well. It's up to you. > Thanks again for your efforts, it's really interesting for me to work > on HAProxy. You're welcome, do not hesitate to send any question you may have. Cheers, Willy
0001-CLEANUP-MINOR-connection-Improve-consistency-of-PPv2.patch
Description: 0001-CLEANUP-MINOR-connection-Improve-consistency-of-PPv2.patch
0002-MEDIUM-connection-Generic-list-based-allocation-and-.patch
Description: 0002-MEDIUM-connection-Generic-list-based-allocation-and-.patch
0003-MEDIUM-sample-Add-fetch-for-arbitrary-TLVs.patch
Description: 0003-MEDIUM-sample-Add-fetch-for-arbitrary-TLVs.patch
0004-MINOR-sample-Refactor-fc_pp_authority-by-wrapping-th.patch
Description: 0004-MINOR-sample-Refactor-fc_pp_authority-by-wrapping-th.patch
0005-MINOR-sample-Refactor-fc_pp_unique_id-by-wrapping-th.patch
Description: 0005-MINOR-sample-Refactor-fc_pp_unique_id-by-wrapping-th.patch
0006-MINOR-sample-Add-common-TLV-types-as-constants-for-f.patch
Description: 0006-MINOR-sample-Add-common-TLV-types-as-constants-for-f.patch
