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.

Signed-off-by: Luyao Huang <lhuang(a)redhat.com&gt; --- 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)

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

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

+ } + 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 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(+) > 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.

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.

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

John

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

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/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(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)) { s/path/nr_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); 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...

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?

> + } > 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.

I can adjust commit messages for syntax/english...

John