4:31 a.m.
The gnulib's physmem_total will as a fallback report 64MB as total
memory if sysconf(_SC_PHYS_PAGES) is unimplemented on linux. This makes
it impossible to detect if physmem_total works or not, so we try first
the linux only sysinfo(2) before falling back to gnulibs physmem_total.
This makes the total memory be correctly reported on musl libc and
uclibc.
Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
---
configure.ac | 2 +-
src/nodeinfo.c | 22 ++++++++++++++++------
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac
index 52c50df..32a2e5a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -310,7 +310,7 @@ dnl Availability of various common headers (non-fatal if missing).
AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/un.h \
sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \
sys/un.h sys/syscall.h sys/sysctl.h netinet/tcp.h ifaddrs.h \
- libtasn1.h sys/ucred.h sys/mount.h])
+ libtasn1.h sys/ucred.h sys/mount.h sys/sysinfo.h])
dnl Check whether endian provides handy macros.
AC_CHECK_DECLS([htole64], [], [], [[#include <endian.h>]])
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index c88f86c..3c76f54 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -40,6 +40,10 @@
# include <sys/resource.h>
#endif
+#if HAVE_SYS_SYSINFO_H
+# include <sys/sysinfo.h>
+#endif
+
#include "c-ctype.h"
#include "viralloc.h"
#include "nodeinfopriv.h"
@@ -1041,7 +1045,18 @@ int nodeGetInfo(virNodeInfoPtr nodeinfo)
#ifdef __linux__
{
int ret = -1;
- FILE *cpuinfo = fopen(CPUINFO_PATH, "r");
+ FILE *cpuinfo;
+#if HAVE_SYS_SYSINFO_H
+ struct sysinfo si;
+
+ /* Convert to KB. */
+ if (sysinfo(&si) == 0) {
+ nodeinfo->memory = si.totalram / 1024;
+ } else
+#endif
+ nodeinfo->memory = physmem_total() / 1024;
+
+ cpuinfo = fopen(CPUINFO_PATH, "r");
if (!cpuinfo) {
virReportSystemError(errno,
_("cannot open %s"), CPUINFO_PATH);
@@ -1049,11 +1064,6 @@ int nodeGetInfo(virNodeInfoPtr nodeinfo)
}
ret = linuxNodeInfoCPUPopulate(cpuinfo, SYSFS_SYSTEM_PATH, nodeinfo);
- if (ret < 0)
- goto cleanup;
-
- /* Convert to KB. */
- nodeinfo->memory = physmem_total() / 1024;
cleanup:
VIR_FORCE_FCLOSE(cpuinfo);
--
1.9.2
4:37 a.m.
On Tue, Apr 15, 2014 at 11:31:23AM +0200, Natanael Copa wrote:
The gnulib's physmem_total will as a fallback report 64MB as
total
memory if sysconf(_SC_PHYS_PAGES) is unimplemented on linux. This makes
it impossible to detect if physmem_total works or not, so we try first
the linux only sysinfo(2) before falling back to gnulibs physmem_total.
This makes the total memory be correctly reported on musl libc and
uclibc.
Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
---
configure.ac | 2 +-
src/nodeinfo.c | 22 ++++++++++++++++------
2 files changed, 17 insertions(+), 7 deletions(-)
I think you should instead patch the gnulib physmem_total() function
to support use of sysinfo() itself. That way all the many users of
GNULIB will work correctly on musl/uclibc, rather than just libvirt.
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 :|
6:26 a.m.
On Tue, 15 Apr 2014 10:37:40 +0100
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Tue, Apr 15, 2014 at 11:31:23AM +0200, Natanael Copa wrote:
> The gnulib's physmem_total will as a fallback report 64MB as total
> memory if sysconf(_SC_PHYS_PAGES) is unimplemented on linux. This makes
> it impossible to detect if physmem_total works or not, so we try first
> the linux only sysinfo(2) before falling back to gnulibs physmem_total.
>
> This makes the total memory be correctly reported on musl libc and
> uclibc.
>
> Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
> ---
> configure.ac | 2 +-
> src/nodeinfo.c | 22 ++++++++++++++++------
> 2 files changed, 17 insertions(+), 7 deletions(-)
I think you should instead patch the gnulib physmem_total() function
to support use of sysinfo() itself. That way all the many users of
GNULIB will work correctly on musl/uclibc, rather than just libvirt.
the intention was to fix all 3 places:
* fix libvirt not trust physmem_total
* fix gnulib to use sysinfo and fallback to sysconf
* add _SC_PHYS_PAGES support to musl libc
In any case, the gnulib's physmem_total will most likely always fall
back to 64MB instead of fail with error since it looks like thats the
way it is designed. I doubt they will accept fix for that.
That means that libvirt cannot really trust physmem_total and IMHO it
should be avoided.
The effective result is that you cannot create any vm with more than
64MB (MB not GB...) ram with virt-manager which I think is worse than
fail with error.
-nc
6:30 a.m.
On Tue, Apr 15, 2014 at 01:26:27PM +0200, Natanael Copa wrote:
On Tue, 15 Apr 2014 10:37:40 +0100
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Tue, Apr 15, 2014 at 11:31:23AM +0200, Natanael Copa wrote:
> > The gnulib's physmem_total will as a fallback report 64MB as total
> > memory if sysconf(_SC_PHYS_PAGES) is unimplemented on linux. This makes
> > it impossible to detect if physmem_total works or not, so we try first
> > the linux only sysinfo(2) before falling back to gnulibs physmem_total.
> >
> > This makes the total memory be correctly reported on musl libc and
> > uclibc.
> >
> > Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
> > ---
> > configure.ac | 2 +-
> > src/nodeinfo.c | 22 ++++++++++++++++------
> > 2 files changed, 17 insertions(+), 7 deletions(-)
>
> I think you should instead patch the gnulib physmem_total() function
> to support use of sysinfo() itself. That way all the many users of
> GNULIB will work correctly on musl/uclibc, rather than just libvirt.
the intention was to fix all 3 places:
* fix libvirt not trust physmem_total
* fix gnulib to use sysinfo and fallback to sysconf
* add _SC_PHYS_PAGES support to musl libc
In any case, the gnulib's physmem_total will most likely always fall
back to 64MB instead of fail with error since it looks like thats the
way it is designed. I doubt they will accept fix for that.
That means that libvirt cannot really trust physmem_total and IMHO it
should be avoided.
The whole point of physmem_total is that libvirt can avoid having to
have a bunch of different implementations. Any kind of fix that libvirt
could do, can equally be done in the physmem_total, so there's no reason
to not rely on physmem_total. So we only need to fix physmem_total, and
the musl libc.
The only reason to fix libvirt, is if there's some problem that would
prevent us getting the fix into gnulib in an acceptable timeframe.
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 :|
9:50 a.m.
On Tue, 15 Apr 2014 12:30:47 +0100
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Tue, Apr 15, 2014 at 01:26:27PM +0200, Natanael Copa wrote:
> On Tue, 15 Apr 2014 10:37:40 +0100
> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>
> > On Tue, Apr 15, 2014 at 11:31:23AM +0200, Natanael Copa wrote:
> > > The gnulib's physmem_total will as a fallback report 64MB as total
> > > memory if sysconf(_SC_PHYS_PAGES) is unimplemented on linux. This makes
> > > it impossible to detect if physmem_total works or not, so we try first
> > > the linux only sysinfo(2) before falling back to gnulibs physmem_total.
> > >
> > > This makes the total memory be correctly reported on musl libc and
> > > uclibc.
> > >
> > > Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org>
> > > ---
> > > configure.ac | 2 +-
> > > src/nodeinfo.c | 22 ++++++++++++++++------
> > > 2 files changed, 17 insertions(+), 7 deletions(-)
> >
> > I think you should instead patch the gnulib physmem_total() function
> > to support use of sysinfo() itself. That way all the many users of
> > GNULIB will work correctly on musl/uclibc, rather than just libvirt.
>
> the intention was to fix all 3 places:
>
> * fix libvirt not trust physmem_total
> * fix gnulib to use sysinfo and fallback to sysconf
> * add _SC_PHYS_PAGES support to musl libc
>
> In any case, the gnulib's physmem_total will most likely always fall
> back to 64MB instead of fail with error since it looks like thats the
> way it is designed. I doubt they will accept fix for that.
>
> That means that libvirt cannot really trust physmem_total and IMHO it
> should be avoided.
The whole point of physmem_total is that libvirt can avoid having to
have a bunch of different implementations. Any kind of fix that libvirt
could do, can equally be done in the physmem_total, so there's no reason
to not rely on physmem_total. So we only need to fix physmem_total, and
the musl libc.
The only reason to fix libvirt, is if there's some problem that would
prevent us getting the fix into gnulib in an acceptable timeframe.
Fair enough.
I posted a bug report and a patch to bugs-gnulib.
-nc
4:03 p.m.
On 04/15/2014 08:50 AM, Natanael Copa wrote:
>
> The whole point of physmem_total is that libvirt can avoid having to
> have a bunch of different implementations. Any kind of fix that libvirt
> could do, can equally be done in the physmem_total, so there's no reason
> to not rely on physmem_total. So we only need to fix physmem_total, and
> the musl libc.
>
> The only reason to fix libvirt, is if there's some problem that would
> prevent us getting the fix into gnulib in an acceptable timeframe.
Fair enough.
I posted a bug report and a patch to bugs-gnulib.
And it was accepted, so I tweaked libvirt's .gnulib submodule to include
it. Thanks again for tracking this!
https://www.redhat.com/archives/libvir-list/2014-April/msg00788.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:06 a.m.
On Tue, Apr 15, 2014 at 04:50:08PM +0200, Natanael Copa wrote:
On Tue, 15 Apr 2014 12:30:47 +0100
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>
> The whole point of physmem_total is that libvirt can avoid having to
> have a bunch of different implementations. Any kind of fix that libvirt
> could do, can equally be done in the physmem_total, so there's no reason
> to not rely on physmem_total. So we only need to fix physmem_total, and
> the musl libc.
>
> The only reason to fix libvirt, is if there's some problem that would
> prevent us getting the fix into gnulib in an acceptable timeframe.
Fair enough.
I posted a bug report and a patch to bugs-gnulib.
Thanks for making the effort / taking the time to fix gnulib.
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 :|
3902
days inactive
3909
days old
6 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Natanael Copa