6:32 a.m.
The udev node device driver tries to glean a few tidbits of information
from /sys/devices/virtual/dmi/id. If either the BIOS or the kernel does
not support DMI, libvirtd logs the error
Failed to get udev device for syspath '/sys/devices/virtual/dmi/id' or
'/sys/class/dmi/id'
and refuses to start.
This seems a bit extreme; information obtained from DMI, like the system
board vendor and BIOS version, is useful but not critical to libvirt's
operation.
This patch eliminates the hard dependency on DMI. On systems without
DMI, libvirtd logs a warning, and leaves the "computer" node device's
properties empty.
Signed-off-by: Ed Swierk <eswierk(a)aristanetworks.com>
---
Index: libvirt-0.7.6/src/node_device/node_device_udev.c
===================================================================
--- libvirt-0.7.6.orig/src/node_device/node_device_udev.c
+++ libvirt-0.7.6/src/node_device/node_device_udev.c
@@ -1471,9 +1471,9 @@ static int udevSetupSystemDev(void)
if (device == NULL) {
device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK);
if (device == NULL) {
- VIR_ERROR("Failed to get udev device for syspath '%s' or
'%s'",
- DMI_DEVPATH, DMI_DEVPATH_FALLBACK);
- goto out;
+ VIR_WARN("Failed to get udev device for syspath '%s' or
'%s'",
+ DMI_DEVPATH, DMI_DEVPATH_FALLBACK);
+ goto out2;
}
}
@@ -1532,6 +1532,7 @@ static int udevSetupSystemDev(void)
udev_device_unref(device);
+out2:
dev = virNodeDeviceAssignDef(&driverState->devs, def);
if (dev == NULL) {
VIR_ERROR("Failed to create device for '%s'", def->name);
6:57 a.m.
On 03/03/2010 05:32 PM, Ed Swierk wrote:
The udev node device driver tries to glean a few tidbits of
information
from /sys/devices/virtual/dmi/id. If either the BIOS or the kernel does
not support DMI, libvirtd logs the error
Failed to get udev device for syspath '/sys/devices/virtual/dmi/id' or
'/sys/class/dmi/id'
and refuses to start.
This seems a bit extreme; information obtained from DMI, like the system
board vendor and BIOS version, is useful but not critical to libvirt's
operation.
Agreed.
This patch eliminates the hard dependency on DMI. On systems
without
DMI, libvirtd logs a warning, and leaves the "computer" node device's
properties empty.
Although I use goto a lot, I generally try to avoid multiple labels
within a function, just because I think it gets out of hand really
quickly. Although it's a slightly more invasive patch, would you
refactor the code to look something like what I've attached? I haven't
even compile tested it as I'm running late, but that's the idea.
Dave
Signed-off-by: Ed Swierk<eswierk(a)aristanetworks.com>
---
Index: libvirt-0.7.6/src/node_device/node_device_udev.c
===================================================================
--- libvirt-0.7.6.orig/src/node_device/node_device_udev.c
+++ libvirt-0.7.6/src/node_device/node_device_udev.c
@@ -1471,9 +1471,9 @@ static int udevSetupSystemDev(void)
if (device == NULL) {
device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK);
if (device == NULL) {
- VIR_ERROR("Failed to get udev device for syspath '%s' or
'%s'",
- DMI_DEVPATH, DMI_DEVPATH_FALLBACK);
- goto out;
+ VIR_WARN("Failed to get udev device for syspath '%s' or
'%s'",
+ DMI_DEVPATH, DMI_DEVPATH_FALLBACK);
+ goto out2;
}
}
@@ -1532,6 +1532,7 @@ static int udevSetupSystemDev(void)
udev_device_unref(device);
+out2:
dev = virNodeDeviceAssignDef(&driverState->devs, def);
if (dev == NULL) {
VIR_ERROR("Failed to create device for '%s'", def->name);
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:20 a.m.
On Wed, Mar 3, 2010 at 2:57 PM, Dave Allan <dallan(a)redhat.com> wrote:
Although I use goto a lot, I generally try to avoid multiple labels
within a
function, just because I think it gets out of hand really quickly. Although
it's a slightly more invasive patch, would you refactor the code to look
something like what I've attached? I haven't even compile tested it as I'm
running late, but that's the idea.
Is there a piece of code in libvirt that exemplifies the preferred
error handling style? (http://libvirt.org/hacking.html doesn't cover
this issue, as far as I can tell.) Just in the very small part of
libvirt I've hacked on recently I've found a variety of styles,
including
- pair every allocation with a goto label that frees the allocation
and all the earlier ones, and goto the appropriate label on error
- don't use goto at all, and on error, do the necessary frees and
return -1, with each error case having to do one more free
- a combination of the above, with each error case doing the necessary
frees, but using goto out more or less as an alias for return -1
- none of the above, not bothering to free anything when an allocation
fails (see udevSetupSystemDev for an example)
There are probably arguments to be made for each of these styles, but
it would be helpful to know which of them is preferred when writing
new code or refactoring existing code.
That said, I'll gladly refactor my patch towards the preferred style.
--Ed
2:31 a.m.
On 03/03/2010 07:20 PM, Ed Swierk wrote:
On Wed, Mar 3, 2010 at 2:57 PM, Dave Allan<dallan(a)redhat.com>
wrote:
> Although I use goto a lot, I generally try to avoid multiple labels within a
> function, just because I think it gets out of hand really quickly. Although
> it's a slightly more invasive patch, would you refactor the code to look
> something like what I've attached? I haven't even compile tested it as
I'm
> running late, but that's the idea.
Is there a piece of code in libvirt that exemplifies the preferred
error handling style? (http://libvirt.org/hacking.html doesn't cover
this issue, as far as I can tell.) Just in the very small part of
libvirt I've hacked on recently I've found a variety of styles,
including
Agreed that we should add a statement to the hacking guide. My
preferences are as follows.
- pair every allocation with a goto label that frees the allocation
and all the earlier ones, and goto the appropriate label on error
I like Robert Love's description of this style at the very end of the
thread at:
http://kerneltrap.org/node/553/2131
I like this style, but my impression is that generally the libvirt
community prefers to have a single label that frees everything, perhaps
conditionally on error, unless it's absolutely necessary to have
multiple labels.
I reworked udevSetupSystemDev into this style (which also fixes the bug
you pointed out that it didn't properly free resources on error). The
patch also makes failure to find DMI data non-fatal.
- don't use goto at all, and on error, do the necessary frees
and
return -1, with each error case having to do one more free
I find this style troublesome to maintain, as any additional allocations
require modifications to each error case.
- a combination of the above, with each error case doing the
necessary
frees, but using goto out more or less as an alias for return -1
Again, I think duplicating the frees in each error case is less
maintainable than having them in on place.
- none of the above, not bothering to free anything when an
allocation
fails (see udevSetupSystemDev for an example)
Failure to cleanup is a bug. Please send mail (and, even better,
patches) about any other instances you find.
There are probably arguments to be made for each of these styles,
but
it would be helpful to know which of them is preferred when writing
new code or refactoring existing code.
That said, I'll gladly refactor my patch towards the preferred style.
--Ed
8:49 a.m.
On Thu, Mar 4, 2010 at 10:31 AM, Dave Allan <dallan(a)redhat.com> wrote:
I reworked udevSetupSystemDev into this style (which also fixes the
bug you
pointed out that it didn't properly free resources on error). The patch
also makes failure to find DMI data non-fatal.
Your patch works fine. Thanks.
Acked-by: Ed Swierk <eswierk(a)aristanetworks.com>
1:41 a.m.
On 03/04/2010 07:49 PM, Ed Swierk wrote:
On Thu, Mar 4, 2010 at 10:31 AM, Dave Allan<dallan(a)redhat.com>
wrote:
> I reworked udevSetupSystemDev into this style (which also fixes the bug you
> pointed out that it didn't properly free resources on error). The patch
> also makes failure to find DMI data non-fatal.
Your patch works fine. Thanks.
Acked-by: Ed Swierk<eswierk(a)aristanetworks.com>
Thanks for the ACK; anybody else willing to provide the second?
Dave
7:04 a.m.
On 03/05/2010 10:41 AM, Dave Allan wrote:
On 03/04/2010 07:49 PM, Ed Swierk wrote:
> On Thu, Mar 4, 2010 at 10:31 AM, Dave Allan<dallan(a)redhat.com> wrote:
>> I reworked udevSetupSystemDev into this style (which also fixes the
>> bug you
>> pointed out that it didn't properly free resources on error). The patch
>> also makes failure to find DMI data non-fatal.
>
> Your patch works fine. Thanks.
>
> Acked-by: Ed Swierk<eswierk(a)aristanetworks.com>
Thanks for the ACK; anybody else willing to provide the second?
>From 23526641083527139548c68b4637bae8350d2f98 Mon Sep 17 00:00:00
2001
From: David Allan <dallan(a)redhat.com>
Date: Thu, 4 Mar 2010 13:17:24 -0500
Subject: [PATCH 1/1] Free resources on error in udev startup
* The udev driver didn't properly free resources that it allocates when setting up
the 'computer' device in the error case.
I didn't see this one in the repository yet; but if it helps to have
another review via code inspection, I've read through your patch and it
looks like a sane split in functionality. Therefore:
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:39 a.m.
On 03/09/2010 06:04 PM, Eric Blake wrote:
On 03/05/2010 10:41 AM, Dave Allan wrote:
> On 03/04/2010 07:49 PM, Ed Swierk wrote:
>> On Thu, Mar 4, 2010 at 10:31 AM, Dave Allan<dallan(a)redhat.com> wrote:
>>> I reworked udevSetupSystemDev into this style (which also fixes the
>>> bug you
>>> pointed out that it didn't properly free resources on error). The patch
>>> also makes failure to find DMI data non-fatal.
>>
>> Your patch works fine. Thanks.
>>
>> Acked-by: Ed Swierk<eswierk(a)aristanetworks.com>
>
> Thanks for the ACK; anybody else willing to provide the second?
> > From 23526641083527139548c68b4637bae8350d2f98 Mon Sep 17 00:00:00 2001
> From: David Allan<dallan(a)redhat.com>
> Date: Thu, 4 Mar 2010 13:17:24 -0500
> Subject: [PATCH 1/1] Free resources on error in udev startup
>
> * The udev driver didn't properly free resources that it allocates when setting
up the 'computer' device in the error case.
I didn't see this one in the repository yet; but if it helps to have
another review via code inspection, I've read through your patch and it
looks like a sane split in functionality. Therefore:
ACK.
Ok, thanks, pushed.
Dave
5:14 p.m.
On Thu, Mar 04, 2010 at 01:31:33PM -0500, Dave Allan wrote:
On 03/03/2010 07:20 PM, Ed Swierk wrote:
>On Wed, Mar 3, 2010 at 2:57 PM, Dave Allan<dallan(a)redhat.com> wrote:
>>Although I use goto a lot, I generally try to avoid multiple labels
>>within a
>>function, just because I think it gets out of hand really quickly.
>>Although
>>it's a slightly more invasive patch, would you refactor the code to look
>>something like what I've attached? I haven't even compile tested it as
>>I'm
>>running late, but that's the idea.
>
>Is there a piece of code in libvirt that exemplifies the preferred
>error handling style? (http://libvirt.org/hacking.html doesn't cover
>this issue, as far as I can tell.) Just in the very small part of
>libvirt I've hacked on recently I've found a variety of styles,
>including
Agreed that we should add a statement to the hacking guide. My
preferences are as follows.
>- pair every allocation with a goto label that frees the allocation
>and all the earlier ones, and goto the appropriate label on error
I like Robert Love's description of this style at the very end of the
thread at:
http://kerneltrap.org/node/553/2131
I like this style, but my impression is that generally the libvirt
community prefers to have a single label that frees everything, perhaps
conditionally on error, unless it's absolutely necessary to have
multiple labels.
If the cleanup code only involves free'ing memory, then having multiple
labels is complete overkill. VIR_FREE() and every function named XXXFree()
in libvirt is required to handle NULL as its arg. Thus you can safely
call free on all the variables even if they wre not yet allocated (yes
they have to have been initialized to NULL). This is much simpler & clearer
than having multiple gotos
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5348
days inactive
5355
days old
8 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Dave Allan -
Ed Swierk -
Eric Blake