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

[lhuang]

On 10/09/2015 10:27 PM, John Ferlan wrote:
>
> 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/ ;-)

Thanks pointing out that, i wrote them too fast, so you know... :)

>
>> +        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...

Yes, this is expected, since if node == -1 we will check the 
/sys/kernel/mm/hugepages/* path, if we get failed, then means the 
hugepage size is not valid.

> 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?

I guess you want to say "if node != -1" ? yes, report error like "NUMA 
node 1 is not available for page size 2048" will be more clearly in the 
case which node is exist but the page_size is not.

>> +        }
>>           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.

Okay, i will post a new version these days.

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

Thanks a lot for your help, so i just leave the old commit messages when 
write a new version ?

I think you (and people who help me to improve the commit) nearly become 
my English teacher :D

> John

Luyao