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
