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 <mprivozn(a)redhat.com&gt; >> --- >> 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?