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 <mklet...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
