Modify vmod-xkey purge() to accept multiple space-delimited keys

[pberkel]

The vmod-xkey module allows multiple space-delimited keys to be set per object, e.g.

xkey: abcd efgh ijkl mnop

However the purge() and softpurge() methods currently only support a single input key:

xkey.purge('abcd');

This pull request simply replicates the string-processing logic from xkey_cb_insert() and applies it in purge() so that the purge() and softpurge() methods support a space-delimited list of keys, e.g:

xkey.purge("abcd efgh ijkl mnop"); xkey.softpurge("abcd efgh ijkl mnop");

This allows multiple keys to be purged / soft-purged in a single request.

While the changeset seems large, it's mainly due to indentation (and unit test), there were no changes to the core logic in the purge() function.

[dridi]

The less scary diff: https://github.com/varnish/varnish-modules/pull/47/files?w=1

[dridi]

LGTM, not sure I should merge it myself.

[dridi]

Hi @pberkel, please introduce the changes we discussed before i merge your changes in. We've discussed it with the team and it's good to go.

Thanks!

[pberkel]

Thanks @Dridi I have actually been considering strtok_r() with delimiter " \t" (i.e. space and tab, similar to isblank(3) spec) a do you think this is safe to use in this case?

[dridi]

I don't see any reason not to use it, feel free to switch to strtok_r and I'll do the review.

[pberkel]

I started implementing strtok_r which was fine in purge() but got complicated in xkey_cb_insert() because I didn't want to mess with objcore and then having to use strncpy() for each xkey / X-HashTwo header seemed inefficient, so I abandoned that approach.

I did make changes to the whitespace character test to use isspace() instead of (*sp == ' ') as advised, which seemed to work well (also adjusted my newly added test07 to include tab and multiple-whitespace characters to prove correctness).

I did discover a small but problematic bug introduced by my initial commit, whereby if a xkey-purge value was supplied that didn't match any existing key if (hashhead == NULL) then continue was called without "sp = ep;" causing an infinite loop. This has been fixed by reversing the check logic to (hashhead != NULL) and removing the continue so that "sp = ep;" will always be reached regardless. Another test was added (test08.vtc) to check for this condition.

Regarding the initial comments about the test being confusing, I reviewed the code again and couldn't find any simpler (or more efficient) way to implement the whitespace splitting logic, but if there are any final suggestions I would be happy to include them in my pull request.

Regards.

[pberkel]

For reference, the diff ignoring whitespace is much easier to follow: https://github.com/varnish/varnish-modules/pull/47/files?w=1

[dridi]

I did make changes to the whitespace character test to use isspace() instead of (*sp == ' ') as advised

I'm sure I advised isblank, there's a huge difference between the two!

Regarding the initial comments about the test being confusing

I will have a closer look, but I still think that my initial suggestion would make things simpler to reason about. I will review the new changes after I see isspace disappear ;)

[pberkel]

I'm sure I advised isblank, there's a huge difference between the two!

Quite right you are, apologies for that, I was viewing code in vmod_cookie.c which used isspace() and used that function instead, latest commit has corrected this.

The way I read the loop logic is:

while (isblank(*sp)) sp++;

• Trims any blank spaces preceding the key.

ep = sp;

• Marks sp (start pointer) as the possible start of key, or end-of-string, and continues with ep.

while (*ep != '\0' && !isblank(*ep)) ep++;

• increment end pointer over non-null, non-blank characters that will form the key

if (sp == ep) break;

• if the previous loop didn't result in any increment, then we've reached the end of string (logically both sp and ep must be '\0'), break out of loop.

... do stuff with the key here ...

sp = ep;

• set the start pointer to the end pointer (which is either blank or null byte), then back to the main while check for non-null byte.

It's not elegant, but safely handles input strings with leading and / or trailing blank space effectively.

UPDATE are you suggesting that if (sp == ep) break; be changed to if (sp == '\0') break;?

I definitely welcome any improvements and will incorporate any optimisation suggestions that into my pull request.

[pberkel]

@Dridi the PR has been updated with your suggested change, all tests passed on my machine (UPDATE: I note that the travis-ci check failed although it doesn't appear to be related to any code changes from this PR). Is this now safe to merge into master? Thanks in advance!

[dridi]

Yeah sorry, I don't have time yet but I'll come back to this soon.

[andrerom]

Any news on this?

A bit of a stretch, but it would greatly simplify usage from client side, so if this makes it in the debian scratch package before freeze (soft janurary, hard in february) it would make sure we don't "have to" support two different api's for this module from fos http cache lib side.

[dridi]

I'm reviewing this today, unlike a previous reviews done directly on github, I will have a more thorough look at the code on my machine.

[dridi]

On closer inspection, do we want to lock for each key, or once for the whole loop? I think locking once would be preferable, and I don't see anything in the code that would make it unsafe.

@mbgrydeland, can you confirm?

Nitpicking: I will fix the code style.

[dridi]

I went ahead and pushed 139aa37 and f723b4c. I'm keeping this open so you can confirm it works as intended. I can't make any promises regarding a release meeting Debian's schedule.

[pberkel]

Thanks @Dridi! I've tested the two commits made to the master branch and they appear to work as expected. Will continue to test over the next few days, however I feel it's safe to close this Pull Request. Best Wishes!

[wimleers]

Very glad to see this fixed. This was indeed very confusing (multiple keys per object supported, but purging multiple keys unsupported). Thanks all! :)