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