Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vmod_debug: add debug.chksha256 / debug.chkcrc32 VDPs #4208

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

nigoroll
Copy link
Member

@nigoroll nigoroll commented Oct 7, 2024

Usually we do not require PRs for vmod_debug additions, but in this case I feel more comfortable asking if anyone has better ideas:

Working on storage engines and delivery changes, I would like to have a simple way to ensure for longer tests (for example using wrk) that delivered body data is correct. While implementing checks in the http client would ensure proper end-to-end validation, the task of handling errors looks tedious, because, for hunting bugs efficiently, getting a core dump of varnishd for failing tests is basically a requirement.

Panicking varnishd from a VDP at the right place is easy and independent of protocols. So this is a suggestion to add body checksum check VDPs.

The code has some duplications, but with just SHA256 and CRC32 I consider the savings of generalizing not worthy.

@dridi
Copy link
Member

dridi commented Oct 7, 2024

Finally, it pays off in CI :]

FAIL: tools/magic_check.sh
==========================

Duplicate magic number:
#define VDP_CHKSHA256_MAGIC		0x6856e913
#define VDP_CHKCRC32_MAGIC		0x6856e913

FAIL tools/magic_check.sh (exit status: 1)

We should maybe move vmod_debug filter coverage to a dedicated vmod_debug_filters.c because vmod_debug.c is a bit crowded. Maybe that's good as it is.

Not a review, but since you are seeking an opinion, no objection on my end. I'm not convinced by the mechanics for panicking at first glance, but don't have time to look closer. If you want to use this outside of varnishtest and out of tree, maybe vmod_vtc would be a better place, or a dedicated vmod_chk where we could bundle various checks.

@nigoroll
Copy link
Member Author

nigoroll commented Oct 7, 2024

Thank you, @dridi ! The magic is fixed. I agree that vmod_debug.c could be split into smaller files, I would prefer to do this after merge to not burn effort on text formatting. Panicking turned out to be helpful to me, but the "log only" option exists where it is not. And yes, we could add more options if needed. I also thought about a more generic usecase scenario, but the panic options (which is important to me) seems like a bad idea outside vmod_debug.

@nigoroll
Copy link
Member Author

nigoroll commented Oct 7, 2024

bugwash: ok, split source files

…tegrity

... from within varnish, which does not allow to check for issues in the
transport, but is useful for validating storage and any previous VDPs in the
filter list.

crc32 has been added as an option with higher performance, because the algorithm
already exists in-tree.
@nigoroll nigoroll merged commit 9c34716 into varnishcache:master Oct 7, 2024
1 of 10 checks passed
@nigoroll nigoroll deleted the bodycheck branch October 7, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants