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

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(a)redhat.com&gt; > --- > 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.