Hi Willy, Thanks for the feedback!
I have attached the 2 patches (with one being the same old patch and the other having the diff since that older patch) to address the comments. Not sure if this is the preferred way for sending modification based on previous patch. Let me know if you want a single patch for everything. Thanks! Ruei-Bang ________________________________ From: Willy Tarreau <w...@1wt.eu> Sent: Monday, October 23, 2023 2:57 AM To: Ruei-Bang Chen <ruec...@linkedin.com> Cc: haproxy@formilux.org <haproxy@formilux.org> Subject: Re: [PATCH] MINOR: sample: Add fetcher for getting all cookie names Hi Ruei-Bang, On Sat, Oct 21, 2023 at 12:46:18AM +0000, Ruei-Bang Chen wrote: > Hi team, > > As discussed in > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-archive.com%2Fhaproxy%40formilux.org%2Fmsg44161.html&data=05%7C01%7Cruechen%40linkedin.com%7C855894ca6f2246bcd9df08dbd3ae74f4%7C72f988bf86f141af91ab2d7cd011db47%7C0%7C0%7C638336518481646316%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=bHwuTsVLTXpQ2SpHJmwPDWt711NzAI81nuxcmMpgGKM%3D&reserved=0<https://www.mail-archive.com/haproxy@formilux.org/msg44161.html>, > I have > attached a patch for adding a new fetcher for getting all the cookie names > for request / response. > > I did check some relevant docs (like CONTRIBUTING, coding-style.txt etc.) for > the guidelines before my first submission but might still miss something. Any > feedback would be greatly appreciated. Well, then it seems that our docs are accurate because for a first submission, it's excellent, congratulations :-) I'm having one request first: > +static int smp_fetch_cookie_names(const struct arg *args, struct sample > *smp, const char *kw, void *private) > +{ > + /* possible keywords: req.cook_names / cook_names, res.cook_names / > scook_names */ > + struct channel *chn = ((kw[0] == 'c' || kw[2] == 'q') ? > SMP_REQ_CHN(smp) : SMP_RES_CHN(smp)); > + struct check *check = ((kw[0] == 's' || kw[2] == 's') ? > objt_check(smp->sess->origin) : NULL); Please do not add "cook_names" / "scook_names", we really want to only add the "req." and "res." forms nowadays as the previous ones quickly became both limited and confusing. So this means also dropping them from the keyword registration at the end. Also, one thing that I'm seeing in your vtest: server s2 { rxreq txresp -hdr "Set-Cookie: cook1=0; cook2=123; cook3=22" } -start Set-Cookie doesn't support setting multiple cookies, it only sets one cookie and the rest are attributes (expires, domain, secure etc). That's a well-known limitation of this header which is the only HTTP header that can be set multiple times but doesn't support folding. Thus when you have: Set-Cookie: blah=foo; expires=Mon, 23 Oct 2023; domain=example.com; path=/ Only "blah" is a cookie, and we must not skip to the comma and restart parsing from "23 Oct 2023 ..". This means that when dealing with Set-Cookie you'll need to only extract one field per full header. It means to me two things: - http_extract_next_cookie_name() should take an extra argument indicating whether to stop after the first semi-colon or not; - when you call http_find_header() in the loop, the last argument must be 1 (find full headers, don't stop on comma) when you're dealing with set-cookie. I think the rest is OK however. Thanks! Willy
0001-MINOR-sample-Add-fetcher-for-getting-all-cookie-name.patch
Description: 0001-MINOR-sample-Add-fetcher-for-getting-all-cookie-name.patch
0002-MINOR-sample-Get-one-cookie-name-per-Set-Cookie-resp.patch
Description: 0002-MINOR-sample-Get-one-cookie-name-per-Set-Cookie-resp.patch
