Re: [libvirt] [PATCH] nodedev: udev: Fix handling of wireless NIC

Dave Allan wrote: > On Wed, May 26, 2010 at 03:55:44PM -0400, Cole Robinson wrote: >> Wireless NICs were being ignored because we weren't correctly handling >> device type. Fix this, as well as wireless NIC net subtype. > > Thanks for finding and fixing this. I made a v2 patch with the change > that since DEVTYPE == "wlan" is a definitive indication that the > device is a network interface, that case should be with the other > device types for which DEVTYPE is definitive. v2 patch attached. > > Dave > > >>From ccf89c57a6335c1c7c7e0b29ae09f4916b33622d Mon Sep 17 00:00:00 2001 > From: David Allan <dallan(a)redhat.com&gt; > Date: Thu, 27 May 2010 10:44:02 -0400 > Subject: [PATCH 1/1] v2 of Cole's wlan support > > * Incorporated Jim's feedback > > * Moved case of DEVTYPE == "wlan" up as it's definitive that we have a network interface. > > * Made comment more detailed about the wired case to explain better > how it differentiates between wired network interfaces and USB > devices. ... > int ret = -1; Thanks Dave. > + const char *devtype = udev_device_get_devtype(device); > union _virNodeDevCapData *data = &def->caps->data; > > + if (devtype && STREQ(devtype, "wlan")) { > + data->net.subtype = VIR_NODE_DEV_CAP_NET_80211; > + } else { > + data->net.subtype = VIR_NODE_DEV_CAP_NET_80203; > + } > + > if (udevGetStringProperty(device, > "INTERFACE", > &data->net.ifname) == PROPERTY_ERROR) { > @@ -1074,6 +1081,8 @@ static int udevGetDeviceType(struct udev_device *device, > int ret = 0; > > devtype = udev_device_get_devtype(device); > + VIR_DEBUG("Found device type '%s' for device '%s'", > + devtype, udev_device_get_sysname(device)); Sorry I missed it the first time, but since devtype can be NULL here, we have to avoid dereferencing it: (it wouldn't cause trouble with glibc, but would segfault on others) VIR_DEBUG("Found device type '%s' for device '%s'", NULLSTR(devtype), udev_device_get_sysname(device)); > if (devtype != NULL && STREQ(devtype, "usb_device")) { > *type = VIR_NODE_DEV_CAP_USB_DEV; > @@ -1105,17 +1114,25 @@ static int udevGetDeviceType(struct udev_device *device, > goto out; > } > > + if (devtype != NULL && STREQ(devtype, "wlan")) { > + *type = VIR_NODE_DEV_CAP_NET; > + goto out; > + } > + > if (udevGetUintProperty(device, "PCI_CLASS", &tmp, 16) == PROPERTY_FOUND) { > *type = VIR_NODE_DEV_CAP_PCI_DEV; > goto out; > } > > - /* It does not appear that network interfaces set the device type > - * property. */ > - if (devtype == NULL && > + /* It does not appear that wired network interfaces set the > + * DEVTYPE property. USB devices also have an INTERFACE property, > + * but they do set DEVTYPE, so if devtype is NULL and the > + * INTERFACE property exists, we have a network device. */ > + if ((devtype == NULL) && I wouldn't bother to add parens above. > udevGetStringProperty(device, > "INTERFACE", > &tmp_string) == PROPERTY_FOUND) { > + IMHO, that added blank line detracts from readability. > VIR_FREE(tmp_string); > *type = VIR_NODE_DEV_CAP_NET; > goto out;