On 04/01/2014 07:34 PM, Solly Ross wrote:
I finally took time to look at this patch again. Sorry for the extreme
delay. Overall, this patch looks decent; I'll probably touch up the
findings I make below and apply it if the rest of the series is okay.
> This commit extracts the parsing logic from vshCommandParse
> so that it can be used by other methods. The vshCommandParse
> 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 vshCommandParse. Depending on what happens, instead of erroring
> or continuing the loop, it returns a value from an enum indicating the
> current state. It also modifies several arguments as appropriate.
>
> Then, a caller can choose to deal with the state, or may simply ignore
> the state when convenient (vshCommandParse deals with the state,
> so as to continue providing its previous functionality, while a
> completer could choose to ignore states involving unknown options,
> for example).
> ---
> tools/virsh.c | 338 +++++++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 228 insertions(+), 110 deletions(-)
>
> + VSH_LINE_STATE_CMD_DONE,
> + VSH_LINE_STATE_LINE_DONE,
> +} vshLineExtractionState;
> +
> +static vshLineExtractionState
These days, we usually have two lines between functions.
> +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;
Static variables are 0-initialized by default; they also make this
non-thread-safe (although I'm not sure that it matters, since our parser
is single-threaded). If you made them static in order to preserve
values between successive callers, that makes me wonder if they should
have been parameters instead.
> + vshCommandToken tok_type;
> + char *tok = NULL;
> + vshLineExtractionState ret = VSH_LINE_STATE_IN_PROGRESS;
> +
> + if (!state) {
> + data_only = false;
> + opts_required = 0;
> + }
> +
> + *opt = NULL;
> +
> + tok_type = parser->getNextArg(ctl, parser, &tok, raise_err);
> +
> + if (tok_type == VSH_TK_ERROR) {
> + ret = VSH_LINE_STATE_TOK_ERR;
> + *opt = NULL;
Redundant assignment.
> + goto cleanup;
> + } else if (tok_type == VSH_TK_END) {
> + ret = VSH_LINE_STATE_LINE_DONE;
> + *opt = NULL;
and again
> + goto cleanup;
> + } else if (tok_type == VSH_TK_SUBCMD_END) {
> + *opt = NULL;
and again
> + //*cmd = NULL;
Why is this commented out?
> + ret = VSH_LINE_STATE_CMD_DONE;
> + goto cleanup;
> + }
> +
> + if (*cmd == NULL) {
> + *cmd = vshCmddefSearch(tok);
> + if (!*cmd) {
> + ret = VSH_LINE_STATE_UNKNOWN_CMD;
> + *tok_out = vshStrdup(ctl, tok);
> + goto cleanup;
> + }
> +
> + if (vshCmddefOptParse(*cmd, opts_need_arg, &opts_required) < 0)
> + ret = VSH_LINE_STATE_BAD_OPTS;
> + else
> + ret = VSH_LINE_STATE_IN_PROGRESS;
> + } else if (data_only) {
> + goto get_data;
> + } else if (tok[0] == '-' && tok[1] == '-' &&
> + c_isalnum(tok[2])) {
> + char *optstr = strchr(tok + 2, '=');
> + int opt_index = 0;
> +
> + if (optstr) {
> + *optstr = '\0'; /* convert the '=' to '\0' */
> + optstr = vshStrdup(ctl, optstr + 1);
> + }
> + /* Special case 'help' to ignore all spurious options */
This comment appears like it no longer applies.
> + *opt = vshCmddefGetOption(ctl, *cmd, tok + 2,
> + opts_seen, &opt_index,
> + &optstr, raise_err);
> + if (!*opt) {
> + VIR_FREE(optstr);
> + *tok_out = vshStrdup(ctl, tok);
> + ret = VSH_LINE_STATE_UNKNOWN_OPT;
> + goto cleanup;
> + }
> +
> + VIR_FREE(tok);
> +
> + if ((*opt)->type != VSH_OT_BOOL) {
> + if (optstr)
> + tok = optstr;
> + else
> + tok_type = parser->getNextArg(ctl, parser, &tok, raise_err);
> +
> + if (tok_type == VSH_TK_ERROR) {
> + ret = VSH_LINE_STATE_TOK_ERR;
> + *tok_out = vshStrdup(ctl, tok);
> + } else if (tok_type == VSH_TK_END) {
> + ret = VSH_LINE_STATE_LINE_DONE;
> + } else if (tok_type == VSH_TK_SUBCMD_END) {
> + ret = VSH_LINE_STATE_CMD_DONE;
> + } else {
> + ret = VSH_LINE_STATE_IN_PROGRESS;
> + *tok_out = vshStrdup(ctl, tok);
> + }
> +
> + if ((*opt)->type != VSH_OT_ARGV)
> + *opts_need_arg &= ~(1 << opt_index);
> + } else {
> + if (optstr) {
> + ret = VSH_LINE_STATE_INVALID_EQUALS;
> + VIR_FREE(optstr);
> + } else {
> + ret = VSH_LINE_STATE_IN_PROGRESS;
> + }
> + }
> + } else if (tok[0] == '-' && tok[1] == '-' && !tok[2]) {
> + ret = VSH_LINE_STATE_DATA_ONLY;
> + data_only = true;
> + } else {
> + get_data:
> + *opt = vshCmddefGetData(*cmd, opts_need_arg, opts_seen);
> + if (!*opt)
> + ret = VSH_LINE_STATE_UNEXPECTED_DATA;
> + else
> + ret = VSH_LINE_STATE_IN_PROGRESS;
> +
> + *tok_out = vshStrdup(ctl, tok);
> + }
> +
> + cleanup:
> + VIR_FREE(tok);
> + return ret;
> +}
> +
> +
> static bool
> vshCommandParse(vshControl *ctl, vshCommandParser *parser)
> {
> char *tkdata = NULL;
> vshCmd *clast = NULL;
> vshCmdOpt *first = NULL;
> + int state = 0;
>
> if (ctl->cmd) {
> vshCommandFree(ctl->cmd);
> @@ -1906,11 +2044,10 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
> while (1) {
> vshCmdOpt *last = NULL;
> const vshCmdDef *cmd = NULL;
> - vshCommandToken tk;
> - bool data_only = false;
> uint32_t opts_need_arg = 0;
> uint32_t opts_required = 0;
> uint32_t opts_seen = 0;
> + vshLineExtractionState line_state;
>
> first = NULL;
>
> @@ -1918,112 +2055,85 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
> const vshCmdOptDef *opt = NULL;
>
> tkdata = NULL;
> - tk = parser->getNextArg(ctl, parser, &tkdata);
>
> - if (tk == VSH_TK_ERROR)
> - goto syntaxError;
> - if (tk != VSH_TK_ARG) {
> - VIR_FREE(tkdata);
> - break;
> - }
> + line_state = vshExtractLinePart(ctl, parser, &opts_seen,
> + &opts_need_arg, &tkdata,
> + &cmd, &opt, true, state);
> + state++;
I'm not quite sure what values state should have. Instead of feeding
back an int each time, would it be better to pass an enum value from the
previous call, where the initial call is the value that indicates start
of line? Then you don't need a static variable in the helper function,
but instead use the fact that the previous indication returned
VSH_LINE_STATE_DATA_ONLY as the state argument for the next iteration to
look for a data argument, for example.
>
> - if (cmd == NULL) {
> - /* first token must be command name */
> - if (!(cmd = vshCmddefSearch(tkdata))) {
> - vshError(ctl, _("unknown command: '%s'"), tkdata);
> - goto syntaxError; /* ... or ignore this command only? */
> + if (line_state == VSH_LINE_STATE_TOK_ERR) {
> + if (opt) { /* we got some funky syntax in an option */
> + vshError(ctl,
> + _("expected syntax: --%s <%s>"),
> + opt->name,
> + opt->type ==
> + VSH_OT_INT ? _("number") : _("string"));
> }
> - if (vshCmddefOptParse(cmd, &opts_need_arg,
> - &opts_required) < 0) {
> + /* otherwise, we're here because the tokenizer couldn't
> + * even start reading */
> + goto syntaxError;
> + } else if (line_state == VSH_LINE_STATE_LINE_DONE ||
> + line_state == VSH_LINE_STATE_CMD_DONE) {
Indentation is off.
> + if (!opt) { /* we're actually at the end of the line */
> + VIR_FREE(tkdata);
> + break;
> + } else { /* we have an flag without an arg */
s/an flag/a flag/
> vshError(ctl,
> - _("internal error: bad options in command: '%s'"),
> - tkdata);
> + _("expected syntax: --%s <%s>"),
> + opt->name,
> + opt->type ==
> + VSH_OT_INT ? _("number") : _("string"));
> goto syntaxError;
> }
> - VIR_FREE(tkdata);
> - } else if (data_only) {
> - goto get_data;
> - } else if (tkdata[0] == '-' && tkdata[1] == '-' &&
> - c_isalnum(tkdata[2])) {
> - char *optstr = strchr(tkdata + 2, '=');
> - int opt_index = 0;
> -
> - if (optstr) {
> - *optstr = '\0'; /* convert the '=' to '\0' */
> - optstr = vshStrdup(ctl, optstr + 1);
> - }
> - /* Special case 'help' to ignore all spurious options */
> - if (!(opt = vshCmddefGetOption(ctl, cmd, tkdata + 2,
> - &opts_seen, &opt_index,
> - &optstr))) {
> - VIR_FREE(optstr);
> - if (STREQ(cmd->name, "help"))
> - continue;
> + } else if (line_state == VSH_LINE_STATE_UNEXPECTED_DATA) {
> + if (STRNEQ(cmd->name, "help")) {
> + /* anything's find after help, but
s/find/fine/
> + * otherwise we got some unexpected
> + * positional arguments */
> + vshError(ctl, _("unexpected data '%s'"), tkdata);
> goto syntaxError;
...
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list