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

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(a)redhat.com&gt; > --- > 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))) { >