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

2013-03-21 Thread Martin Kletzander 
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 Bschedinfo Idomain [[I--config] [I--live] | [I--current]]
 [I--set Bparameter=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

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

2013-03-14 Thread Martin Kletzander 
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(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ab90f58..a7cc9d5 100644
--- a/tools/virsh-domain.c
+++ 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)
 },
 {.name = cap,
  .type = VSH_OT_INT,
- .flags = VSH_OFLAG_NONE,
+ .flags = VSH_OFLAG_REQ_OPT,
  .help = N_(cap for XEN_CREDIT)
 },
 {.name = current,
  .type = VSH_OT_BOOL,
- .flags = 0,
  .help = N_(get/set current scheduler info)
 },
 {.name = config,
  .type = VSH_OT_BOOL,
- .flags = 0,
  .help = N_(get/set value to be used on next boot)
 },
 {.name = live,
  .type = VSH_OT_BOOL,
- .flags = 0,
  .help = N_(get/set value from running domain)
 },
+{.name = set,
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_NONE,
+ .help = N_(parameter=value)
+},
 {.name = NULL}
 };

@@ -4064,9 +4061,9 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
virTypedParameterPtr src_params, int nsrc_params,
virTypedParameterPtr *update_params)
 {
-const char *set_arg;
 char *set_field = NULL;
 char *set_val = NULL;
+const vshCmdOpt *opt = NULL;
 virTypedParameterPtr param;
 virTypedParameterPtr params = NULL;
 int nparams = 0;
@@ -4076,17 +4073,6 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
 int val;
 int i;

-if (vshCommandOptString(cmd, set, set_arg)  0) {
-set_field = vshStrdup(ctl, set_arg);
-if (!(set_val = strchr(set_field, '='))) {
-vshError(ctl, %s, _(Invalid syntax for --set, expecting 
name=value));
-goto cleanup;
-}
-
-*set_val = '\0';
-set_val++;
-}
-
 for (i = 0; i  nsrc_params; i++) {
 param = (src_params[i]);

@@ -4108,15 +4094,28 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
 continue;
 }

-if (set_field  STREQ(set_field, param-field)) {
-if (virTypedParamsAddFromString(params, nparams, maxparams,
-set_field, param-type,
-set_val)  0) {
-vshSaveLibvirtError();
+opt = NULL;
+while ((opt = vshCommandOptArgv(cmd, opt))) {
+set_field = vshStrdup(ctl, opt-data);
+if (!(set_val = strchr(set_field, '='))) {
+vshError(ctl, %s, _(Invalid syntax for --set, expecting 
name=value));
 goto cleanup;
 }

-continue;
+*set_val = '\0';
+set_val++;
+
+if (STREQ(set_field, param-field)) {
+if (virTypedParamsAddFromString(params, nparams, maxparams,
+set_field, param-type,
+set_val)  0) {
+vshSaveLibvirtError();
+goto cleanup;
+}
+VIR_FREE(set_field);
+break;
+}
+VIR_FREE(set_field);
 }
 }

-- 
1.8.1.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-03-14 Thread Eric Blake 
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 Bschedinfo Idomain [[I--config] [I--live] | [I--current]]
[I--set Bparameter=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.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list