10:09 p.m.
On 03/29/2014 08:36 PM, Solly Ross wrote:
This commit extracts the parsing logic from vshCommnadParse
s/Commnad/Command/
so that it can be used by other methods. The vshCommnadParse
and again
method is designed to parse full commands and error out on
unknown information, so it is not suitable to simply use it
for autocompletion. Instead, the logic has been extracted.
The new method essentially performs one pass of the loop previously
done by vshCommnadParse. Depending on what happens, instead of erroring
and again
or continuing the loop, it returns a value from an enum indicating
the
current state. It also modifieds several arguments as appropriate.
s/modifieds/modifies/
Then, a caller can choose to deal with the state, or may simply ignore
the state when convinient (vshCommandParse deals with the state,
s/convinient/convenient/
so as to continue providing its previous functionality, while a
completer could choose to ingore states involving unknown options,
s/ingore/ignore/
for example).
---
outgoing/0000-cover-letter.patch | 68 +
...prove-virsh-autocompletion-extract-parser.patch | 428 ++++++
...prove-virsh-autocompletion-base-framework.patch | 278 ++++
...ve-virsh-autocompletion-global-ctl-object.patch | 116 ++
...ove-virsh-autocompletion-domain-completer.patch | 1479 ++++++++++++++++++++
...virsh-autocompletion-enum-completer-macro.patch | 131 ++
Ouch - these 6 files mistakenly got committed into your tree. They
should not be part of this commit. I'll push a patch to .gitignore to
overlook .patch files, but you'll need to amend your patch to drop these
additions. Easiest might be to use 'git rebase -i', mark this patch as
'edit', then exit the editor to start the efforts. From the shell, I
would then do 'git reset HEAD^' then 'git add tools/virsh.c' then
'git
rebase --continue' - it will preserve your commit message, but should
show just one file instead of 7 in the amended commit.
It's late for me today, so for now, this review is just high-level (I'm
not closely reading the code, just looking for obvious violations of
coding standards).
diff --git a/tools/virsh.c b/tools/virsh.c
index f2e4c4a..0273abe 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1110,7 +1110,7 @@ static vshCmdOptDef helpopt = {
};
static const vshCmdOptDef *
vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name,
- uint32_t *opts_seen, int *opt_index, char **optstr)
+ uint32_t *opts_seen, int *opt_index, char **optstr, bool raise_err)
Please wrap this line to stay within 80 columns.
+static vshLineExtractionState
+vshExtractLinePart(vshControl *ctl, vshCommandParser *parser,
+ uint32_t *opts_seen, uint32_t *opts_need_arg,
+ char **tok_out, const vshCmdDef **cmd,
+ const vshCmdOptDef **opt, bool raise_err, int state)
+{
+ static bool data_only = false;
+ static uint32_t opts_required = 0;
Explicit initialization of static variables to 0 is overkill; the C
standard already guarantees this. Are these variables really necessary
to be saved across invocations of this function?
+ } else if (tok_type == VSH_TK_SUBCMD_END) {
+ *opt = NULL;
+ //*cmd = NULL;
No // comments, please.
static bool
vshCommandParse(vshControl *ctl, vshCommandParser *parser)
{
char *tkdata = NULL;
vshCmd *clast = NULL;
vshCmdOpt *first = NULL;
+ int state = 0;
Might as well type this with the appropriate enum type, instead of int.
- /* Special case 'help' to ignore spurious
data */
- if (!(opt = vshCmddefGetData(cmd, &opts_need_arg,
- &opts_seen)) &&
- STRNEQ(cmd->name, "help")) {
- vshError(ctl, _("unexpected data '%s'"), tkdata);
- goto syntaxError;
+
+ if (!first)
+ first = arg;
+ if (last)
+ last->next = arg;
+ last = arg;
+
+ vshDebug(ctl, VSH_ERR_INFO, "%s: %s(%s): %s\n",
+ cmd->name,
+ opt->name,
+ opt->type != VSH_OT_BOOL ? _("optdata") :
_("bool"),
+ opt->type != VSH_OT_BOOL ? arg->data :
_("(none)"));
}
- }
- if (opt) {
- /* save option */
- vshCmdOpt *arg = vshMalloc(ctl, sizeof(vshCmdOpt));
-
- arg->def = opt;
- arg->data = tkdata;
- arg->next = NULL;
- tkdata = NULL;
-
- if (!first)
- first = arg;
- if (last)
- last->next = arg;
- last = arg;
-
- vshDebug(ctl, VSH_ERR_INFO, "%s: %s(%s): %s\n",
Looks like a lot of reindentation. I find it easier to split patches
like this into two parts - one that is just new content but wrong
indentation, and the other that is just whitespace changes.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org