[libvirt] [PATCH 2/2] util: Produce friendlier error message to user

[John Ferlan]

On 10/08/2015 05:12 AM, lhuang wrote:
> 
> 
> On 10/02/2015 11:54 PM, John Ferlan wrote:
>>
>> On 09/30/2015 12:01 AM, Luyao Huang wrote:
>>> Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
>>> this patch just like it and fix the issue in another function.
>>>
>>> when user use freepages and specify a invalid node or page size,
>>> will get error like this:
>>>
>>>   # virsh freepages 0 1
>>>   error: Failed to open file
>>> '/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages':
>>> No such file or directory
>>>
>>> Add two checks to catch this and therefore produce much more
>>> friendlier error messages.
>>>
>>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>>> ---
>>>   src/util/virnuma.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
>>> index 8577bd8..c8beade 100644
>>> --- a/src/util/virnuma.c
>>> +++ b/src/util/virnuma.c
>>> @@ -560,6 +560,12 @@ virNumaGetHugePageInfo(int node,
>>>                                          page_size, "nr_hugepages") < 0)
>>>               goto cleanup;
>>>   +        if (!virFileExists(path)) {
>>> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>> +                           _("page size or NUMA node not available"));
>> duh - meant to add:
>>
>> as perhaps a "patch 2" of the series (making this patch 3) - would it be
>> perhaps better to indicate "page size of "%u" or NUMA node "%d" not
>> available", using page_size, node as the arguments?
>>
>> ??
> 
> Good idea, and how about make the code like this:
> 
> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
> index cb80972..8d85dc3 100644
> --- a/src/util/virnuma.c
> +++ b/src/util/virnuma.c
> @@ -553,6 +553,19 @@ virNumaGetHugePageInfo(int node,
>                                         page_size, "nr_hugepages") < 0)
>              goto cleanup;
> 
> +        if (!virFileExists(path)) {
> +            if (node != -1 && !virNumaNodeIsAvailable(node)) {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                               _("NUMA node %d is not available"),
> +                               node);
> +            } else {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                               _("page size %u is not available"),
> +                               page_size);
> +            }
> +            goto cleanup;
> +        }
> +
>          if (virFileReadAll(path, 1024, &buf) < 0)
>              goto cleanup;
> 
> @@ -572,6 +585,19 @@ virNumaGetHugePageInfo(int node,
>                                         page_size, "free_hugepages") < 0)
>              goto cleanup;
> 
> +        if (!virFileExists(path)) {
> +            if (node != -1 && !virNumaNodeIsAvailable(node)) {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                               _("NUMA node %d is not available"),
> +                               node);
> +            } else {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                               _("page size %u is not available"),
> +                               page_size);
> +            }
> +            goto cleanup;
> +        }
> +
>          if (virFileReadAll(path, 1024, &buf) < 0)
>              goto cleanup;
> 
> @@ -836,19 +862,19 @@ virNumaSetPagePoolSize(int node,
>          goto cleanup;
>      }
> 
> -    if (node != -1 && !virNumaNodeIsAvailable(node)) {
> -        virReportError(VIR_ERR_OPERATION_FAILED,
> -                       _("NUMA node %d is not available"),
> -                       node);
> -        goto cleanup;
> -    }
> -
>      if (virNumaGetHugePageInfoPath(&nr_path, node, page_size,
> "nr_hugepages") < 0)
>          goto cleanup;
> 
> -    if (!virFileExists(nr_path)) {
> -        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> -                       _("page size or NUMA node not available"));
> +    if (!virFileExists(path)) {

s/path/nr_path/ ;-)

> +        if (node != -1 && !virNumaNodeIsAvailable(node)) {
> +            virReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("NUMA node %d is not available"),
> +                           node);
> +        } else {
> +            virReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("page size %u is not available"),
> +                           page_size);

This part of the message could be displayed when node == -1 in the event
that !virNumaNodeIsAvailable is false. I think that's right based on
what virNumaGetHugePageInfoPath does, but I just want to make sure that
is what you'd expect...

Conversely if page_size != -1, then would it make sense to add the
page_size or not?  That is, sure page_size 'x' is not available, but
would it make sense to say "NUMA node 1 is not available for page size
2048"?  IDC either way, but what if NUMA node 1 does exist, but the
specified page size doesn't?

> +        }
>          goto cleanup;
>      }
> 
> 
> 
> We need check if node == -1 to avoid output error include "NUMA node
> "-1" ", and instead of check the nodeset again and again, i think just
> check them when we cannot find the file (!virFileExists(nr_path)),
> check the return from virNumaNodeIsAvailable() to output different error.
> 
> Thanks in advance for your reply and help.
> 

Let's have you create a v2... Insert the "virNumaSetPagePoolSize" change
as patch #2 with the adjusted code path and error message. The commit
message should mention commit id '1c24cfe9' and that you're looking to
make the error message/path cleare.

They the current patch 2 becomes patch 3 with the similar code or
whatever else you've considered doing.

I can adjust commit messages for syntax/english...

John