On Sun, Oct 20, 2013 at 10:14 AM, Ryota Ozaki <ozaki.ryota(a)gmail.com&gt; 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(a)gmail.com&gt; > --- > 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(a)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.

-- Doug Goldstein

On Sun, Oct 20, 2013 at 10:28 PM, Ryota Ozaki <ozaki.ryota(a)gmail.com&gt; wrote: > Hi Doug, > > > On Mon, Oct 21, 2013 at 10:51 AM, Doug Goldstein <cardoe(a)gentoo.org&gt; wrote: >> >> On Sun, Oct 20, 2013 at 10:14 AM, Ryota Ozaki <ozaki.ryota(a)gmail.com&gt; 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(a)gmail.com&gt; >> > --- >> > 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(a)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.

-- Doug Goldstein