[libvirt] [PATCH] nodeinfo: fix physical memory size on Mac OS X

[Ryota Ozaki]

On Mon, Oct 21, 2013 at 11:57 PM, Doug Goldstein <cardoe at gentoo.org> wrote:
> On Sun, Oct 20, 2013 at 10:28 PM, Ryota Ozaki <ozaki.ryota at gmail.com> wrote:
>> Hi Doug,
>>
>>
>> On Mon, Oct 21, 2013 at 10:51 AM, Doug Goldstein <cardoe at gentoo.org> wrote:
>>>
>>> On Sun, Oct 20, 2013 at 10:14 AM, Ryota Ozaki <ozaki.ryota at gmail.com> wrote:
>>> > HW_PHYSMEM is available on Mac OS X as well as FreeBSD, however,
>>> > its resulting value for Mac OS X is 32 bits. Mac OS X provides
>>> > HW_MEMSIZE that is 64 bits version of HW_PHYSMEM. We have to use it.
>>> >
>>> > I tested the patch on Mac OS X 10.6.8, 10.7.4, 10.8.5 and FreeBSD 9.2.
>>> >
>>> > Signed-off-by: Ryota Ozaki <ozaki.ryota at gmail.com>
>>> > ---
>>> >  src/nodeinfo.c | 38 ++++++++++++++++++++++++++++----------
>>> >  1 file changed, 28 insertions(+), 10 deletions(-)
>>> >
>>> > diff --git a/src/nodeinfo.c b/src/nodeinfo.c
>>> > index 70814c2..a0fdfe7 100644
>>> > --- a/src/nodeinfo.c
>>> > +++ b/src/nodeinfo.c
>>> > @@ -73,6 +73,33 @@ 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;
>>> > +}
>>> >  #endif
>>> >
>>> >  #ifdef __linux__
>>> > @@ -914,17 +941,8 @@ cleanup:
>>> >      nodeinfo->mhz = cpu_freq / 1000000;
>>> >  # endif
>>> >
>>> > -    /* get memory information */
>>> > -    int mib[2] = { CTL_HW, 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"));
>>> > +    if (appleFreebsdNodeGetMemorySize(&nodeinfo->memory) < 0)
>>> >          return -1;
>>> > -    }
>>> > -
>>> > -    nodeinfo->memory = (unsigned long)(physmem / 1024);
>>> >
>>> >      return 0;
>>> >      }
>>> > --
>>> > 1.8.4
>>> >
>>> > --
>>> > libvir-list mailing list
>>> > libvir-list at redhat.com
>>> > https://www.redhat.com/mailman/listinfo/libvir-list
>>>
>>> I'd say ACK on this but I'm not sure how the other devs feel about
>>> breaking this out into another function with no clear benefit.
>>
>> The aim of the pull-out is to reduce the complexity of nodeGetInfo.
>> I can separate the patch into two, tidy-up and fix-up, if preferable.
>>
>> Thanks,
>>   ozaki-r
>
> I understand. the nodeGetInfo() at that point is ifdef'd only for
> FreeBSD and Apple at that point. But I do see another place we've done
> this in within that function so I don't see a problem merging this.
> I'll pull this in.

Thanks!
  ozaki-r

>
> --
> Doug Goldstein