[libvirt] [PATCH] Allow blkdeviotune to take human readable values
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
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,
On Tue, May 3, 2016 at 8:00 PM, Peter Krempa <pkrempa@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
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@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
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
On Tue, May 3, 2016 at 11:17 PM, Cole Robinson <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:
I was going too, but wasn't sure if that would be allowed in a commit message, will do in the next patches. Nishith
On 05/03/2016 01:52 PM, Nishith Shah wrote:
On Tue, May 3, 2016 at 11:17 PM, Cole Robinson <crobinso@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
participants (4)
-
Cole Robinson -
Ján Tomko -
Nishith Shah -
Peter Krempa