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

[Dave Allan]

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 at 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