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 <lhu...@redhat.com> >>> --- >>> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list