[libvirt] PATCH: Include OS driver name in device information

[Daniel P. Berrange]

On Wed, Jun 03, 2009 at 12:43:36PM +0100, Mark McLoughlin wrote:
> Hey,
> 
> Looks fine to me, but if you're looking for comments ... :-)
> 
> On Fri, 2009-05-29 at 14:39 +0100, Daniel P. Berrange wrote:
> 
> > To, also include the operating system driver name. 
> > 
> > $ virsh nodedev-dumpxml pci_8086_27d6
> > <device>
> >   <name>pci_8086_27d6</name>
> >   <parent>computer</parent>
> >   <driver>
> >     <name>pcieport-driver</name>
> >   </driver>
> >   <capability type='pci'>
> >     <domain>0</domain>
> >     <bus>0</bus>
> >     <slot>28</slot>
> >     <function>3</function>
> >     <product id='0x27d6'>82801G (ICH7 Family) PCI Express Port 4</product>
> >     <vendor id='0x8086'>Intel Corporation</vendor>
> >   </capability>
> > </device>
> >
> > We're not defining any particular semantics for the driver name, it is
> > just a opaque string following rules of the OS in question.
> 
> The obvious question is "what's this for?"
> 
> If this is just an opaque string, what should people use it for?

To determine what driver is associated with a device.

> 
> > diff -r ec78a5d6c00c src/node_device.c
> > --- a/src/node_device.c	Thu May 28 14:42:24 2009 +0100
> > +++ b/src/node_device.c	Thu May 28 16:00:12 2009 +0100
> > @@ -46,6 +46,60 @@ static int dev_has_cap(const virNodeDevi
> >      return 0;
> >  }
> >  
> > +#ifdef __linux__
> > +static int update_driver_name(virConnectPtr conn,
> > +                              virNodeDeviceObjPtr dev)
> > +{
> > +    char *driver_link = NULL;
> > +    char devpath[PATH_MAX];
> > +    char *p;
> > +    int ret = -1;
> > +    int n;
> > +
> > +    VIR_FREE(dev->def->driver);
> > +
> > +    if (virAsprintf(&driver_link, "%s/driver", dev->devicePath) < 0) {
> > +        virReportOOMError(conn);
> > +        goto cleanup;
> > +    }
> > +
> > +    /* Some devices don't have an explicit driver, so just return
> > +       without a name */
> > +    if (access(driver_link, R_OK) < 0) {
> > +        ret = 0;
> > +        goto cleanup;
> > +    }
> > +
> > +    if ((n = readlink(driver_link, devpath, sizeof devpath)) < 0) {
> > +        virReportSystemError(conn, errno,
> > +                             _("cannot resolve driver link %s"), driver_link);
> 
> Do we really want dumpxml to fail for this?

This would only fail if the kernel had created a broken symlink, so I
think its fine to return an error here. We already check for whether
it actually exists just before.

> 
> > +        goto cleanup;
> > +    }
> > +    devpath[n] = '\0';
> > +
> > +    p = strrchr(devpath, '/');
> > +    if (p) {
> > +        dev->def->driver = strdup(p+1);
> 
> basename() ?

Basename isn't a very nice function becasue it potentially
modifies the original string.

> (you already use basename() in storage_backend_disk.c)

Didn't notice them - one of them is definitely risky due
to the potential for modification of original string. Need
to kill those IMHO

> > diff -r ec78a5d6c00c src/node_device_hal.c
> > --- a/src/node_device_hal.c	Thu May 28 14:42:24 2009 +0100
> > +++ b/src/node_device_hal.c	Thu May 28 16:00:12 2009 +0100
> 
> Probably should add a FIXME in node_device_devkit.c about adding support
> for this.

Yeah, the devkit.c driver is just sooooo incomplete, mostly reflecting
the inadequacies of device kit itself. Not really sure where togo with
that. It is verging on being better to kill this junk and just talk to
udev directly, because I'm told the info that's missing won't be added
that they we need to fetch it from sysfs ourselves. Talking to devicekit
for just a small part of the data is looking like a waste of time.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|