Re: [PATCH 1/3] virsh-host: fix pagesize unit of freepages

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 22.09.2014 12:14, Jincheng Miao wrote:
> The unit of '--pagesize' of freepages is kibibytes.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1145048
>
> Signed-off-by: Jincheng Miao <jmiao@xxxxxxxxxx>
> ---
>   tools/virsh-host.c | 27 ++++++++++++++++++---------
>   1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index 7fc2120..6dcf77e 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -293,7 +293,7 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd)
>       bool ret = false;
>       unsigned int npages;
>       unsigned int *pagesize = NULL;
> -    unsigned long long tmp = 0;
> +    unsigned int tmp = 0;
>       int cell;
>       unsigned long long *counts = NULL;
>       size_t i, j;
> @@ -304,9 +304,20 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd)
>       xmlXPathContextPtr ctxt = NULL;
>       bool all = vshCommandOptBool(cmd, "all");
>       bool cellno = vshCommandOptBool(cmd, "cellno");
> +    bool pagesz = vshCommandOptBool(cmd, "pagesize");
>
>       VSH_EXCLUSIVE_OPTIONS_VAR(all, cellno);
>
> +    if (vshCommandOptUInt(cmd, "pagesize", &tmp) < 0) {
> +        vshError(ctl, "%s", _("Invalid pagesize argument"));
> +        goto cleanup;
> +    }
> +

No, previously we accepted something like '--pagesize 2M'. This undo 
this feature.

> +    if ((pagesz || cellno) && tmp < 1) {
> +        vshError(ctl, "%s", _("page size must be at least 1KiB"));
> +        goto cleanup;
> +    }
> +
>       if (all) {
>           if (!(cap_xml = virConnectGetCapabilities(ctl->conn))) {
>               vshError(ctl, "%s", _("unable to get node capabilities"));
> @@ -363,6 +374,8 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd)
>
>               vshPrint(ctl, _("Node %d:\n"), cell);
>               for (j = 0; j < npages; j++) {
> +                if (pagesz && tmp != pagesize[j])
> +                    continue;

Instead of this, I'd honor --pagesize when creating @pagesize array to 
call viNodeGetFreePages() a few lines above.

>                   vshPrint(ctl, "%uKiB: %lld\n", pagesize[j], counts[j]);
>               }
>               vshPrint(ctl, "%c", '\n');
> @@ -380,20 +393,16 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd)
>           }
>
>           if (cell < -1) {
> -            vshError(ctl, "%s", _("cell number must be non-negative integer or -1"));
> +            vshError(ctl, "%s",
> +                     _("cell number must be non-negative integer or -1"));
>               goto cleanup;
>           }
>
> -        if (vshCommandOptScaledInt(cmd, "pagesize", &tmp, 1, UINT_MAX) < 0) {

In fact this is the bit that's buggy here. It should have been 1024 
instead of 1.

> -            vshError(ctl, "%s", _("page size has to be a number"));
> -            goto cleanup;
> -        }
>           /* page size is expected in kibibytes */
>           pagesize = vshMalloc(ctl, sizeof(*pagesize));
> -        *pagesize = tmp / 1024;
>
> -        if (!pagesize[0]) {
> -            vshError(ctl, "%s", _("page size must be at least 1KiB"));
> +        if (vshCommandOptUInt(cmd, "pagesize", pagesize) < 0) {
> +            vshError(ctl, "%s", _("Invalid cellno argument"));
>               goto cleanup;
>           }

Michal

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