[Libvir] PATCH: Separate QEMU impl of nodeinfo API
While fixing the QEMU nodeinfo API to correctly deal with case where CPU sockets have sparse numbering (eg sockets 0 & 3 are populated), I realized that OpenVZ doesn't have a nodeinfo API, and its requirements are basically identical to the QEMU driver's. So this patch moves the impl of the nodeinfo API into a nodeinfo.c file, and makes both the QEMU and OpenVZ driver call out to this shared impl. I also put #ifdef __linux__ around the impl since code reading /proc/cpuinfo is never going to work on any non-Linux platform. For non linux I just return -1 which'll get treated as not-implemented. If QEMU driver is ported to work on Solaris, the nodeinfo.c file can be easily extended for their custom impl. Finally I'm adding a testcase with a bunch of example /proc/cpuinfo files to validate correctness Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
+ if (!strncmp(line, "processor", 9)) { //... + } else if (!strncmp(line, "cpu MHz", 7)) { Gaah. I know we don't have macros for these, but "== 0". There might be a pathological cpuinfo file which has another entry beginning with ^processor. If only we were using a real language which allowed simple use of regexps. Such is the state of software "engineering" .. The error handling in this next section of the patch is wrong. It should call __virRaiseError when the files cannot be opened or if the functions fail for some reason. In the Solaris case it should also call __virRaiseError (that print to stderr could be lost), but perhaps it's better to #error out? +#ifdef __linux__ + cpuinfo = fopen(CPUINFO_PATH, "r"); + if (!cpuinfo) + return -1; + ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo); + fclose(cpuinfo); + if (ret < 0) + return -1; + + meminfo = fopen(MEMINFO_PATH, "r"); + if (!meminfo) + return -1; + ret = linuxNodeInfoMemPopulate(meminfo, nodeinfo); + fclose(meminfo); + + return ret; +#else + /* XXX Solaris will need an impl later if they port QEMU driver */ + fprintf(stderr, "%s:%s not implemented on this platform\n", __FILE__, __FUNCTION__); + return -1; +#endif Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Wed, Jul 25, 2007 at 10:22:53AM +0100, Richard W.M. Jones wrote:
+ if (!strncmp(line, "processor", 9)) { //... + } else if (!strncmp(line, "cpu MHz", 7)) {
Gaah. I know we don't have macros for these, but "== 0".
We do now. STREQLEN(a,b,n) STRNEQLEN(a,b,n)
There might be a pathological cpuinfo file which has another entry beginning with ^processor. If only we were using a real language which allowed simple use of regexps.
It now checks that the data following is all whitespace upto the ':' character.
The error handling in this next section of the patch is wrong. It should call __virRaiseError when the files cannot be opened or if the functions fail for some reason. In the Solaris case it should also call __virRaiseError (that print to stderr could be lost), but perhaps it's better to #error out?
I made it use __virRaiseError everywhere. #error is unneccessarily harsh - all our APIs return -1 & raise an error if they're not implemented by a particular driver. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
+1. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 24, 2007 at 06:41:57PM +0100, Daniel P. Berrange wrote:
While fixing the QEMU nodeinfo API to correctly deal with case where CPU sockets have sparse numbering (eg sockets 0 & 3 are populated), I realized that OpenVZ doesn't have a nodeinfo API, and its requirements are basically identical to the QEMU driver's. So this patch moves the impl of the nodeinfo API into a nodeinfo.c file, and makes both the QEMU and OpenVZ driver call out to this shared impl. I also put #ifdef __linux__ around the impl since code reading /proc/cpuinfo is never going to work on any non-Linux platform. For non linux I just return -1 which'll get treated as not-implemented. If QEMU driver is ported to work on Solaris, the nodeinfo.c file can be easily extended for their custom impl. Finally I'm adding a testcase with a bunch of example /proc/cpuinfo files to validate correctness
Sounds just right, thanks !
diff -u -p -u -p -r1.46 Makefile.am --- src/Makefile.am 19 Jul 2007 16:22:40 -0000 1.46 +++ src/Makefile.am 24 Jul 2007 17:40:32 -0000 @@ -50,6 +50,7 @@ CLIENT_SOURCES = \ qemu_conf.c qemu_conf.h \ openvz_conf.c openvz_conf.h \ openvz_driver.c openvz_driver.h \ + nodeinfo.h nodeinfo.c \ util.c util.h
I will have to exclude those from docs/generator.py (or fix the logic of said generator selection of files) Looks good, +1, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones