Re: [libvirt] [PATCH 1/9] pci: Make reattach work for unbound devices

On 17.01.2014 11:39, Jiri Denemark wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1046919 > > When a PCI device is not bound to any driver, reattach should just > trigger driver probe rather than failing with > > Invalid device 0000:00:19.0 driver file > /sys/bus/pci/devices/0000:00:19.0/driver is not a symlink > > While virPCIDeviceGetDriverPathAndName was documented to return success > and NULL driver and path when a device is not attached to any driver but > didn't do so. Thus callers could not distinguish unbound devices from > failures. > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > src/util/virpci.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/src/util/virpci.c b/src/util/virpci.c > index 51dbee6..8577fd4 100644 > --- a/src/util/virpci.c > +++ b/src/util/virpci.c > @@ -236,6 +236,11 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) > if (virPCIFile(&drvlink, dev->name, "driver") < 0) > goto cleanup; > > + if (!virFileExists(drvlink)) { > + ret = 0; > + goto cleanup; > + } > + > if (virFileIsLink(drvlink) != 1) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Invalid device %s driver file %s is not a symlink"), > @@ -1023,6 +1028,11 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) > if (virPCIDeviceGetDriverPathAndName(dev, &drvdir, &driver) < 0) > goto cleanup; > > + if (!driver) { > + /* The device is not bound to any driver and we are almost done. */ > + goto reprobe; > + } > + > if (!dev->unbind_from_stub) > goto remove_slot; > > @@ -1079,11 +1089,10 @@ reprobe: > * available, then re-probing would just cause the device to be > * re-bound to the stub. > */ > - if (virPCIDriverFile(&path, driver, "remove_id") < 0) { > + if (driver && virPCIDriverFile(&path, driver, "remove_id") < 0) > goto cleanup; > - } > Up to here I understand the patch. > - if (!virFileExists(drvdir) || virFileExists(path)) { > + if (!driver || !virFileExists(drvdir) || virFileExists(path)) { > if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) { > virReportSystemError(errno, > _("Failed to trigger a re-probe for PCI device '%s'"), > But this bit doesn't make much sense to me. If a device is bound to a stub driver, say pci-stub, then the @path="/sys/bus/pci/drivers/pci-stub/remove_id"; I don't see a reason how/why this should have any affect on decision whether to write into "/sys/bus/pci/drivers_probe" or not. Even though it's pre-existing, now that you're touching the line it makes sense to do it right.