Hi Aurélien,
On Tue, Apr 17, 2018 at 09:06:35PM +0200, Aurélien Nephtali wrote:
> Example:
> If this command is entered:
> set server bk/fe <<
> state ready
>
> Three arguments and a payload will be tagged. This command requires (in
> some cases) seven arguments (including the keywords) so a call to
> cli_resync_args(ci, 7) will extract what can be found from the payload.
> Once a parser has done everything it can to ensure it has access to its
> arguments, it can call cli_split_args() to do the proper tokenization
> and modify the input string to insert null bytes.
I'm confused, I thought we agreed *not* to mix arguments and payload, like
in the map example I provided, but now you seem to imply that extra args
are looked up in the payload part, while here you seem to have stuck to
this design, and your "add map" example seems to do exactly the case which
makes it useless (ie no way to enter spaces in the value because of the
undesired tokenizing).
> For now three commands was modified to take advantage of the multi-line
> mode: "set ssl ocsp-response" and "add map"/"add acl".
>
> The number of arguments has been increased to 8192 (from 64) and the
> array to hold the pointers gets its memory from a pool. I chose 8192
> since it's the maximum number of arguments possible with an input buffer
> of 16384 (default tune.bufsize) (16384 / 2 if all arguments are one-char
> words). Would computing the max value at runtime be better ?
That's exactly what scares me in fact. For me there's no reason to increase
the argument count, the parser should parse a command's argument and decide
that what follows '<<' is a raw payload (we can debate whether it starts on
the next line or immediately after '<<' or if it's only '<<\n' which starts
a payload but for not it's a detail). The command should then get the pointer
to that payload as a payload pointer and take care of its payload itself.
Then we can indeed improve ocsp-response, map/acl to support taking entries
either as arguments or as payload or both. In fact I think that there is no
need for doing something very different from what we see in shell already :
the command's args are argv[0..N] and the payload is in stdin. Then many
commands decide where to pick their data from
Also, think about it, if you start to look up arguments in the payload during
the parsing, you will never be able to extend the system to support payloads
larger than one buffer. For example one day someone may want to be able to
preload the cache with known objects loaded from another instance (eg: on
reload). We could then have :
> add cache object example.com/foo/bar <<
+ [base64 contents spanning over multiple buffers]
+
Above for me the arguments are only on the first line. The rest is made
of bytes. It's up to the command's payload parser to know that the \n\n
sequence will mark the end of the body.
Similarly in shell you can do :
$ grep foo /tmp/blah
$ grep -f- /tmp/blah <<EOF
> foo
> EOF
Please just let me know if I missed anything.
By the way, please better split the patch in several parts :
- one for the infrastructure changes (CLI parser in general and its
repercussions all over the code)
- one per modified subsystem to benefit from it (eg: adding payload
support to ocsp should be one patch, adding payload support to map/acl
should be another one)
This makes it much easier for all involved parties to review the changes
in their respective areas. And when a regression is reported and bisect
ends up on such a patch, it's much easier to spot the affected area (eg:
you prefer to know that add map was broken by the patch adding payload
support to add map than that it was broken by the one introducing the
multiline support, possibly requiring to revert it for all commands at
once to work around the issue instead of just for this one).
Thanks,
Willy
