Simplify key=val argument handling #4272
Merged
+52
−49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dridi
requested changes
Feb 13, 2025
bin/varnishd/mgt/mgt_jail_unix.c
Outdated
| if (vju_getuid((*args) + l_user)) | ||
| val = keyval(*args, "user="); | ||
| if (val != NULL) { | ||
| if (vju_getuid(val)) | ||
| ARGV_ERR( | ||
| "Unix jail: %s user not found.\n", | ||
| (*args) + 5); |
bin/varnishd/mgt/mgt_jail_unix.c
Outdated
| if (vju_getwrkuid((*args) + l_workuser)) | ||
| val = keyval(*args, "workuser="); | ||
| if (val != NULL) { | ||
| if (vju_getwrkuid(val)) | ||
| ARGV_ERR( | ||
| "Unix jail: %s user not found.\n", | ||
| (*args) + 9); |
There was a problem hiding this comment.
Not ready to replace #4202 until you get rid of (*args) + ... occurrences.
bin/varnishd/mgt/mgt_jail_unix.c
Outdated
| if (vju_getccgid((*args) + l_ccgroup)) | ||
| val = keyval(*args, "ccgroup="); | ||
| if (val != NULL) { | ||
| if (vju_getccgid(val)) | ||
| ARGV_ERR( | ||
| "Unix jail: %s group not found.\n", | ||
| (*args) + 8); |
There was a problem hiding this comment.
Not ready to replace #4202 until you get rid of (*args) + ... occurrences.
we add a simple utility function to return the pointer to the thing after "key=" if the first argument begins with "key=". Note that this is no worse than a macro with a "reasonable good" compiler *) and default (-O2) optimization, because the function call gets inlined and the strlen() turned into a constant. 0x00000000000a1bd1 <+49>: lea 0x6ed49(%rip),%r14 # 0x110921 ... 0x00000000000a1c01 <+97>: mov $0x5,%edx 0x00000000000a1c06 <+102>: mov %rbx,%rdi 0x00000000000a1c09 <+105>: mov %r14,%rsi 0x00000000000a1c0c <+108>: call 0x2d360 <strncmp@plt> 0x00000000000a1c11 <+113>: lea 0x5(%rbx),%rbp 0x00000000000a1c15 <+117>: test %eax,%eax (gdb) p (const char *)0x110921 $2 = 0x110921 "user=" Replaces varnishcache#4202 Polishes 4001ea2 *) tested: gcc version 12.2.0 (Debian 12.2.0-14) Debian clang version 14.0.6
nigoroll
force-pushed
the
jail_keyval
branch
from
a76d832 to
45cdc60
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@dridi I would use this in place of #4202. Do you agree?
This is a Draft because I still want to look if we can replace other similar cases and probably move the helper function to a better place as a
static inline1we add a simple utility function to return the pointer to the thing after "key=" if the first argument begins with "key=".
Note that this is no worse than a macro with a "reasonable good" compiler *) and default (-O2) optimization, because the function call gets inlined and the strlen() turned into a constant.
Replaces #4202
Polishes 4001ea2
*) tested:
gcc version 12.2.0 (Debian 12.2.0-14)
Debian clang version 14.0.6
Footnotes
Reminder that
inlinehas no other relevance than "do not complain if unused", as the compiler is free to inline no matter what ↩