Hi Aurélien, On Wed, Apr 18, 2018 at 09:36:14AM +0200, Aurélien Nephtali wrote: > > My worries come from your example above which splits the "set server" > > command and some of you comments which seem to imply that the arguments > > are looked up in the payload part. > > Well, they are looked up in the payload part but only if the command > parser is missing its mandadory ones. > Let's use the "set server" keywords as an example. This command does not > usually takes a payload but it's what I wanted to add: "multi-lines for > everyone". But maybe the real point should be to add "payload for some" ? > Here would be valid ways of using "set server": > > # set server << > bk/fe state ready > # set server bk/fe state ready > # set server bk/fe << > state > ready > # set server << > bk/fe > state > ready > > The parser will first call cli_resync_args(). > What that will do is consume the payload in order to reach > its number of mandatory arguments. Internally the list of arguments will > become: 'set' 'server 'bk/fe' 'state' 'ready' and the payload will be > set to NULL. Then the parser will request tokenization (through > cli_split_args()) and do its job.
I really don't like to mix this. It makes error control very hard, and will make it even harder to later add optional arguments to some commands, because they could optionally be looked up in the payload if the previous command used to support one. If we want to make some commands support getting one of its current arguments as payload (such as ocsp), let's do it explicitly for the command. For example it could be said that "set ssl ocsp-response" supports either one argument on the command line, or a payload. But making commands concatenate arguments and start of payload seems very wrong to me because we're mixing data of different semantics. Look at this totally made up example below. Let's say we implement support for payload to fill in ACLs. The following command is used to enter a list of new keywords that you want to detect in a query string as potentially dangerous actions on a login page : > add acl dangerous_words << + delete + clear + kill + close + remove + Then later someone proposes a patch for the ACLs to perform an atomic load after clearing existing ones, and suggests to append the optional keyword "clear" on the command line arguments. The command becomes : > add acl foo clear << + blah + foo + bar + zoo + As you can see this has no apparent effect on the first command above. But now the script used above to load the dangerous words sends them sorted in alphabetical order for whatever reason (eg: deduplication) : > add acl dangerous_words << + clear + delete + kill + close + remove + And suddenly this "clear" word appearing first in the payload is mistakenly used as an extension of the command line and silently destroys previous contents before loading the 4 remaining words. > There is no "real" split between arguments and payload. It will be > command parsers that will decide what is the final payload according to > what they need. And this is exactly what prevents commands from being extended in the future : you know what you used to support as arguments because it was in the code, but you don't know what users used to feed via the payload so you have no way to avoid causing conflicts. I could make an analogy with HTTP headers vs payload. Each one has its own role and you never want to look up a cookie or accept-encoding in the payload of a POST for example. But at the same time it doesn't prevent specific applications from specifically implementing support for picking the information they need from the payload. It just doesn't happen this way in general. > > Please keep in mind the example I proposed regarding > > "add map" with HTTP status codes and reasons, to be sure that once your > > change is applied, it finally becomes possible to add "404 Not Found" as > > a payload where only the "add map" command decides how to parse that line > > (ie: decide that 404 is the key and the rest is the value, which implies > > that > > it's *not* tokenized). If this works easily, I'm quite confident we'll be on > > a good track. > > > > Well, it already does that :) : > > $ cat tst3 | socat /tmp/sock1 - > > $ socat /tmp/sock1 - > show map #-1 > 0x7e6ad0 test.com bk > 0x7f5260 200 OK > 0x7f52a0 203 Non-Authoritative Information > 0x82e3b0 204 No Content > 0x82e4b0 301 Moved Permanently > 0x82e5b0 302 Found > 0x82e6b0 303 See Other > 0x82e7b0 400 Bad Request > > $ cat tst3 > add map << > #-1 > 100 Continue > 200 OK > 203 Non-Authoritative Information > 204 No Content > 301 Moved Permanently > 302 Found > 303 See Other > 400 Bad Request > > $ Do you realize that as a *user* I don't understand how to safely and reliably use this ? It makes me feel like I just throw a random amount of words to the command and that it will figure by itself which ones are arguments and which ones are data to be loaded :-/ I can possibly understand why there may be a benefit to support argument-as-payload for *some* commands, but these ones will also be the ones which will need to be improved to support batched actions. Your "set server" example could be one of these. Right now (unless I'm mistaken) we can set a single setting per command on "set server". We could extend the command to support a list of settings at once, but it would quickly become a mess and be limited by the number of arguments. Alternatively we could have a "set server bk/sv at-once <<" command via which we load one setting per *line* of payload (ie \n is really used as a setting delimitor there), all of them for the same server. Then the horrors below : > # set server << > bk/fe state ready > # set server bk/fe state ready > # set server bk/fe << > state > ready > # set server << > bk/fe > state > ready Could become : # set server bk/fe at-once << + state ready + weight 100% + agent-send "GET / HTTP/1.0\r\n\r\n" + fqdn server3 + addr 10.0.1.2 + port 3000 + Then the command is processed atomically for all these settings. And as you see, if you'd tokenize this as words it would become the same mess as having them on the command line. Hoping this helps, Willy
