[libvirt] [PATCHv2 0/3] improve the error report for virNodeAllocPages and virNodeGetFreePages
v1: https://www.redhat.com/archives/libvir-list/2015-September/msg01054.html v2: -improve the error message -improve the code struct -add a new patch for rework the exist way to check node and page size Luyao Huang (3): util: split the virNumaGetHugePageInfoPath into separate function util: move the pagesize and node check and error report to one place util: Produce friendlier error message to user src/util/virnuma.c | 113 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 72 insertions(+), 41 deletions(-) -- 1.8.3.1
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@redhat.com> --- src/util/virnuma.c | 51 +++++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 1a62d62..cb80972 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -493,44 +493,31 @@ 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 virAsprintf(path, + HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX "%ukB/%s", + page_size, suffix ? suffix : ""); } 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 virAsprintf(path, + HUGEPAGES_NUMA_PREFIX "node%d/hugepages/" + HUGEPAGES_PREFIX "%ukB/%s", + node, page_size, suffix ? suffix : ""); } - - ret = 0; - cleanup: - return ret; } +static int +virNumaGetHugePageInfoDir(char **path, int node) +{ + if (node == -1) { + return VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX); + } else { + return virAsprintf(path, + HUGEPAGES_NUMA_PREFIX "node%d/hugepages/", + node); + } +} /** * virNumaGetHugePageInfo: @@ -724,7 +711,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))) { -- 1.8.3.1
Just like 1c24cfe9, check the pagesize and numa node if we cannot find the hugepage path. And improve the error message again. After this patch: # virsh allocpages --pagesize 2047 --pagecount 1 --cellno 0 error: operation failed: page size 2047 is not available on node 0 # virsh allocpages --pagesize 2047 --pagecount 1 error: operation failed: page size 2047 is not available Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/util/virnuma.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index cb80972..815cbc1 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -836,19 +836,25 @@ 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 (node != -1) { + if (!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 on node %d"), + page_size, node); + } + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("page size %u is not available"), + page_size); + } goto cleanup; } -- 1.8.3.1
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 | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 815cbc1..cd000f9 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node, page_size, "nr_hugepages") < 0) goto cleanup; + if (!virFileExists(path)) { + if (node != -1) { + if (!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 on node %d"), + page_size, 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 +591,25 @@ virNumaGetHugePageInfo(int node, page_size, "free_hugepages") < 0) goto cleanup; + if (!virFileExists(path)) { + if (node != -1) { + if (!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 on node %d"), + page_size, 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; -- 1.8.3.1
On 10/21/2015 12:13 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 | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
Now that I see things in their final form, one more suggestion which I can take care of... You've repeated the same "if (!virFileExists)" check and error messages for all 3 callers to virNumaGetHugePageInfo. So I think moving that check into virNumaGetHugePageInfo would now be appropriate (see the attached patch which I'll squash) I'll also fix up the commit messages and push some time later today John
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 815cbc1..cd000f9 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node, page_size, "nr_hugepages") < 0) goto cleanup;
+ if (!virFileExists(path)) { + if (node != -1) { + if (!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 on node %d"), + page_size, 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 +591,25 @@ virNumaGetHugePageInfo(int node, page_size, "free_hugepages") < 0) goto cleanup;
+ if (!virFileExists(path)) { + if (node != -1) { + if (!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 on node %d"), + page_size, 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;
On 10/21/2015 09:58 PM, John Ferlan wrote:
On 10/21/2015 12:13 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 | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
Now that I see things in their final form, one more suggestion which I can take care of... You've repeated the same "if (!virFileExists)" check and error messages for all 3 callers to virNumaGetHugePageInfo.
So I think moving that check into virNumaGetHugePageInfo would now be appropriate (see the attached patch which I'll squash)
I'll also fix up the commit messages and push some time later today
Oh, right, i should move the check in virNumaGetHugePageInfo to avoid code duplicated. Your patch looks good to me. And thanks a lot for your quick review and lots of help!
John
Luyao
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 815cbc1..cd000f9 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node, page_size, "nr_hugepages") < 0) goto cleanup;
+ if (!virFileExists(path)) { + if (node != -1) { + if (!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 on node %d"), + page_size, 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 +591,25 @@ virNumaGetHugePageInfo(int node, page_size, "free_hugepages") < 0) goto cleanup;
+ if (!virFileExists(path)) { + if (node != -1) { + if (!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 on node %d"), + page_size, 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;
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang