Hello Willy, On Wed, Apr 18, 2018 at 07:38:24AM +0200, Willy Tarreau wrote: > 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). >
The problem is: the main parser (cli_parse_request()) does not have any idea of what is an argument and how many there should be so it can't tell if the command syntax is valid or not before passing it to the commands parser. What I did is allow the user to use << where he wants and then the commands parsers will do their jobs and if they need more data they will look for them in what is called 'payload'. I guess your point was to use << as a strict delimiter of arguments and payload. > > 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. Ok, I can remove that part. > > 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 Yes, that is what I did but I also allowed the << pattern to be loosely placed at the user conveniance. > 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) Sure. -- Aurélien.
