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

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(a)redhat.com&gt; >> --- >> 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.