-
Notifications
You must be signed in to change notification settings - Fork 367
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
param: Optionally pass a duration unit to timeouts #3766
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not looked at the details of the code yet, but 👍🏽 on the interface change
@@ -67,29 +69,29 @@ parse_decimal(const char *p, const char **err) | |||
|
|||
static int | |||
tweak_generic_double(struct vsb *vsb, const char *arg, const struct parspec *pp, | |||
const char *fmt) | |||
parse_double_f parse, const char *fmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have made the parse argument default to parse_decimal if NULL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a default for a static function with 3 call sites?
bin/varnishd/mgt/mgt_param_tweak.c
Outdated
pt.priv = &px.max_age; | ||
pt.min = "0"; | ||
pt.max = "1000000"; | ||
retval = tweak_generic_double(vsb, av[3], &pt, "%.0f"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is pretty much exactly why I made those arguments explicit to tweak_generic_double
in the first place.
I'm not a fan of this ephemeral parspec, but I can live with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a u-turn here because the function signature was requiring more effort than the temporary parspec.
This is a first step towards even more generalization.
By themselves, structured fields parsing functions don't fail on extra input. To avoid repeating the same checks twice, they are wrapped into a function that will help generalize tweak_generic_double() at least once more.
Allowing units is much more convenient and keeping them optional maintains compatibility. On the other hand, changing the defaults for default_ttl and default_grace has no impact on the generated RST. Keeping track of the user input could help since default values are "washed" like user input, without resorting to a dynamic default. Refs varnishcache#3756
Rebased against master to solve the merge conflict after having pushed e2220de directly. |
OK with me. |
Allowing units is much more convenient and keeping them optional
maintains compatibility. On the other hand, changing the defaults
for default_ttl and default_grace has no impact on the generated
RST.
Keeping track of the user input could help since default values
are "washed" like user input, without resorting to a dynamic
default.
Refs #3756