On 09/30/2015 12:01 AM, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1265114

When pass 0 page_size to virNumaGetHugePageInfoPath function, we will get fail like this:

error : virFileReadAll:1358 : Failed to read file '/sys/devices/system/node/node0/hugepages/': Is a directory

Because when the page_size is 0 the virNumaGetHugePageInfoPath will build the directory of system path, but we don't want that.

Introduce a new helper to build the dir path could avoid this issue.

This appears to be just a straight refactor - it took me a while to figure out why the key was 'page_size == 0'. Looking at "just" this context it would seem virNumaGetPages using 0 && suffix==NULL was the only user/cause for this. But the real abuser/issue shows up in patch 2 where if "other" callers use a page_size == 0, then we have issues. So, how about the following for a commit message: Refactor virNumaGetHugePageInfoPath to handle the passing a page_size of 0 and suffix == NULL into a new function virNumaGetHugePageInfoDir. Function virNumaGetPages expects to return a directory; whereas, other callers that passed a page_size == 0 would not expect a directory path in return. Each of those callers will use virFileReadAll which fails if a directory path is returned as follows: error : virFileReadAll:1358 : Failed to read file '/sys/devices/system/node/node0/hugepages/': Is a directory

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/util/virnuma.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 1a62d62..8577bd8 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -493,45 +493,39 @@ virNumaGetHugePageInfoPath(char **path, unsigned int page_size, const char *suffix) { - - int ret = -1; - if (node == -1) { /* We are aiming at overall system info */ - if (page_size) { - /* And even on specific huge page size */ if (virAsprintf(path, HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX "%ukB/%s", page_size, suffix ? suffix : "") < 0)

Now this has too many spaces shifted to the right (4 to be exact)

- goto cleanup; - } else { - if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0) - goto cleanup; - } - + return -1; } else { /* We are aiming on specific NUMA node */ - if (page_size) { - /* And even on specific huge page size */ if (virAsprintf(path, HUGEPAGES_NUMA_PREFIX "node%d/hugepages/" HUGEPAGES_PREFIX "%ukB/%s", node, page_size, suffix ? suffix : "") < 0)

same note here

- goto cleanup; - } else { - if (virAsprintf(path, - HUGEPAGES_NUMA_PREFIX "node%d/hugepages/", - node) < 0) - goto cleanup; - } + return -1; }

- ret = 0; - cleanup: - return ret; + return 0; }

+static int +virNumaGetHugePageInfoDir(char **path, int node) +{ + if (node == -1) { + if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0) + return -1; + } else { + if (virAsprintf(path, + HUGEPAGES_NUMA_PREFIX "node%d/hugepages/", + node) < 0) + return -1;

Even more optimization could be to just return VIR_STRDUP or virAsprintf result. Similarly the virNumaGetHugePageInfoPath can have the same change - that is return directly from virAsprintf commands. I can adjust the commit message and nits - just let me know if the new commit message reads better. John

+ }

+ return 0; +} /** * virNumaGetHugePageInfo: * @node: NUMA node id @@ -724,7 +718,7 @@ virNumaGetPages(int node, * is always shown as used memory. Here, however, we want to report * slightly different information. So we take the total memory on a node * and subtract memory taken by the huge pages. */ - if (virNumaGetHugePageInfoPath(&path, node, 0, NULL) < 0) + if (virNumaGetHugePageInfoDir(&path, node) < 0) goto cleanup;

if (!(dir = opendir(path))) {

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

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

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