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

[John Ferlan]

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


> 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)

> -                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.

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

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))) {
>