[libvirt] [PATCH 02/17] nodeinfo: make nodeGetInfo() call nodeGetMemory for memory size

[John Ferlan]

On 04/14/2016 11:22 AM, Daniel P. Berrange wrote:
> The nodeGetInfo() method currently has its own code for getting
> memory size in KB, that basically just re-invents what nodeGetMemory
> already does. Remove it and just call nodeGetMemory, converting its
> result from bytes to KB, allowing removal of more platform specific
> conditional code.
> ---
>  src/nodeinfo.c | 38 +++++---------------------------------
>  1 file changed, 5 insertions(+), 33 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 1288543..bc5400f 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -81,33 +81,6 @@ appleFreebsdNodeGetCPUCount(void)
>  
>      return ncpu;
>  }
> -
> -/* VIR_HW_PHYSMEM - the resulting value of HW_PHYSMEM of FreeBSD
> - * is 64 bits while that of Mac OS X is still 32 bits.
> - * Mac OS X provides HW_MEMSIZE for 64 bits version of HW_PHYSMEM
> - * since 10.6.8 (Snow Leopard) at least.
> - */
> -# ifdef HW_MEMSIZE
> -#  define VIR_HW_PHYSMEM HW_MEMSIZE
> -# else
> -#  define VIR_HW_PHYSMEM HW_PHYSMEM
> -# endif
> -static int
> -appleFreebsdNodeGetMemorySize(unsigned long *memory)
> -{
> -    int mib[2] = { CTL_HW, VIR_HW_PHYSMEM };
> -    unsigned long physmem;
> -    size_t len = sizeof(physmem);
> -
> -    if (sysctl(mib, 2, &physmem, &len, NULL, 0) == -1) {
> -        virReportSystemError(errno, "%s", _("cannot obtain memory size"));
> -        return -1;
> -    }
> -
> -    *memory = (unsigned long)(physmem / 1024);
> -
> -    return 0;
> -}

I have no idea "how" FreeBSD and Apple do things, but this is different
than what's in nodeGetMemoryFake()...

Would that code need an "|| defined(__APPLE__)" though?  or it's own
APPLE only section using the above?

Although I suppose if nodeGetMemoryFake has been good enough "so far"
for the nodeGetMemory specific call, then it should be good enough for
the nodeGetInfo call (to at least get the same answer!).


>  #endif /* defined(__FreeBSD__) || defined(__APPLE__) */
>  
>  #ifdef __FreeBSD__
> @@ -1192,12 +1165,17 @@ int
>  nodeGetInfo(virNodeInfoPtr nodeinfo)
>  {
>      virArch hostarch = virArchFromHost();
> +    unsigned long long memorybytes;
>  
>      memset(nodeinfo, 0, sizeof(*nodeinfo));
>  
>      if (virStrcpyStatic(nodeinfo->model, virArchToString(hostarch)) == NULL)
>          return -1;
>  
> +    if (nodeGetMemory(&memorybytes, NULL) < 0)
> +        return -1;
> +    nodeinfo->memory = memorybytes / 1024;
> +

_virNodeInfo->memory is unsigned long
local memory is unsigned long long

Do we need any sort of overflow check ?  I imagine our compiler takes
care of the cast...

ACK - in principle at least as long as the "details" work out.

John
>  #ifdef __linux__
>      {
>      int ret = -1;
> @@ -1213,9 +1191,6 @@ nodeGetInfo(virNodeInfoPtr nodeinfo)
>      if (ret < 0)
>          goto cleanup;
>  
> -    /* Convert to KB. */
> -    nodeinfo->memory = physmem_total() / 1024;
> -
>   cleanup:
>      VIR_FORCE_FCLOSE(cpuinfo);
>      return ret;
> @@ -1251,9 +1226,6 @@ nodeGetInfo(virNodeInfoPtr nodeinfo)
>      nodeinfo->mhz = cpu_freq / 1000000;
>  # endif
>  
> -    if (appleFreebsdNodeGetMemorySize(&nodeinfo->memory) < 0)
> -        return -1;
> -
>      return 0;
>      }
>  #else
>