Varnishadm: Add -e argument #4018

walid-git · 2023-11-08T10:55:53Z

-e
Exit immediately if a command fails in pass mode and return the CLI
status code of the failing command. This has no effect on interactive
mode (except when -p is used). Similarly to CLI Command File (see
:ref:varnishd(1)), if a command is prefixed with '-', its failure will
be ignored and will not cause varnishadm to exit.

Fixes: #4012

walid-git · 2023-11-08T14:17:14Z

Bugwash:

[15:09:38] <phk> I dont see any problems
[15:09:49] <phk> but I guess we should give @nigoroll a chance

doc/sphinx/reference/varnishadm.rst

bin/varnishadm/varnishadm.c

bin/varnishtest/tests/u00001.vtc

lib/libvarnish/vcli_serve.c

bin/varnishadm/varnishadm.c

dridi

Since we are in the neighborhood...

dridi · 2023-11-10T09:47:35Z

bin/varnishadm/varnishadm.c

+	for (i = 1; av[i] != NULL; i++) {
+		VSB_quote(cli_buf, av[i], strlen(av[i]), 0);
+		VSB_putc(cli_buf, ' ');
+	}


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.

nigoroll · 2023-11-13T14:28:09Z

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?

dridi · 2023-11-13T14:40:19Z

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.

walid-git · 2023-11-13T16:29:51Z

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.

walid-git · 2023-11-16T09:24:58Z

Changed the return value of varnishadm -e to cli status / 100 after phk's suggestion during bugwash. That is because CLI status can go beyond 255 and we might have a status code that is N*256 that will return 0.
Added polling to the remote cli socket so that we are notified if the socket hangs up when we are not writing anything.
Added quoting to the CLI command arguments so that the VCLS server we are forwarding commands to sees the arguments grouped in the same way they were received.

Note that tests will not pass until the bugfix in f354ca9 is merged.

nigoroll

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...

lib/libvarnish/vsb.c

bin/varnishadm/varnishadm.c

dridi · 2023-12-05T10:49:52Z

doc/sphinx/reference/varnishadm.rst

+-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


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.

dridi · 2023-12-05T10:57:32Z

bin/varnishadm/varnishadm.c

+		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);
+		}


-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)?

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

dridi · 2023-12-05T11:00:33Z

include/vsb.h

+#define VSB_QUOTE_CLI		64
+	/*
+	 * Add quotes ".." around arguments when needed
+	 */


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.

walid-git · 2023-12-28T09:03:26Z

Addressed dridi's comments:

Harmonized the exit status and documented them in EXIT STATUS section of the man page.
We don't forward the parsed heredoc now, we reconstruct it and forward it to be parsed in mgt instead.

Ready for review

dridi

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.

dridi · 2024-01-03T13:45:58Z

bin/varnishadm/varnishadm.c

-			printf("%s\n", answer);
-		if (status == CLIS_TRUNCATED)
-			printf("[response was truncated]\n");
+	if (cli) {


Explicit null check please.

dridi · 2024-01-03T13:48:17Z

bin/varnishadm/varnishadm.c

+	pass_print_answer(cli->result, VSB_data(cli->sb), pass_script);
+
+}


Remove blank line after pass_print_answer() call.

dridi · 2024-01-03T13:53:44Z

bin/varnishadm/varnishadm.c

 vadm_cli_cb_after(const struct cli *cli)
 {

+
+	const char *cmd;


Remove blank lines before cmd declaration.

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.

Fixes: varnishcache#4012

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.

walid-git force-pushed the varnishadm_fix branch from 6936de2 to eec728a Compare November 8, 2023 13:04

dridi requested changes Nov 8, 2023

View reviewed changes

dridi reviewed Nov 8, 2023

View reviewed changes

lib/libvarnish/vcli_serve.c Outdated Show resolved Hide resolved

bin/varnishadm/varnishadm.c Outdated Show resolved Hide resolved

walid-git force-pushed the varnishadm_fix branch from eec728a to 0639c81 Compare November 9, 2023 12:03

dridi reviewed Nov 10, 2023

View reviewed changes

bin/varnishadm/varnishadm.c Outdated Show resolved Hide resolved

dridi reviewed Nov 10, 2023

View reviewed changes

walid-git force-pushed the varnishadm_fix branch from 0639c81 to d931856 Compare November 13, 2023 16:25

walid-git force-pushed the varnishadm_fix branch from d931856 to aaa97ae Compare November 16, 2023 09:18

walid-git force-pushed the varnishadm_fix branch from aaa97ae to e32b768 Compare November 20, 2023 14:16

walid-git force-pushed the varnishadm_fix branch 3 times, most recently from 18982c2 to 0a26519 Compare December 1, 2023 10:00

nigoroll approved these changes Dec 4, 2023

View reviewed changes

lib/libvarnish/vsb.c Outdated Show resolved Hide resolved

walid-git force-pushed the varnishadm_fix branch from 0a26519 to c00de41 Compare December 5, 2023 10:18

dridi requested changes Dec 5, 2023

View reviewed changes

walid-git force-pushed the varnishadm_fix branch 2 times, most recently from 5bab39e to cb825bc Compare December 27, 2023 18:33

dridi requested changes Jan 5, 2024

View reviewed changes

walid-git force-pushed the varnishadm_fix branch 2 times, most recently from 7622e10 to d4835cf Compare January 23, 2024 13:49

walid-git added 4 commits January 23, 2024 14:51

vcls: Make VCLS output format configurable

4626b30

varnishadm: Use a VCLP in pass mode

f37669d

walid-git added 7 commits January 23, 2024 14:57

varnishadm: split pass_answer()

ad31065

varnishadm: Add -e argument

6ee0842

Fixes: varnishcache#4012

VSB_quote: Add VSB_QUOTE_CLI

9c035d2

Put arguments between quotes ".." when needed

cli: use VSB_QUOTE_CLI to quote arguments

4070e2a

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.

varnishadm: Add coverage for heredoc

d54dac4

varnishadm: Reconstruct heredoc before forwarding

5959c4b

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.

varnishadm: Harmonize exit status

7355499

walid-git force-pushed the varnishadm_fix branch from d4835cf to 7355499 Compare January 23, 2024 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Varnishadm: Add -e argument #4018

Varnishadm: Add -e argument #4018

walid-git commented Nov 8, 2023

walid-git commented Nov 8, 2023

dridi left a comment

dridi Nov 10, 2023

nigoroll commented Nov 13, 2023

dridi commented Nov 13, 2023

walid-git commented Nov 13, 2023

walid-git commented Nov 16, 2023 •

edited

nigoroll left a comment

dridi Dec 5, 2023

dridi Dec 5, 2023

walid-git Dec 7, 2023

dridi Dec 5, 2023

walid-git commented Dec 28, 2023

dridi left a comment

dridi Jan 3, 2024

dridi Jan 3, 2024

dridi Jan 3, 2024

		pass_print_answer(cli->result, VSB_data(cli->sb), pass_script);

		}

Varnishadm: Add -e argument #4018

Are you sure you want to change the base?

Varnishadm: Add -e argument #4018

Conversation

walid-git commented Nov 8, 2023

walid-git commented Nov 8, 2023

dridi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nigoroll commented Nov 13, 2023

dridi commented Nov 13, 2023

walid-git commented Nov 13, 2023

walid-git commented Nov 16, 2023 • edited

nigoroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walid-git commented Dec 28, 2023

dridi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walid-git commented Nov 16, 2023 •

edited