Control command refactor v2 #940
Closed
Conversation
Use a table-based lookup to find the right command handler. This will serve as the basement for several future improvements, as we improve the API for parsing commands.
There _is_ an underlying logic to these commands, but it isn't wholly uniform, given years of tweaks and changes. Fortunately I think there is a superset that will work. This commit adds a parser for some of the most basic cases -- the ones currently handled by getargs_helper() and some of the object-taking ones. Soon will come initial tests; then I'll start using the parser. After that, I'll expand the parser to handle the other cases that come up in the controller protocol.
This is preliminary work for fixing 29984; no behavior has changed.
The first line break in particular was mishandled: it was discarded if no arguments came before it, which made it impossible to distinguish arguments from the first line of the body. To solve this, we need to allocate a copy of the command rather than using NUL to separate it, since we might have "COMMAND\n" as our input. Fixes ticket 29984.
Pull Request Test Coverage Report for Build 4844
|
(This should be all of the command that work nicely with positional arguments only.) Some of these commands should probably treat extra arguments as incorrect, but for now I'm trying to be careful not to break any existing users.
The two options are mutually exclusive, since otherwise an entry like "Foo" would be ambiguous. We want to have the ability to treat entries like this as keys, though, since some controller commands interpret them as flags.
This should let us handle all (or nearly all) of the remaining commands.
Here we get to throw away a LOT of unused code, since most of the old parsing was redundant with kvline.
This command does not fit perfectly with the others, since its second argument is optional and may contain equal signs. Still, it's probably better to squeeze it into the new metaformat, since doing so allows us to remove several pieces of the old command-parsing machinery.
This function decodes something different from the usual c-escaped format. It is only used in controller authorization.
These two presented their own challenge, because of their use of QString, and their distinguished handling of quoted versus non-quoted values.
Now that the legacy handlers are gone, we can simplify the structures and macros here.
tlyu
reviewed
Apr 15, 2019
Minor comment suggestions inline. Also, this maybe needs a changes file for ticket 29984?
tlyu
reviewed
Apr 17, 2019
Mostly minor issues, plus a couple of technical debt items.
…inebreaks Fix grammar in documentation for control_split_incoming_command().
…inebreaks Add a changes file for bug29984.
…inebreaks Explain the logic in only skipping horizontal space.
This makes it match control-spec.txt.
[[NOTE: When squashing, replace "alues" with "values" in summary.]]
tlyu
reviewed
Apr 24, 2019
Minor typo and tech debt comment
src/feature/control/control_cmd.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| if (syntax->allowed_keywords) { |
This is a little better now that it's in a helper function. I still think this is a great place for an early return. (and it makes even more sense because there's no need for a goto.
I think in this case I'd like to keep it as-is -- I anticipate that there are likely to be more kinds of check in the future, such as required keywords.
I prefer to take the extra nesting out now, and add it back when we need it due to future complexity. It's not a strong preference in this case.
|
Now squashed as #980 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
No description provided.
The text was updated successfully, but these errors were encountered: