[libvirt] [PATCH 1/2] util: split the virNumaGetHugePageInfoPath into separate function

[lhuang]

Sorry for the delay, i just came back from a long holiday.


On 10/02/2015 11:49 PM, John Ferlan wrote:
>
> On 09/30/2015 12:01 AM, Luyao Huang wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1265114
>>
>> When pass 0 page_size to virNumaGetHugePageInfoPath function, we will
>> get fail like this:
>>
>>   error : virFileReadAll:1358 : Failed to read file '/sys/devices/system/node/node0/hugepages/': Is a directory
>>
>> Because when the page_size is 0 the virNumaGetHugePageInfoPath will
>> build the directory of system path, but we don't want that.
>>
>> Introduce a new helper to build the dir path could avoid this issue.
>>
> This appears to be just a straight refactor - it took me a while to
> figure out why the key was 'page_size == 0'. Looking at "just" this
> context it would seem virNumaGetPages using 0 && suffix==NULL was the
> only user/cause for this.  But the real abuser/issue shows up in patch 2
> where if "other" callers use a page_size == 0, then we have issues.
>
> So, how about the following for a commit message:
>
> Refactor virNumaGetHugePageInfoPath to handle the passing a page_size
> of 0 and suffix == NULL into a new function virNumaGetHugePageInfoDir.
> Function virNumaGetPages expects to return a directory; whereas, other
> callers that passed a page_size == 0 would not expect a directory
> path in return. Each of those callers will use virFileReadAll which
> fails if a directory path is returned as follows:
>
>      error : virFileReadAll:1358 : Failed to read file
>              '/sys/devices/system/node/node0/hugepages/': Is a directory
>
>

Looks good to me, thanks a lot for your help.

>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/util/virnuma.c | 42 ++++++++++++++++++------------------------
>>   1 file changed, 18 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
>> index 1a62d62..8577bd8 100644
>> --- a/src/util/virnuma.c
>> +++ b/src/util/virnuma.c
>> @@ -493,45 +493,39 @@ virNumaGetHugePageInfoPath(char **path,
>>                              unsigned int page_size,
>>                              const char *suffix)
>>   {
>> -
>> -    int ret = -1;
>> -
>>       if (node == -1) {
>>           /* We are aiming at overall system info */
>> -        if (page_size) {
>> -            /* And even on specific huge page size */
>>               if (virAsprintf(path,
>>                               HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX "%ukB/%s",
>>                               page_size, suffix ? suffix : "") < 0)
> Now this has too many spaces shifted to the right (4 to be exact)

Indeed, i forgot this during wrote this patch.

>> -                goto cleanup;
>> -        } else {
>> -            if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
>> -                goto cleanup;
>> -        }
>> -
>> +                return -1;
>>       } else {
>>           /* We are aiming on specific NUMA node */
>> -        if (page_size) {
>> -            /* And even on specific huge page size */
>>               if (virAsprintf(path,
>>                               HUGEPAGES_NUMA_PREFIX "node%d/hugepages/"
>>                               HUGEPAGES_PREFIX "%ukB/%s",
>>                               node, page_size, suffix ? suffix : "") < 0)
> same note here
>
>> -                goto cleanup;
>> -        } else {
>> -            if (virAsprintf(path,
>> -                            HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
>> -                            node) < 0)
>> -                goto cleanup;
>> -        }
>> +                return -1;
>>       }
>>   
>> -    ret = 0;
>> - cleanup:
>> -    return ret;
>> +    return 0;
>>   }
>>   
>> +static int
>> +virNumaGetHugePageInfoDir(char **path, int node)
>> +{
>> +    if (node == -1) {
>> +        if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
>> +            return -1;
>> +    } else {
>> +        if (virAsprintf(path,
>> +                        HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
>> +                        node) < 0)
>> +            return -1;
> Even more optimization could be to just return VIR_STRDUP or virAsprintf
> result.
>
> Similarly the virNumaGetHugePageInfoPath can have the same change - that
> is return directly from virAsprintf commands.

Right, i missed this, thanks for pointing out that.

> I can adjust the commit message and nits - just let me know if the new
> commit message reads better.

The new commit message is better, thanks a lot for your review and help !

> John
>
>> +    }
>>   
>> +    return 0;
>> +}
>>   /**
>>    * virNumaGetHugePageInfo:
>>    * @node: NUMA node id
>> @@ -724,7 +718,7 @@ virNumaGetPages(int node,
>>        * is always shown as used memory. Here, however, we want to report
>>        * slightly different information. So we take the total memory on a node
>>        * and subtract memory taken by the huge pages. */
>> -    if (virNumaGetHugePageInfoPath(&path, node, 0, NULL) < 0)
>> +    if (virNumaGetHugePageInfoDir(&path, node) < 0)
>>           goto cleanup;
>>   
>>       if (!(dir = opendir(path))) {
>>

Luyao