[libvirt] [PATCH] virNumaSetPagePoolSize: Produce friendlier error message

Martin Kletzander mkletzan at redhat.com
Wed Jun 10 12:52:27 UTC 2015


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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150610/b0fa5924/attachment-0001.sig>


More information about the libvir-list mailing list