[libvirt] [PATCH 02/13] improve the iteration of VSH_OT_ARGV options

[Lai Jiangshan]

On 05/26/2011 12:38 AM, Daniel P. Berrange wrote:
> On Wed, May 25, 2011 at 05:37:44PM +0800, Lai Jiangshan wrote:
>> Signed-off-by: Lai Jiangshan <laijs at fujitsu.com>
>> ---
>>  tools/virsh.c |   47 ++++++++++++++++++++++++-----------------------
>>  1 files changed, 24 insertions(+), 23 deletions(-)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index c358580..2e27535 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -277,7 +277,27 @@ static int vshCommandOptULongLong(const vshCmd *cmd, const char *name,
>>                                    unsigned long long *value)
>>      ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
>>  static bool vshCommandOptBool(const vshCmd *cmd, const char *name);
>> -static char *vshCommandOptArgv(const vshCmd *cmd, int count);
>> +
>> +/*
>> + * Iterate all the argv arguments.
>> + *
>> + * Requires that a VSH_OT_ARGV option  be last in the
>> + * list of supported options in CMD->def->opts.
>> + */
>> +static inline const vshCmdOpt *__variable_arg(const vshCmdOpt *opt)
>> +{
>> +    while (opt) {
>> +         if (opt->def && opt->def->type == VSH_OT_ARGV)
>> +             break;
>> +         opt = opt->next;
>> +    }
>> +
>> +    return opt;
>> +}
>> +
>> +#define for_each_variable_arg(cmd, opt)                                       \
>> +    for (opt = __variable_arg(cmd->opts); opt; opt = __variable_arg(opt->next))
>> +
>>  
>>  #define VSH_BYID     (1 << 1)
>>  #define VSH_BYUUID   (1 << 2)
>> @@ -10059,6 +10079,7 @@ cmdEcho (vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd)
>>      bool shell = false;
>>      bool xml = false;
>>      int count = 0;
>> +    const vshCmdOpt *opt;
>>      char *arg;
>>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>>  
>> @@ -10067,10 +10088,11 @@ cmdEcho (vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd)
>>      if (vshCommandOptBool(cmd, "xml"))
>>          xml = true;
>>  
>> -    while ((arg = vshCommandOptArgv(cmd, count)) != NULL) {
>> +    for_each_variable_arg(cmd, opt) {
>>          bool close_quote = false;
>>          char *q;
>>  
>> +        arg = opt->data;
>>          if (count)
>>              virBufferAddChar(&buf, ' ');
>>          /* Add outer '' only if arg included shell metacharacters.  */
>> @@ -11484,27 +11506,6 @@ vshCommandOptBool(const vshCmd *cmd, const char *name)
>>      return vshCommandOpt(cmd, name) != NULL;
>>  }
>>  
>> -/*
>> - * Returns the COUNT argv argument, or NULL after last argument.
>> - *
>> - * Requires that a VSH_OT_ARGV option with the name "" be last in the
>> - * list of supported options in CMD->def->opts.
>> - */
>> -static char *
>> -vshCommandOptArgv(const vshCmd *cmd, int count)
>> -{
>> -    vshCmdOpt *opt = cmd->opts;
>> -
>> -    while (opt) {
>> -        if (opt->def && opt->def->type == VSH_OT_ARGV) {
>> -            if (count-- == 0)
>> -                return opt->data;
>> -        }
>> -        opt = opt->next;
>> -    }
>> -    return NULL;
>> -}
>> -
>>  /* Determine whether CMD->opts includes an option with name OPTNAME.
>>     If not, give a diagnostic and return false.
>>     If so, return true.  */
> 
> I'm not entirely sure I understand what the effect of this patch
> is. Can you explain what the change in semantics for the parser
> is with this patch applied
> 

"for_each_XXXX" macro is more friendly/directly, it very common in linux kernel.
"while ((arg = vshCommandOptArgv(cmd, count)) != NULL)" is O(N*N) time.
"while ((arg = vshCommandOptArgv(cmd, count)) != NULL)" requires a "count" to control the iteration.

Thanks,
Lai