[libvirt] [PATCH] nodedev: udev: Fix handling of wireless NIC
Wireless NICs were being ignored because we weren't correctly handling device type. Fix this, as well as wireless NIC net subtype. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/node_device/node_device_udev.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f0485f1..4915d4e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -597,8 +597,16 @@ static int udevProcessNetworkInterface(struct udev_device *device, virNodeDeviceDefPtr def) { int ret = -1; + const char *devtype = NULL; union _virNodeDevCapData *data = &def->caps->data; + devtype = udev_device_get_devtype(device); + 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 +1082,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)); if (devtype != NULL && STREQ(devtype, "usb_device")) { *type = VIR_NODE_DEV_CAP_USB_DEV; @@ -1112,7 +1122,7 @@ static int udevGetDeviceType(struct udev_device *device, /* It does not appear that network interfaces set the device type * property. */ - if (devtype == NULL && + if ((devtype == NULL || STREQ(devtype, "wlan")) && udevGetStringProperty(device, "INTERFACE", &tmp_string) == PROPERTY_FOUND) { -- 1.6.6.1
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.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/node_device/node_device_udev.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f0485f1..4915d4e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -597,8 +597,16 @@ static int udevProcessNetworkInterface(struct udev_device *device, virNodeDeviceDefPtr def) { int ret = -1; + const char *devtype = NULL; union _virNodeDevCapData *data = &def->caps->data;
+ devtype = udev_device_get_devtype(device);
You can combine the declaration and first assignment: const char *devtype = udev_device_get_devtype(device); to avoid a dead store (clang and coverity complain about those) and eliminate the minor duplication of that variable name.
+ 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 +1082,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));
if (devtype != NULL && STREQ(devtype, "usb_device")) { *type = VIR_NODE_DEV_CAP_USB_DEV; @@ -1112,7 +1122,7 @@ static int udevGetDeviceType(struct udev_device *device,
/* It does not appear that network interfaces set the device type * property. */ - if (devtype == NULL && + if ((devtype == NULL || STREQ(devtype, "wlan")) && udevGetStringProperty(device, "INTERFACE", &tmp_string) == PROPERTY_FOUND) {
Otherwise, this looks fine. ACK.
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
On 05/27/2010 10:58 AM, 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
ACK. - Cole
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@redhat.com> 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;
On Thu, May 27, 2010 at 06:12:25PM +0200, Jim Meyering wrote:
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@redhat.com> 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;
Ok, made those changes and pushed. Thanks for the review. Dave
participants (3)
-
Cole Robinson -
Dave Allan -
Jim Meyering