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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 20, 2013 at 10:28 PM, Ryota Ozaki <ozaki.ryota@xxxxxxxxx> wrote:
> Hi Doug,
>
>
> On Mon, Oct 21, 2013 at 10:51 AM, Doug Goldstein <cardoe@xxxxxxxxxx> wrote:
>>
>> On Sun, Oct 20, 2013 at 10:14 AM, Ryota Ozaki <ozaki.ryota@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxxx
>> > 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]