-
Notifications
You must be signed in to change notification settings - Fork 381
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
Varnishadm: Add -e argument #4018
base: master
Are you sure you want to change the base?
Conversation
6936de2
to
eec728a
Compare
Bugwash:
|
eec728a
to
0639c81
Compare
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.
Since we are in the neighborhood...
bin/varnishadm/varnishadm.c
Outdated
for (i = 1; av[i] != NULL; i++) { | ||
VSB_quote(cli_buf, av[i], strlen(av[i]), 0); | ||
VSB_putc(cli_buf, ' '); | ||
} |
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 think now is a good time to tackle the lack of SP quoting and share the logic with the do_args()
function.
This could mean a new VSB_QUOTE_CLI
flag. I'll let you think about our options here. One could be to always write double-quoted arguments with such a flag, except of course when we have a here-document.
I am not sure if what I am asking for justifies the effort, but I am having a hard time following the first commit. If it is not too much work, could it be split into smaller commits or could the commit message explain better what it does? |
Not sure how the github web UI would render it, but if the discarded comment was preserved as I suggested we would likely have a cleaner diff. |
0639c81
to
d931856
Compare
I brought back the comment mentioned by Dridi, hopefully this will make the diff a bit more readable. Otherwise, I don't see any way to break the commit into smaller ones. |
d931856
to
aaa97ae
Compare
Note that tests will not pass until the bugfix in f354ca9 is merged. |
aaa97ae
to
e32b768
Compare
18982c2
to
0a26519
Compare
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.
Thank you for the improved diff. I think I understand the patch better now, but still I am not that experienced in this area. Looks OK to my eyes...
0a26519
to
c00de41
Compare
doc/sphinx/reference/varnishadm.rst
Outdated
-e | ||
Exit immediately if a command fails in `pass` mode and return the CLI | ||
status code of the failing command divided by 100. This has no effect |
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 think we should point to the existing section in the manual and detail things there:
Exit immediately if a command fails in
pass
mode, see EXIT STATUS for more details.
We can even make EXIT STATUS
a reference for the HTML rendering with sphinx.
Then in that section we don't document it as "divided by 100" but rather as a list of status codes with their meanings:
- 1: could not execute the command
- 3: the command failed
- 4: the connection was lost
- 5: the connection was closed
Help to better word this would be appreciated.
This should probably be done first in argv
mode, at the beginning of this patch series.
Not sure about the interactive mode.
bin/varnishadm/varnishadm.c
Outdated
i = VCLS_Poll(vcls, cli, 0); | ||
j = poll(fds, 1, 0); | ||
if (j == 1 && recv(sock, buf, 1, MSG_PEEK) == 0) { | ||
fprintf(stderr, "CLI communication error\n"); | ||
RL_EXIT(1); | ||
} |
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.
-RL_EXIT(1);
+adm_exit(CLIS_COMMS);
Where the adm_exit()
function does the division by 100.
Also, I'm curious, why peek instead of checking the result for poll(2)
?
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 used peek, because, on my system at least, poll would report POLLIN event when the remote socket is closed, and that could not be differentiated from when we have actual data to read (which should never be the case but still..), so I found using recv with MSG_PEEK to be the most reliable solution here as it will always return 0 on EOF
#define VSB_QUOTE_CLI 64 | ||
/* | ||
* Add quotes ".." around arguments when needed | ||
*/ |
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 think the rule for "when needed" is the presence of either space, tabulation, or double quote in the argument value, so in its current state VSB_QUOTE_CLI
is insufficient.
5bab39e
to
cb825bc
Compare
Addressed dridi's comments:
Ready for review |
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.
We looked at some of the issues brought up in this pull request with @walid-git and we are considering adding small building blocks to vcli_serve
to offer a "CLI proxy" feature. @walid-git is working on it, and meanwhile I forgot to submit the small nitpicks we found while reviewing the commits.
bin/varnishadm/varnishadm.c
Outdated
printf("%s\n", answer); | ||
if (status == CLIS_TRUNCATED) | ||
printf("[response was truncated]\n"); | ||
if (cli) { |
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.
Explicit null check please.
bin/varnishadm/varnishadm.c
Outdated
pass_print_answer(cli->result, VSB_data(cli->sb), pass_script); | ||
|
||
} |
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.
Remove blank line after pass_print_answer()
call.
bin/varnishadm/varnishadm.c
Outdated
vadm_cli_cb_after(const struct cli *cli) | ||
{ | ||
|
||
|
||
const char *cmd; |
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.
Remove blank lines before cmd declaration.
7622e10
to
d4835cf
Compare
VCLP is a VCLS used as a proxy, it accepts connections similary to a VCLS and has a single wildcard command that forwards the input to another remote VCLS. VCLP_poll is different from VCLS_poll in that it also polls the target VCLS socket and returns an error in case the connection is broken.
From POLL(2) man page's POLLHUP section: Note that when reading from a channel such as a pipe or a stream socket, this event merely indicates that the peer closed its end of the channel. Subsequent reads from the channel will return 0 (end of file) only after all outstanding data in the channel has been consumed. This means that receiving a POLLHUP event should not prevent us from trying a read since data might still be available to consume. Thus, we can simply ignore POLLHUP events and rely entirely on read return value.
Put arguments between quotes ".." when needed
This will make sure that the VCLS server that we are forwarding commands to will see the commands in the same way they were received.
Let mgt parse the heredocs as received instead of forwarding the parsed version. This keeps the previous logging behvior where only the first line of the heredoc command gets logged to syslog instead of the whole thing.
d4835cf
to
7355499
Compare
Fixes: #4012