On Tue, Apr 26, 2016 at 02:20:18PM -0400, John Ferlan wrote:
>
>
> 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!).
The nodeGetMemoryFake method will end up calling physmem_total/available
provided by gnulib, which are supported on Darwin. For that matter they
are also supported on *BSD. So we can just kill that FreeBSD block from
the nodeGetMemoryFake method too. If there's issues with the gnulib impl
we should put the platform specific fixes in gnulib directly rather than
in libvirt.
> > + 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...
I think we're ok here - at least this isn't any worse than the
code already is. It is a shame we used unsigned long for the
NodeInfo struct, but we have to live with it.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list