Re: [libvirt] [PATCH] virNumaSetPagePoolSize: Produce friendlier error message
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 mpriv...@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
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virNumaSetPagePoolSize: Produce friendlier error message
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 mpriv...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virNumaSetPagePoolSize: Produce friendlier error message
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 mpriv...@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
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
