[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/4] Allow multiple parameters for schedinfo



On 03/14/2013 09:53 PM, Eric Blake wrote:
> On 03/14/2013 03:27 AM, Martin Kletzander wrote:
>> virsh schedinfo was able to set only one parameter at a time (not
>> counting the deprecated options), but it is useful to set more at
>> once, so this patch adds the possibility to do stuff like this:
>>
>> virsh schedinfo <domain> cpu_shares=0 vcpu_period=0 vcpu_quota=0 \
>> emulator_period=0 emulator_quota=0
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=919372
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=919375
>>
>> Signed-off-by: Martin Kletzander <mkletzan redhat com>
>> ---
>>  tools/virsh-domain.c | 55 ++++++++++++++++++++++++++--------------------------
>>  1 file changed, 27 insertions(+), 28 deletions(-)
> 
> No change to tools/virsh.pod?  I would have expected something like:
> 
> =item B<schedinfo> I<domain> [[I<--config>] [I<--live>] | [I<--current>]]
> [I<--set> B<parameter=value>]...
> 
>> +++ b/tools/virsh-domain.c
>> @@ -4026,36 +4026,33 @@ static const vshCmdOptDef opts_schedinfo[] = {
>>       .flags = VSH_OFLAG_REQ,
>>       .help = N_("domain name, id or uuid")
>>      },
>> -    {.name = "set",
>> -     .type = VSH_OT_STRING,
>> -     .flags = VSH_OFLAG_NONE,
>> -     .help = N_("parameter=value")
>> -    },
>>      {.name = "weight",
>>       .type = VSH_OT_INT,
>> -     .flags = VSH_OFLAG_NONE,
>> +     .flags = VSH_OFLAG_REQ_OPT,
>>       .help = N_("weight for XEN_CREDIT")
> 
> Previously, 'schedinfo domain 1' was parsed as --set=1, but then errored
> out because there was no '=' in the argument to set; a user doing weight
> in isolation had to do an explicit --weight=1 to skip the --set field.
> Now that you have re-ordered parameters, but used VSH_OFLAG_REQ_OPT on
> all parameters that got moved before set, a single argument still parses
> as --set, and the user still has to do an explicit --weight=1 to use the
> weight option instead.  That's good - no semantic change for the
> single-argument case.
> 
> For the multi-argument case: previously, 'schedinfo domain foo=bar 1'
> was parsed as --set=foo=bar --weight=1, now it will parse as
> --set=foo=bar --set=1 and error out.  But I don't think that anyone was
> relying on mixing old and new syntax (the man page called out --weight
> on a different line than --set), so I can live with that change.
> 
> Thus, even though I see a difference in parse, that difference is only
> on a case that users should not have been doing, and I'm happy with your
> patch.
> 
> ACK, if you touch up virsh.pod to match.
> 

Thanks for that, but I've found out one more inconsistency which is
bothering me a bit (even though it was present there even before this
patch), so I'll be sending a v2 for this one.  This time with the manual
fixed as well.

Martin


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]