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

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?

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

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

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

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