Re: [PATCH 0/6] 'ist'ify members of struct proxy
Willy, On 3/15/22 08:26, Willy Tarreau wrote: Or perhaps you could ask and include me in Cc, at least people already know you. I'll be happy to further improve the existing Coccinelle patches and to further 'ist'ify the codebase, but would need some handholding to get me started. Note that beyond changing code, one thing that would be extremely useful would be to build up a collection of patches to detect certain classes of bugs. I wrote some scripts that used to match improbable expressions like: unlikely(E) < 0 But I can't figure how to make them match anymore, or they match too much thus for me the behavior has always been a bit random and I never know if I can trust the absence of a match. If you send over your existing Coccinelle files I can have a look into cleaning them up and then committing them. Overall this is still a fantastic tool but it requires a significant and sustained investment to really unleash its power. Unfortunately in a small project like haproxy most often it takes less time to perform a few changes by hand than to try to figure a reliable way to express what we want. At least for the ist.cocci new cases pop up every now and then and the rules within that file are all very very simple. So for that the investment into writing the patches is worth it I believe. I think the hard part is not so much "performing the changes by hand", but rather "reliably finding the places by hand". Using grep to find accesses of '.ptr' for an ist is going to turn up all kinds of unrelated stuff. Best regards Tim Düsterhus
Re: [PATCH 0/6] 'ist'ify members of struct proxy
Hi Tim, On Fri, Mar 11, 2022 at 09:15:48PM +0100, Tim Düsterhus wrote: > Yeah, I've attempted to look into the Coccinelle patches in the Linux kernel > sources, but I agree that many of those are very complex :-) > > Do you happen to know where we could ask for assistance with making the > necessary adjustments to the patches? Not really. I'm seeing that the doc on the site was updated less than a year ago, I should probably re-read it (I had only seen a very old one with a bunch of examples and too few tips): https://coccinelle.gitlabpages.inria.fr/website/ There's also a mailing list (cocci at inria.fr) which is quite active, but it looks like Julia is very responsive on it and I don't want to bother her with end-user questions when she likely has more important things to spend her time on. > Or perhaps you could ask and include > me in Cc, at least people already know you. I'll be happy to further improve > the existing Coccinelle patches and to further 'ist'ify the codebase, but > would need some handholding to get me started. Note that beyond changing code, one thing that would be extremely useful would be to build up a collection of patches to detect certain classes of bugs. I wrote some scripts that used to match improbable expressions like: unlikely(E) < 0 But I can't figure how to make them match anymore, or they match too much thus for me the behavior has always been a bit random and I never know if I can trust the absence of a match. Overall this is still a fantastic tool but it requires a significant and sustained investment to really unleash its power. Unfortunately in a small project like haproxy most often it takes less time to perform a few changes by hand than to try to figure a reliable way to express what we want. Willy
Re: [PATCH 0/6] 'ist'ify members of struct proxy
Willy, [Dropping Christopher from Cc] On 3/9/22 08:11, Willy Tarreau wrote: As for the second CLEANUP commit: If one of you knows how to fix the Coccinelle patch to detect that specific pattern, I'd appreciate if you could make the necessary changes to ist.cocci. Unfortunately my Coccinelle skills are not good enough. I've already faced this situation where it didn't find fields of a given type inside a structure, and I stopped searching when I figured that by the time I would finally find, I could have replaced 10 times the 3 occurrences I needed. I essentially use Coccinelle as a helpful tool to save me time, and I can't resolve myself to spend more time trying to write unmaintainable scripts that will never be reused. A good source of inspiration are the scripts in the linux kernel, but those are often of an extreme level of complexity and mix python scripting within the patch, resulting in me not understanding anything anymore. But some of them are still human-readable or at least show the syntax hacks you're looking for. Yeah, I've attempted to look into the Coccinelle patches in the Linux kernel sources, but I agree that many of those are very complex :-) Do you happen to know where we could ask for assistance with making the necessary adjustments to the patches? Or perhaps you could ask and include me in Cc, at least people already know you. I'll be happy to further improve the existing Coccinelle patches and to further 'ist'ify the codebase, but would need some handholding to get me started. Best regards Tim Düsterhus
Re: [PATCH 0/6] 'ist'ify members of struct proxy
Hi Tim, On Sat, Mar 05, 2022 at 12:52:39AM +0100, Tim Duesterhus wrote: > Willy, > Christopher, > > find a series that converts a few members of `struct proxy` into ists. All > of them have already been converted into ists when operating on them, so > directly storing them as ists makes that code cleaner. Now applied, thank you for doing this, it's always good to see such remains of old code diminish over time. If you're interested in chasing other ones, I'm seeing that the following commands seem to yield likely candidates: $ git grep -B1 '_len;' include > One drawback is that `struct proxy` grows by 16 bytes. It might or might not > be necessary to reorder the struct members to keep it efficient. Sure but 16 bytes for such a huge struct is not much, and indeed there are a few holes that we may repack later: $ pahole -C proxy ./haproxy /* size: 6944, cachelines: 109, members: 133 */ /* sum members: 6924, holes: 5, sum holes: 20 */ /* paddings: 1, sum paddings: 4 */ /* last cacheline: 32 bytes */ > The `server_id_hdr_name` one is tagged MEDIUM, because that required some > non-trivial changes in the FCGI implementation. As I've needed to modify > that code anyway, I've also added two additional CLEANUP commits. For me these looked good. > As for the second CLEANUP commit: If one of you knows how to fix the > Coccinelle > patch to detect that specific pattern, I'd appreciate if you could make the > necessary changes to ist.cocci. Unfortunately my Coccinelle skills are not > good enough. I've already faced this situation where it didn't find fields of a given type inside a structure, and I stopped searching when I figured that by the time I would finally find, I could have replaced 10 times the 3 occurrences I needed. I essentially use Coccinelle as a helpful tool to save me time, and I can't resolve myself to spend more time trying to write unmaintainable scripts that will never be reused. A good source of inspiration are the scripts in the linux kernel, but those are often of an extreme level of complexity and mix python scripting within the patch, resulting in me not understanding anything anymore. But some of them are still human-readable or at least show the syntax hacks you're looking for. > I've tested that HAProxy compiles and that the existing reg-tests pass, but > I didn't specifically test FCGI (and there are no exiting reg-tests for that). There are indeed no regtest because we don't have an fcgi server either in vtest nor in haproxy. > So please carefully check the patches for dumb mistakes. For me that was OK so I merged them. If we later find a regression, we'll both be co-responsible :-) Thanks, Willy
[PATCH 0/6] 'ist'ify members of struct proxy
Willy, Christopher, find a series that converts a few members of `struct proxy` into ists. All of them have already been converted into ists when operating on them, so directly storing them as ists makes that code cleaner. One drawback is that `struct proxy` grows by 16 bytes. It might or might not be necessary to reorder the struct members to keep it efficient. The `server_id_hdr_name` one is tagged MEDIUM, because that required some non-trivial changes in the FCGI implementation. As I've needed to modify that code anyway, I've also added two additional CLEANUP commits. As for the second CLEANUP commit: If one of you knows how to fix the Coccinelle patch to detect that specific pattern, I'd appreciate if you could make the necessary changes to ist.cocci. Unfortunately my Coccinelle skills are not good enough. I've tested that HAProxy compiles and that the existing reg-tests pass, but I didn't specifically test FCGI (and there are no exiting reg-tests for that). So please carefully check the patches for dumb mistakes. Best regards Tim Duesterhus (6): MINOR: proxy: Store monitor_uri as a `struct ist` MINOR: proxy: Store fwdfor_hdr_name as a `struct ist` MINOR: proxy: Store orgto_hdr_name as a `struct ist` MEDIUM: proxy: Store server_id_hdr_name as a `struct ist` CLEANUP: fcgi: Replace memcpy() on ist by istcat() CLEANUP: fcgi: Use `istadv()` in `fcgi_strm_send_params` include/haproxy/proxy-t.h | 12 -- src/cfgparse-listen.c | 46 --- src/http_ana.c| 11 -- src/mux_fcgi.c| 23 ++-- src/mux_h1.c | 8 +++ src/mux_h2.c | 8 +++ src/proxy.c | 37 +-- 7 files changed, 62 insertions(+), 83 deletions(-) -- 2.35.1
