9:09 a.m.
Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there is
no need to pass values in raw bytes, which is seldom the case.
---
tools/virsh-domain.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0a6caae..3843675 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1299,7 +1299,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptStringReq(ctl, cmd, "device", &disk) < 0)
goto cleanup;
- if ((rv = vshCommandOptULongLong(ctl, cmd, "total-bytes-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-bytes-sec", &value,
1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1308,7 +1308,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "read-bytes-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "read-bytes-sec", &value, 1,
ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1317,7 +1317,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "write-bytes-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "write-bytes-sec", &value,
1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1326,7 +1326,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "total-bytes-sec-max",
&value)) < 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-bytes-sec-max",
&value, 1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1335,7 +1335,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "read-bytes-sec-max",
&value)) < 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "read-bytes-sec-max",
&value, 1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1344,7 +1344,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "write-bytes-sec-max",
&value)) < 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "write-bytes-sec-max",
&value, 1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1353,7 +1353,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-iops-sec", &value, 1,
ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1362,7 +1362,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "read-iops-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "read-iops-sec", &value, 1,
ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1371,7 +1371,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "write-iops-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "write-iops-sec", &value, 1,
ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1380,7 +1380,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "write-iops-sec-max",
&value)) < 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "write-iops-sec-max",
&value, 1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1389,7 +1389,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "read-iops-sec-max",
&value)) < 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "read-iops-sec-max", &value,
1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1398,7 +1398,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec-max",
&value)) < 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-iops-sec-max",
&value, 1, ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
@@ -1407,7 +1407,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "size-iops-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "size-iops-sec", &value, 1,
ULLONG_MAX)) < 0) {
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
--
2.1.4
9:30 a.m.
On Tue, May 03, 2016 at 14:09:42 +0000, Nishith Shah wrote:
Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there
is
no need to pass values in raw bytes, which is seldom the case.
A change like this is incomplete without adding documentation to the
virsh man page (tools/virsh.pod) and in the help strings that document
the individual arguments in the source file.
---
tools/virsh-domain.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0a6caae..3843675 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
[...]
@@ -1353,7 +1353,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd
*cmd)
goto save_error;
}
- if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec", &value))
< 0) {
+ if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-iops-sec", &value,
1, ULLONG_MAX)) < 0) {
iops is a count rather than a byte size so i don't think the default
scaling function is a good fit since:
kiB -> 1024
KB -> 1000
k -> 1024
goto interror;
} else if (rv > 0) {
if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams,
9:48 a.m.
On Tue, May 3, 2016 at 8:00 PM, Peter Krempa <pkrempa(a)redhat.com> wrote:
On Tue, May 03, 2016 at 14:09:42 +0000, Nishith Shah wrote:
> Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there is
> no need to pass values in raw bytes, which is seldom the case.
A change like this is incomplete without adding documentation to the
virsh man page (tools/virsh.pod) and in the help strings that document
the individual arguments in the source file.
Sorry about that, this is my first patch and so I am still figuring things
out. I will add the details in virsh.pod. Can you elaborate a bit on the
help strings in this context?
> ---
> tools/virsh-domain.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 0a6caae..3843675 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
[...]
> @@ -1353,7 +1353,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
> goto save_error;
> }
>
> - if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec",
&value)) < 0) {
> + if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-iops-sec",
&value, 1, ULLONG_MAX)) < 0) {
iops is a count rather than a byte size so i don't think the default
scaling function is a good fit since:
kiB -> 1024
KB -> 1000
k -> 1024
True, I hadn't thought of that. Should we use a scaling function at all
here, since the usual values of iops[1] dont really need to be given in a
short format like 1K or 1M?
[1]: https://en.wikipedia.org/wiki/IOPS#Examples
Regards,
Nishith
10:02 a.m.
On Tue, May 03, 2016 at 08:18:41PM +0530, Nishith Shah wrote:
On Tue, May 3, 2016 at 8:00 PM, Peter Krempa
<pkrempa(a)redhat.com> wrote:
> On Tue, May 03, 2016 at 14:09:42 +0000, Nishith Shah wrote:
> > Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there is
> > no need to pass values in raw bytes, which is seldom the case.
>
> A change like this is incomplete without adding documentation to the
> virsh man page (tools/virsh.pod) and in the help strings that document
> the individual arguments in the source file.
>
Sorry about that, this is my first patch and so I am still figuring things
out.
Welcome!
I will add the details in virsh.pod. Can you elaborate a bit on the
help strings in this context?
The opts_blkdeviotune[] array contains structs with all the options
available for the command.
The strings contained in .help are then printed by 'virsh blkdeviotune --help'.
Jan
12:47 p.m.
On 05/03/2016 10:09 AM, Nishith Shah wrote:
Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there
is
no need to pass values in raw bytes, which is seldom the case.
Also, on v2, you can add this bug link to the commit message:
https://bugzilla.redhat.com/show_bug.cgi?id=885380
Thanks,
Cole
12:52 p.m.
On Tue, May 3, 2016 at 11:17 PM, Cole Robinson <crobinso(a)redhat.com> wrote:
On 05/03/2016 10:09 AM, Nishith Shah wrote:
> Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there is
> no need to pass values in raw bytes, which is seldom the case.
Also, on v2, you can add this bug link to the commit message:
https://bugzilla.redhat.com/show_bug.cgi?id=885380
I was going too, but wasn't sure if that would be allowed in a commit
message, will do in the next patches.
Nishith
12:55 p.m.
On 05/03/2016 01:52 PM, Nishith Shah wrote:
On Tue, May 3, 2016 at 11:17 PM, Cole Robinson
<crobinso(a)redhat.com
<mailto:crobinso@redhat.com>> wrote:
On 05/03/2016 10:09 AM, Nishith Shah wrote:
> Use vshCommandOptScaledInt instead of vshCommandOptULongLong so there is
> no need to pass values in raw bytes, which is seldom the case.
Also, on v2, you can add this bug link to the commit message:
https://bugzilla.redhat.com/show_bug.cgi?id=885380
I was going too, but wasn't sure if that would be allowed in a commit message,
will do in the next patches.
If in doubt, check out other examples from 'git log' and follow the
conventions you see. For example I suggest a subject line like:
virsh: blkdeviotune: accept human readable values
At least the intro 'virsh:' bit is pretty standard
Thanks,
Cole
3161
days inactive
3161
days old
6 comments
4 participants
participants (4)
-
Cole Robinson -
Ján Tomko -
Nishith Shah -
Peter Krempa