[libvirt] [PATCH 1/5] Improve virsh autocompletion (extract parser)

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140329/18e4714f/attachment-0001.sig>