From 37e25bb8d6e7def07bf41914394edf64bcfbb6d9 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 07:21:03 +0100 Subject: [PATCH 1/8] param: Clear least significant bits with none The most significant bits of the least significant octet would be omitted when the number of bits is not a multiple of eight. --- bin/varnishd/mgt/mgt_param_tweak.c | 11 +++++++++-- bin/varnishtest/tests/c00054.vtc | 17 +++-------------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index 6990f1b79f0..0c5301b837e 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -607,6 +607,13 @@ bit(uint8_t *p, unsigned no, enum bit_do act) return (*p & b); } +static inline void +bit_clear(uint8_t *p, unsigned l) +{ + + memset(p, 0, (l + 7) >> 3); +} + /*-------------------------------------------------------------------- */ @@ -665,14 +672,14 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, if (arg != NULL && !strcmp(arg, "default") && strcmp(par->def, "none")) { - memset(p, 0, l >> 3); + bit_clear(p, l); return (tweak_generic_bits(vsb, par, par->def, p, l, tags, desc, sign)); } if (arg != NULL && arg != JSON_FMT) { if (sign == '+' && !strcmp(arg, "none")) - memset(p, 0, l >> 3); + bit_clear(p, l); else return (bit_tweak(vsb, p, l, arg, tags, desc, sign)); } else { diff --git a/bin/varnishtest/tests/c00054.vtc b/bin/varnishtest/tests/c00054.vtc index 420386b89a1..6730a6baa5e 100644 --- a/bin/varnishtest/tests/c00054.vtc +++ b/bin/varnishtest/tests/c00054.vtc @@ -1,13 +1,5 @@ varnishtest "bitmap params masking" - -server s1 { - rxreq - txresp -} -start - -varnish v1 -vcl+backend {} -start - varnish v1 -cliok "param.show vsl_mask" varnish v1 -cliok "param.set vsl_mask -VCL_trace" varnish v1 -cliok "param.show vsl_mask" @@ -15,6 +7,9 @@ varnish v1 -cliok "param.set vsl_mask -WorkThread,-TTL" varnish v1 -cliok "param.show vsl_mask" varnish v1 -cliok "param.set vsl_mask +WorkThread,+TTL,+Hash" varnish v1 -cliok "param.show vsl_mask" + +varnish v1 -cliexpect {"value": "none"} "param.set -j feature none" + varnish v1 -clierr 106 "param.set vsl_mask FooBar" varnish v1 -clierr 106 "param.set vsl_mask -FooBar" varnish v1 -clierr 106 {param.set vsl_mask \"} @@ -24,9 +19,3 @@ varnish v1 -cliok "param.show debug" varnish v1 -cliok "param.show feature" varnish v1 -cliok "param.set feature +short_panic" varnish v1 -cliok "param.show feature" - - -client c1 { - txreq - rxresp -} -run From 00b5554697b24bdd3c3339345ee57337e97fa36e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 07:25:08 +0100 Subject: [PATCH 2/8] param: New "all" counterpart to "none" for vsl_mask --- bin/varnishd/mgt/mgt_param_tweak.c | 4 +++- bin/varnishtest/tests/c00054.vtc | 1 + include/tbl/params.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index 0c5301b837e..bfd0f2beef6 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -680,6 +680,8 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, if (arg != NULL && arg != JSON_FMT) { if (sign == '+' && !strcmp(arg, "none")) bit_clear(p, l); + else if (sign == '-' && !strcmp(arg, "all")) + bit_clear(p, l); else return (bit_tweak(vsb, p, l, arg, tags, desc, sign)); } else { @@ -693,7 +695,7 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, } } if (*s == '\0') - VSB_cat(vsb, sign == '+' ? "none" : "(all enabled)"); + VSB_cat(vsb, sign == '+' ? "none" : "all"); if (arg == JSON_FMT) VSB_putc(vsb, '"'); } diff --git a/bin/varnishtest/tests/c00054.vtc b/bin/varnishtest/tests/c00054.vtc index 6730a6baa5e..63aef00a15d 100644 --- a/bin/varnishtest/tests/c00054.vtc +++ b/bin/varnishtest/tests/c00054.vtc @@ -9,6 +9,7 @@ varnish v1 -cliok "param.set vsl_mask +WorkThread,+TTL,+Hash" varnish v1 -cliok "param.show vsl_mask" varnish v1 -cliexpect {"value": "none"} "param.set -j feature none" +varnish v1 -cliexpect {"value": "all"} "param.set -j vsl_mask all" varnish v1 -clierr 106 "param.set vsl_mask FooBar" varnish v1 -clierr 106 "param.set vsl_mask -FooBar" diff --git a/include/tbl/params.h b/include/tbl/params.h index 073e5be3b38..de16b146ea7 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1881,6 +1881,7 @@ PARAM_BITS( "-WorkThread", /* descr */ "Mask individual VSL messages from being logged.\n" + "\tall\tEnable all tags\n" "\tdefault\tSet default value\n" "\nUse +/- prefix in front of VSL tag name to unmask/mask " "individual VSL messages.") From 08742bcedbf9007fce02b2a92ac210a8c7c16c9e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 07:34:58 +0100 Subject: [PATCH 3/8] param: Set all and none as bit parameters values As opposed to special values. This enables setting "absolute" values atomically: param.set foo none,+bar,+baz --- bin/varnishd/mgt/mgt_param_tweak.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index bfd0f2beef6..0a8e1e595e0 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -634,6 +634,14 @@ bit_tweak(struct vsb *vsb, uint8_t *p, unsigned l, const char *arg, } for (i = 1; av[i] != NULL; i++) { s = av[i]; + if (sign == '+' && !strcmp(s, "none")) { + bit_clear(p, l); + continue; + } + if (sign == '-' && !strcmp(s, "all")) { + bit_clear(p, l); + continue; + } if (*s != '-' && *s != '+') { VSB_printf(vsb, "Missing '+' or '-' (%s)\n", s); VAV_Free(av); @@ -678,12 +686,7 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, } if (arg != NULL && arg != JSON_FMT) { - if (sign == '+' && !strcmp(arg, "none")) - bit_clear(p, l); - else if (sign == '-' && !strcmp(arg, "all")) - bit_clear(p, l); - else - return (bit_tweak(vsb, p, l, arg, tags, desc, sign)); + return (bit_tweak(vsb, p, l, arg, tags, desc, sign)); } else { if (arg == JSON_FMT) VSB_putc(vsb, '"'); From 95669b4a512995531d43c61208a82bf9b399d14d Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 07:49:30 +0100 Subject: [PATCH 4/8] param: Remove unncessary else branch Better diff with the --ignore-all-space option. --- bin/varnishd/mgt/mgt_param_tweak.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index 0a8e1e595e0..f1ac4d4b5de 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -685,23 +685,22 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, desc, sign)); } - if (arg != NULL && arg != JSON_FMT) { + if (arg != NULL && arg != JSON_FMT) return (bit_tweak(vsb, p, l, arg, tags, desc, sign)); - } else { - if (arg == JSON_FMT) - VSB_putc(vsb, '"'); - s = ""; - for (j = 0; j < l; j++) { - if (bit(p, j, BTST)) { - VSB_printf(vsb, "%s%c%s", s, sign, tags[j]); - s = ","; - } + + if (arg == JSON_FMT) + VSB_putc(vsb, '"'); + s = ""; + for (j = 0; j < l; j++) { + if (bit(p, j, BTST)) { + VSB_printf(vsb, "%s%c%s", s, sign, tags[j]); + s = ","; } - if (*s == '\0') - VSB_cat(vsb, sign == '+' ? "none" : "all"); - if (arg == JSON_FMT) - VSB_putc(vsb, '"'); } + if (*s == '\0') + VSB_cat(vsb, sign == '+' ? "none" : "all"); + if (arg == JSON_FMT) + VSB_putc(vsb, '"'); return (0); } From 81a85bcacc78acefbd5438fae18aabbcb137efa0 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 07:58:19 +0100 Subject: [PATCH 5/8] param: Show bits parameters as absolute values --- bin/varnishd/mgt/mgt_param_tweak.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index f1ac4d4b5de..82a1e192adc 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -675,7 +675,6 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, uint8_t *p, unsigned l, const char * const *tags, const char *desc, char sign) { - const char *s; unsigned j; if (arg != NULL && !strcmp(arg, "default") && @@ -690,15 +689,11 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, if (arg == JSON_FMT) VSB_putc(vsb, '"'); - s = ""; + VSB_cat(vsb, sign == '+' ? "none" : "all"); for (j = 0; j < l; j++) { - if (bit(p, j, BTST)) { - VSB_printf(vsb, "%s%c%s", s, sign, tags[j]); - s = ","; - } + if (bit(p, j, BTST)) + VSB_printf(vsb, ",%c%s", sign, tags[j]); } - if (*s == '\0') - VSB_cat(vsb, sign == '+' ? "none" : "all"); if (arg == JSON_FMT) VSB_putc(vsb, '"'); return (0); From 55cff9a031d868310c9e62bf7a3c8c8c1a5e4173 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 09:06:01 +0100 Subject: [PATCH 6/8] param: Give all bits parameters absolute defaults This is primarily for self-documentation purposes, but also to simplify the bit tweak. --- bin/varnishd/mgt/mgt_param_tweak.c | 4 +--- include/tbl/params.h | 3 +++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index 82a1e192adc..6ec97c01add 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -677,9 +677,7 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, { unsigned j; - if (arg != NULL && !strcmp(arg, "default") && - strcmp(par->def, "none")) { - bit_clear(p, l); + if (arg != NULL && !strcmp(arg, "default")) { return (tweak_generic_bits(vsb, par, par->def, p, l, tags, desc, sign)); } diff --git a/include/tbl/params.h b/include/tbl/params.h index de16b146ea7..36923172af4 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1829,6 +1829,7 @@ PARAM_BITS( /* name */ feature, /* fld */ feature_bits, /* def */ + "none," "+validate_headers," "+vcl_req_reset", /* descr */ @@ -1847,6 +1848,7 @@ PARAM_BITS( /* name */ vcc_feature, /* fld */ vcc_feature_bits, /* def */ + "none," "+err_unref," "+unsafe_path", /* descr */ @@ -1865,6 +1867,7 @@ PARAM_BITS( /* name */ vsl_mask, /* fld */ vsl_mask, /* def */ + "all," "-Debug," "-ExpKill," "-H2RxBody," From 61c7cc469144d2909ec45e3af7650559ef858d4f Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 09:16:56 +0100 Subject: [PATCH 7/8] cli: Teach param.reset -j to output param.show --- bin/varnishd/mgt/mgt_param.c | 2 +- bin/varnishtest/tests/c00054.vtc | 1 + include/tbl/cli_cmds.h | 6 ++++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param.c b/bin/varnishd/mgt/mgt_param.c index 1cfd789ba36..d305a100698 100644 --- a/bin/varnishd/mgt/mgt_param.c +++ b/bin/varnishd/mgt/mgt_param.c @@ -713,7 +713,7 @@ mcf_wash_param(struct cli *cli, struct parspec *pp, enum mcf_which_e which, static struct cli_proto cli_params[] = { { CLICMD_PARAM_SHOW, "", mcf_param_show, mcf_param_show_json }, { CLICMD_PARAM_SET, "", mcf_param_set, mcf_param_set_json }, - { CLICMD_PARAM_RESET, "", mcf_param_reset }, + { CLICMD_PARAM_RESET, "", mcf_param_reset, mcf_param_set_json }, { NULL } }; diff --git a/bin/varnishtest/tests/c00054.vtc b/bin/varnishtest/tests/c00054.vtc index 63aef00a15d..0ee1c2674fc 100644 --- a/bin/varnishtest/tests/c00054.vtc +++ b/bin/varnishtest/tests/c00054.vtc @@ -10,6 +10,7 @@ varnish v1 -cliok "param.show vsl_mask" varnish v1 -cliexpect {"value": "none"} "param.set -j feature none" varnish v1 -cliexpect {"value": "all"} "param.set -j vsl_mask all" +varnish v1 -cliexpect {"value": "all(,-\w+)+"} "param.reset -j vsl_mask" varnish v1 -clierr 106 "param.set vsl_mask FooBar" varnish v1 -clierr 106 "param.set vsl_mask -FooBar" diff --git a/include/tbl/cli_cmds.h b/include/tbl/cli_cmds.h index b4b9c3194db..3295b5844ee 100644 --- a/include/tbl/cli_cmds.h +++ b/include/tbl/cli_cmds.h @@ -172,9 +172,11 @@ CLI_CMD(VCL_LABEL, CLI_CMD(PARAM_RESET, "param.reset", - "param.reset ", + "param.reset [-j] ", "Reset parameter to default value.", - "", + " The JSON output is the same as ``param.show -j `` and" + " contains the updated value as it would be represented by a" + " subsequent execution of ``param.show``.\n\n", 1,1 ) From cb6a58484346b243186f97dd9281bd5c7370506e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 7 Nov 2023 09:21:24 +0100 Subject: [PATCH 8/8] param: Deprecate default for bits parameters We have had the ability to reset any parameter to its default value for a while now. --- bin/varnishd/mgt/mgt_param_tweak.c | 1 + include/tbl/params.h | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c index 6ec97c01add..07ad6ce303a 100644 --- a/bin/varnishd/mgt/mgt_param_tweak.c +++ b/bin/varnishd/mgt/mgt_param_tweak.c @@ -678,6 +678,7 @@ tweak_generic_bits(struct vsb *vsb, const struct parspec *par, const char *arg, unsigned j; if (arg != NULL && !strcmp(arg, "default")) { + /* XXX: deprecated in favor of param.reset */ return (tweak_generic_bits(vsb, par, par->def, p, l, tags, desc, sign)); } diff --git a/include/tbl/params.h b/include/tbl/params.h index 36923172af4..c88809da709 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1834,7 +1834,7 @@ PARAM_BITS( "+vcl_req_reset", /* descr */ "Enable/Disable various minor features.\n" - "\tdefault\tSet default value\n" + "\tdefault\tSet default value (deprecated: use param.reset)\n" "\tnone\tDisable all features.\n\n" "Use +/- prefix to enable/disable individual feature:") #ifdef PARAM_ALL @@ -1853,7 +1853,7 @@ PARAM_BITS( "+unsafe_path", /* descr */ "Enable/Disable various VCC behaviors.\n" - "\tdefault\tSet default value\n" + "\tdefault\tSet default value (deprecated: use param.reset)\n" "\tnone\tDisable all behaviors.\n\n" "Use +/- prefix to enable/disable individual behavior:") #ifdef PARAM_ALL @@ -1885,7 +1885,7 @@ PARAM_BITS( /* descr */ "Mask individual VSL messages from being logged.\n" "\tall\tEnable all tags\n" - "\tdefault\tSet default value\n" + "\tdefault\tSet default value (deprecated: use param.reset)\n" "\nUse +/- prefix in front of VSL tag name to unmask/mask " "individual VSL messages.") PARAM_POST