[libvirt] [PATCH] virNumaSetPagePoolSize: Produce friendlier error message
https://bugzilla.redhat.com/show_bug.cgi?id=1224587 The function takes two important arguments (among many others): @node and @page_size. From these two a path under /sys is constructed. The path is then used to read and write the desired size of huge pages pool. However, if the path does not exists due to either @node or @page_size having nonexistent value (e.g. there's no such NUMA node or no page size like -2), an cryptic error message is produced: virsh # allocpages --pagesize 2049 --pagecount 8 --cellno -2 error: Failed to open file '/sys/devices/system/node/node-2/hugepages/hugepages-2049kB/nr_hugepages': No such file or directory Add two more checks to catch this and therefore produce much more friendlier error messages. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnuma.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 669192a..5807d8f 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -849,9 +849,27 @@ 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)) { + /* Strictly speaking, @nr_path contains both NUMA node and page size. + * So if it doesn't exist it can be due to any of those two is wrong. + * However, the existence of the node was checked a few lines above, so + * it can be only page size here. */ + virReportError(VIR_ERR_OPERATION_FAILED, + _("page size %u not available"), + page_size); + goto cleanup; + } + /* Firstly check, if there's anything for us to do */ if (virFileReadAll(nr_path, 1024, &nr_buf) < 0) goto cleanup; -- 2.3.6
On Mon, Jun 08, 2015 at 02:41:05PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1224587
The function takes two important arguments (among many others): @node and @page_size. From these two a path under /sys is constructed. The path is then used to read and write the desired size of huge pages pool. However, if the path does not exists due to either @node or @page_size having nonexistent value (e.g. there's no such NUMA node or no page size like -2), an cryptic error message is produced:
virsh # allocpages --pagesize 2049 --pagecount 8 --cellno -2 error: Failed to open file '/sys/devices/system/node/node-2/hugepages/hugepages-2049kB/nr_hugepages': No such file or directory
Add two more checks to catch this and therefore produce much more friendlier error messages.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnuma.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 669192a..5807d8f 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -849,9 +849,27 @@ 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)) { + /* Strictly speaking, @nr_path contains both NUMA node and page size. + * So if it doesn't exist it can be due to any of those two is wrong. + * However, the existence of the node was checked a few lines above, so + * it can be only page size here. */
Über-strictly speaking, unless you compile with both WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET then virNumaNodeIsAvailable() can pass for invalid node in case of non-contiguous NUMA node numbers.
+ virReportError(VIR_ERR_OPERATION_FAILED, + _("page size %u not available"), + page_size); + goto cleanup; + } + /* Firstly check, if there's anything for us to do */ if (virFileReadAll(nr_path, 1024, &nr_buf) < 0) goto cleanup; -- 2.3.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09.06.2015 08:42, Martin Kletzander wrote:
On Mon, Jun 08, 2015 at 02:41:05PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1224587
The function takes two important arguments (among many others): @node and @page_size. From these two a path under /sys is constructed. The path is then used to read and write the desired size of huge pages pool. However, if the path does not exists due to either @node or @page_size having nonexistent value (e.g. there's no such NUMA node or no page size like -2), an cryptic error message is produced:
virsh # allocpages --pagesize 2049 --pagecount 8 --cellno -2 error: Failed to open file '/sys/devices/system/node/node-2/hugepages/hugepages-2049kB/nr_hugepages': No such file or directory
Add two more checks to catch this and therefore produce much more friendlier error messages.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnuma.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 669192a..5807d8f 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -849,9 +849,27 @@ 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)) { + /* Strictly speaking, @nr_path contains both NUMA node and page size. + * So if it doesn't exist it can be due to any of those two is wrong. + * However, the existence of the node was checked a few lines above, so + * it can be only page size here. */
Über-strictly speaking, unless you compile with both WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET then virNumaNodeIsAvailable() can pass for invalid node in case of non-contiguous NUMA node numbers.
So what are you saying is that I should update the comment or the error message or leave everything as-is and push it? Michal
On Wed, Jun 10, 2015 at 11:16:38AM +0200, Michal Privoznik wrote:
On 09.06.2015 08:42, Martin Kletzander wrote:
On Mon, Jun 08, 2015 at 02:41:05PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1224587
The function takes two important arguments (among many others): @node and @page_size. From these two a path under /sys is constructed. The path is then used to read and write the desired size of huge pages pool. However, if the path does not exists due to either @node or @page_size having nonexistent value (e.g. there's no such NUMA node or no page size like -2), an cryptic error message is produced:
virsh # allocpages --pagesize 2049 --pagecount 8 --cellno -2 error: Failed to open file '/sys/devices/system/node/node-2/hugepages/hugepages-2049kB/nr_hugepages': No such file or directory
Add two more checks to catch this and therefore produce much more friendlier error messages.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnuma.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 669192a..5807d8f 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -849,9 +849,27 @@ 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)) { + /* Strictly speaking, @nr_path contains both NUMA node and page size. + * So if it doesn't exist it can be due to any of those two is wrong. + * However, the existence of the node was checked a few lines above, so + * it can be only page size here. */
Über-strictly speaking, unless you compile with both WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET then virNumaNodeIsAvailable() can pass for invalid node in case of non-contiguous NUMA node numbers.
So what are you saying is that I should update the comment or the error message or leave everything as-is and push it?
It might've came out too strong. I just meant that the error message virFileReadAll would've gave sounds enough to me, but if you want to add this here, then I'd suggest mentioning the NUMA node there as well. However, feel free to correct me as I might misunderstood some higher purpose you had in mind; that's why I wanted to initiate a possible discussion about it if that was the case. Long story short, I'd say ACK with updated error message. Martin
participants (2)
-
Martin Kletzander -
Michal Privoznik