[libvirt] [PATCH] nodeinfo: fix detection of physical memory on uclibc/musl libc
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@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
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@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 :|
On Tue, 15 Apr 2014 10:37:40 +0100 "Daniel P. Berrange" <berrange@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@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
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@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@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 :|
On Tue, 15 Apr 2014 12:30:47 +0100 "Daniel P. Berrange" <berrange@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@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@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
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
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@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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Natanael Copa