On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote: > Hi all, > > The attached patch fixes QEMU getCapabilities to refresh caps before > returning them to the user. Currently the capabilities are only > refreshed once (at daemon startup), which means libvirtd needs to be > restarted to pick up changes if QEMU or KVM are installed while the > daemon is running. See: > https://bugzilla.redhat.com/show_bug.cgi?id=460649 > > There are several things 'wrong' with this change: > > - We reset/rescan fields that won't change (host arch, mac address > prefix). This should be fixed at some point, but isn't a big deal since > total performance impact is negligible (see below). > > - We only refresh the capabilities when the user calls getCapabilities, > which means we are still carrying around stale caps prior to that, which > is what libvirt validates against. In practice, virt-manager and > virt-install both call getCapabilities often so this isn't an issue, > though the caps internal API should probably address this at some point. > > To test the performance impact, I used a simple python script: > > import libvirt > conn = libvirt.open("qemu:///system") > for i in range(0, 30): > conn.getCapabilities() > > The time difference was on average .02 seconds slower, which I think is > negligible. More importantly I don't see this API call as being a performance critical one. It is not something applicaitions will be calling inside a hot loop. > diff --git a/src/qemu_driver.c b/src/qemu_driver.c > index 79ee072..6b5c17f 100644 > --- a/src/qemu_driver.c > +++ b/src/qemu_driver.c > @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn, > > static char *qemudGetCapabilities(virConnectPtr conn) { > struct qemud_driver *driver = conn->privateData; > - char *xml; > + char *xml = NULL; > > qemuDriverLock(driver); > - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) > + virCapabilitiesFree(qemu_driver->caps); > + if ((qemu_driver->caps = qemudCapsInit()) == NULL || > + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) > virReportOOMError(conn); > qemuDriverUnlock(driver); The thing to be wary of now, is that all use of driver->caps needs to be protected by the driver mutex. Most usages are OK, but I spotted a couple that are not. Daniel

Daniel P. Berrange wrote: > On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote: > > >> diff --git a/src/qemu_driver.c b/src/qemu_driver.c >> index 79ee072..6b5c17f 100644 >> --- a/src/qemu_driver.c >> +++ b/src/qemu_driver.c >> @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn, >> >> static char *qemudGetCapabilities(virConnectPtr conn) { >> struct qemud_driver *driver = conn->privateData; >> - char *xml; >> + char *xml = NULL; >> >> qemuDriverLock(driver); >> - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) >> + virCapabilitiesFree(qemu_driver->caps); >> + if ((qemu_driver->caps = qemudCapsInit()) == NULL || >> + (xml = virCapabilitiesFormatXML(driver->caps)) == NULL) >> virReportOOMError(conn); >> qemuDriverUnlock(driver); > > The thing to be wary of now, is that all use of driver->caps needs > to be protected by the driver mutex. Most usages are OK, but I > spotted a couple that are not. > > Daniel Okay, updated patch attached. The only unsafe caps usage I found was in qemudNodeGetSecurityModel, not sure if you spotted any others.