Re: [libvirt] [PATCH] Refresh QEMU driver caps in getCapabilities

[Cole Robinson <crobinso redhat com>]

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.

Thanks,
Cole

commit 0e51348cb9aeafe5e2fd6469a4bde0baa1eb8720
Author: Cole Robinson <crobinso redhat com>
Date:   Mon May 4 15:06:03 2009 -0400

    Refresh QEMU driver capabilities for each getCapabilities call.
    
    Also fix up a couple issues where caps are accessed without locking
    the driver structure.

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 23ea961..790dac6 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -1885,10 +1885,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);
 
@@ -3169,20 +3171,26 @@ cleanup:
     return ret;
 }
 
-static int qemudNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel)
+static int qemudNodeGetSecurityModel(virConnectPtr conn,
+                                     virSecurityModelPtr secmodel)
 {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     char *p;
+    int ret = 0;
 
-    if (!driver->securityDriver)
-        return -2;
+    qemuDriverLock(driver);
+    if (!driver->securityDriver) {
+        ret = -2;
+        goto cleanup;
+    }
 
     p = driver->caps->host.secModel.model;
     if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                          _("security model string exceeds max %d bytes"),
                          VIR_SECURITY_MODEL_BUFLEN-1);
-        return -1;
+        ret = -1;
+        goto cleanup;
     }
     strcpy(secmodel->model, p);
 
@@ -3191,10 +3199,14 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr sec
         qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                          _("security DOI string exceeds max %d bytes"),
                          VIR_SECURITY_DOI_BUFLEN-1);
-        return -1;
+        ret = -1;
+        goto cleanup;
     }
     strcpy(secmodel->doi, p);
-    return 0;
+
+cleanup:
+    qemuDriverUnlock(driver);
+    return ret;
 }
 
 /* TODO: check seclabel restore */