[libvirt] [PATCH] udev: Don't try to dump DMI on non-intel archs
DMI is Intel & Intel-compatible specific. Don't try to dump information on non-compatible architectures, which results only in error message in logs. --- NB: libsmbios is exclusively for x86_64 ia64 %{ix86} src/node_device/node_device_udev.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a6b5b2e..2c5d016 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1477,6 +1477,8 @@ out: } +/* DMI is intel-compatible specific */ +#if defined(__x86_64__) || defined(__i386__) || defined(__amd64__) static void udevGetDMIData(union _virNodeDevCapData *data) { @@ -1549,6 +1551,7 @@ out: } return; } +#endif static int udevSetupSystemDev(void) @@ -1573,7 +1576,9 @@ static int udevSetupSystemDev(void) goto out; } +#if defined(__x86_64__) || defined(__i386__) || defined(__amd64__) udevGetDMIData(&def->caps->data); +#endif dev = virNodeDeviceAssignDef(&driverState->devs, def); if (dev == NULL) { -- 1.7.5.rc3
On 07/19/2011 08:52 AM, Michal Privoznik wrote:
DMI is Intel& Intel-compatible specific. Don't try to dump information on non-compatible architectures, which results only in error message in logs. --- NB: libsmbios is exclusively for x86_64 ia64 %{ix86}
src/node_device/node_device_udev.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
ACK. But do we also need to patch src/util/sysinfo.c, which makes callouts to dmidecode(1)? That is, if libsmbios is arch-specific, then I imagine that so is dmidecode, and do we gracefully handle dmidecode failures in that case? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 19.07.2011 17:01, Eric Blake wrote:
On 07/19/2011 08:52 AM, Michal Privoznik wrote:
DMI is Intel& Intel-compatible specific. Don't try to dump information on non-compatible architectures, which results only in error message in logs. --- NB: libsmbios is exclusively for x86_64 ia64 %{ix86}
src/node_device/node_device_udev.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
ACK.
But do we also need to patch src/util/sysinfo.c, which makes callouts to dmidecode(1)? That is, if libsmbios is arch-specific, then I imagine that so is dmidecode, and do we gracefully handle dmidecode failures in that case?
You're right. dmidecode is arch-specific as well. Right now we just fail in running it. Is it enough or should that be enclosed in conditional compilation as well? Michal
On 07/19/2011 09:12 AM, Michal Privoznik wrote:
On 19.07.2011 17:01, Eric Blake wrote:
On 07/19/2011 08:52 AM, Michal Privoznik wrote:
DMI is Intel& Intel-compatible specific. Don't try to dump information on non-compatible architectures, which results only in error message in logs. --- NB: libsmbios is exclusively for x86_64 ia64 %{ix86}
src/node_device/node_device_udev.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
ACK.
But do we also need to patch src/util/sysinfo.c, which makes callouts to dmidecode(1)? That is, if libsmbios is arch-specific, then I imagine that so is dmidecode, and do we gracefully handle dmidecode failures in that case?
You're right. dmidecode is arch-specific as well. Right now we just fail in running it. Is it enough or should that be enclosed in conditional compilation as well?
That depends on how things behave on other arches. I don't have ready access to such an architecture, which is why I'm asking - do we already gracefully handle any failures on code paths that try to use dmidecode, or are there currently code paths that make unconditional use of dmidecode (and fail) which should be made arch-conditional? It's pointless to write a patch until we know what behavior we're patching. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 19.07.2011 17:17, Eric Blake wrote:
On 07/19/2011 09:12 AM, Michal Privoznik wrote:
On 19.07.2011 17:01, Eric Blake wrote:
On 07/19/2011 08:52 AM, Michal Privoznik wrote:
DMI is Intel& Intel-compatible specific. Don't try to dump information on non-compatible architectures, which results only in error message in logs. --- NB: libsmbios is exclusively for x86_64 ia64 %{ix86}
src/node_device/node_device_udev.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
ACK.
But do we also need to patch src/util/sysinfo.c, which makes callouts to dmidecode(1)? That is, if libsmbios is arch-specific, then I imagine that so is dmidecode, and do we gracefully handle dmidecode failures in that case?
You're right. dmidecode is arch-specific as well. Right now we just fail in running it. Is it enough or should that be enclosed in conditional compilation as well?
That depends on how things behave on other arches. I don't have ready access to such an architecture, which is why I'm asking - do we already gracefully handle any failures on code paths that try to use dmidecode, or are there currently code paths that make unconditional use of dmidecode (and fail) which should be made arch-conditional?
It's pointless to write a patch until we know what behavior we're patching.
So I've managed to run libvirt on ppc64. Here are the results: # virsh sysinfo error: failed to get sysinfo error: unsupported configuration: Host SMBIOS information is not available and in logs: 05:25:40.682: 29653: error : virSysinfoRead:462 : internal error Failed to find path for dmidecode binary which get logged on daemon startup, not that virsh command. Michal
On 07/20/2011 08:09 AM, Michal Privoznik wrote:
That depends on how things behave on other arches. I don't have ready access to such an architecture, which is why I'm asking - do we already gracefully handle any failures on code paths that try to use dmidecode, or are there currently code paths that make unconditional use of dmidecode (and fail) which should be made arch-conditional?
It's pointless to write a patch until we know what behavior we're patching.
So I've managed to run libvirt on ppc64. Here are the results:
# virsh sysinfo error: failed to get sysinfo error: unsupported configuration: Host SMBIOS information is not available
Reasonable - we're gracefully handling the lack of information, and with an appropriate error message.
and in logs: 05:25:40.682: 29653: error : virSysinfoRead:462 : internal error Failed to find path for dmidecode binary
Not so nice. "internal error" in a log message always sparks worry among people, even if in this case it did not stop libvirt from doing useful work. I would be in favor of a patch to src/util/sysinfo.c that changes the #ifdef WIN32 around virSysinfoRead() stub that fails with ENOSYS instead of VIR_ERR_INTERNAL_ERROR to also non-x86 arches. Could you test this, to see whether that makes the log less scary? diff --git i/src/util/sysinfo.c w/src/util/sysinfo.c index 2c8e687..9cd6849 100644 --- i/src/util/sysinfo.c +++ w/src/util/sysinfo.c @@ -113,7 +113,10 @@ void virSysinfoDefFree(virSysinfoDefPtr def) * * Returns: a filled up sysinfo structure or NULL in case of error */ -#ifdef WIN32 +#if defined(WIN32) || \ + defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__) virSysinfoDefPtr virSysinfoRead(void) { /* @@ -125,7 +128,7 @@ virSysinfoRead(void) { return NULL; } -#else /* !WIN32 */ +#else /* !WIN32 && x86 */ static char * virSysinfoParseBIOS(char *base, virSysinfoDefPtr ret) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 20.07.2011 16:20, Eric Blake wrote:
On 07/20/2011 08:09 AM, Michal Privoznik wrote:
That depends on how things behave on other arches. I don't have ready access to such an architecture, which is why I'm asking - do we already gracefully handle any failures on code paths that try to use dmidecode, or are there currently code paths that make unconditional use of dmidecode (and fail) which should be made arch-conditional?
It's pointless to write a patch until we know what behavior we're patching.
So I've managed to run libvirt on ppc64. Here are the results:
# virsh sysinfo error: failed to get sysinfo error: unsupported configuration: Host SMBIOS information is not available
Reasonable - we're gracefully handling the lack of information, and with an appropriate error message.
and in logs: 05:25:40.682: 29653: error : virSysinfoRead:462 : internal error Failed to find path for dmidecode binary
Not so nice. "internal error" in a log message always sparks worry among people, even if in this case it did not stop libvirt from doing useful work. I would be in favor of a patch to src/util/sysinfo.c that changes the #ifdef WIN32 around virSysinfoRead() stub that fails with ENOSYS instead of VIR_ERR_INTERNAL_ERROR to also non-x86 arches. Could you test this, to see whether that makes the log less scary?
diff --git i/src/util/sysinfo.c w/src/util/sysinfo.c index 2c8e687..9cd6849 100644 --- i/src/util/sysinfo.c +++ w/src/util/sysinfo.c @@ -113,7 +113,10 @@ void virSysinfoDefFree(virSysinfoDefPtr def) * * Returns: a filled up sysinfo structure or NULL in case of error */ -#ifdef WIN32 +#if defined(WIN32) || \ + defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__) virSysinfoDefPtr virSysinfoRead(void) { /* @@ -125,7 +128,7 @@ virSysinfoRead(void) { return NULL; }
-#else /* !WIN32 */ +#else /* !WIN32 && x86 */
static char * virSysinfoParseBIOS(char *base, virSysinfoDefPtr ret)
In fact, we need a patch with complement condition: diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index 2c8e687..6625cae 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -113,7 +113,10 @@ void virSysinfoDefFree(virSysinfoDefPtr def) * * Returns: a filled up sysinfo structure or NULL in case of error */ -#ifdef WIN32 +#if defined(WIN32) || \ + !(defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__)) virSysinfoDefPtr virSysinfoRead(void) { /* @@ -125,7 +128,7 @@ virSysinfoRead(void) { return NULL; } -#else /* !WIN32 */ +#else /* !WIN32 && x86 */ static char * virSysinfoParseBIOS(char *base, virSysinfoDefPtr ret) @@ -509,7 +512,7 @@ no_memory: ret = NULL; goto cleanup; } -#endif /* !WIN32 */ +#endif /* !WIN32 && x86 */ static void virSysinfoBIOSFormat(virSysinfoDefPtr def, const char *prefix, With this patch we transfer to error: 10:51:36.515: 4412: error : virSysinfoRead:127 : Host sysinfo extraction not supported on this platform: Function not implemented I think this is the right way of dealing with this, isn't it? Michal
On 07/20/2011 08:54 AM, Michal Privoznik wrote:
-#ifdef WIN32 +#if defined(WIN32) || \ + defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__)
In fact, we need a patch with complement condition:
Doh, serves me right for typing without testing.
diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index 2c8e687..6625cae 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -113,7 +113,10 @@ void virSysinfoDefFree(virSysinfoDefPtr def) * * Returns: a filled up sysinfo structure or NULL in case of error */ -#ifdef WIN32 +#if defined(WIN32) || \ + !(defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__))
Much nicer.
With this patch we transfer to error: 10:51:36.515: 4412: error : virSysinfoRead:127 : Host sysinfo extraction not supported on this platform: Function not implemented
I think this is the right way of dealing with this, isn't it?
Yes, that looks nicer. ACK to committing this as a patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 20.07.2011 16:58, Eric Blake wrote:
On 07/20/2011 08:54 AM, Michal Privoznik wrote:
-#ifdef WIN32 +#if defined(WIN32) || \ + defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__)
In fact, we need a patch with complement condition:
Doh, serves me right for typing without testing.
diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index 2c8e687..6625cae 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -113,7 +113,10 @@ void virSysinfoDefFree(virSysinfoDefPtr def) * * Returns: a filled up sysinfo structure or NULL in case of error */ -#ifdef WIN32 +#if defined(WIN32) || \ + !(defined(__x86_64__) || \ + defined(__i386__) || \ + defined(__amd64__))
Much nicer.
With this patch we transfer to error: 10:51:36.515: 4412: error : virSysinfoRead:127 : Host sysinfo extraction not supported on this platform: Function not implemented
I think this is the right way of dealing with this, isn't it?
Yes, that looks nicer. ACK to committing this as a patch.
Pushed. Michal
On 07/19/2011 09:01 AM, Eric Blake wrote:
But do we also need to patch src/util/sysinfo.c, which makes callouts to dmidecode(1)? That is, if libsmbios is arch-specific, then I imagine that so is dmidecode, and do we gracefully handle dmidecode failures in that case?
Oh, and while I'm thinking about it: on x86 architectures with newer kernels, the kernel is starting to provide various SMBIOS information to user space via sysfs, without requiring the running of root-only dmidecode. We need to teach libvirt to try that method, so that qemu:///session can take advantage of copying host SMBIOS information to the guest. https://www.redhat.com/archives/virt-tools-list/2011-July/msg00012.html -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 20.07.2011 16:25, Eric Blake wrote:
On 07/19/2011 09:01 AM, Eric Blake wrote:
But do we also need to patch src/util/sysinfo.c, which makes callouts to dmidecode(1)? That is, if libsmbios is arch-specific, then I imagine that so is dmidecode, and do we gracefully handle dmidecode failures in that case?
Oh, and while I'm thinking about it:
on x86 architectures with newer kernels, the kernel is starting to provide various SMBIOS information to user space via sysfs, without requiring the running of root-only dmidecode. We need to teach libvirt to try that method, so that qemu:///session can take advantage of copying host SMBIOS information to the guest.
https://www.redhat.com/archives/virt-tools-list/2011-July/msg00012.html
well, sysfs currently offer much less info than dmidecode. But it still might be worth of expanding it. Michal
participants (2)
-
Eric Blake -
Michal Privoznik