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

[lhuang]

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)) {
+        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;
      }



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.

Luyao

> John
>> +            goto cleanup;
>> +        }
>> +
>>           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;
>>   
>>