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

[lhuang]

On 10/02/2015 11:49 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(+)
>>
> Perhaps the more entertaining part of this patch is/was it possible to
> use "freepages 0 0"?  Before patch 1, before patch 2, after patch 2.

Patch 1 + 2 can improve the error when use "freepages 0 0"

> Based on the answer to the question below, I can adjust the commit
> message (and code if necessary) a bit in order to remove the really long
> line and make it flow better.

Thanks a lot for your help, i am not sure about if we need rework this 
code or add a patch 3 to improve the error message, please help to check 
the reply i will give in another mail.

>> 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"));
>> +            goto cleanup;
>> +        }
>> +
> Why is there no "if (node != -1 && !virNumaNodeIsAvailable(node)) {"
> check prior to the virNumaGetHugePageInfoPath call?
>
> <same for next change too>

Hmm... i think this check is enough since the output error is "page size 
or NUMA node not available", the output message have already include the 
case which pass a invalid NUMA node.

I thought that add a check like "if (node != -1 && 
!virNumaNodeIsAvailable(node)) {" just output a different error in this 
place, however this (invalid nodeset) will be caught more early in 
another place (in nodeGetFreePages function):

     if (startCell > lastCell) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("start cell %d out of range (0-%d)"),
                        startCell, lastCell);
         goto cleanup;
     }


I noticed another reply from you to this mail, and think we can make the 
error more clearly as your said in another reply. I will continue to 
reply in another mail.

Thanks a lot for your review and help !

> John
>

Luyao

>>           if (virFileReadAll(path, 1024, &buf) < 0)
>>               goto cleanup;
>>   
>> @@ -579,6 +585,12 @@ virNumaGetHugePageInfo(int node,
>>                                          page_size, "free_hugepages") < 0)
>>               goto cleanup;
>>   
>> +        if (!virFileExists(path)) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                           _("page size or NUMA node not available"));
>> +            goto cleanup;
>> +        }
>> +
>>           if (virFileReadAll(path, 1024, &buf) < 0)
>>               goto cleanup;
>>   
>>