Re: [PATCH 07/12] libxl: Introduce libxlDriverConfig object

[Jim Fehlig <jfehlig@xxxxxxxx>]

Michal Privoznik wrote:
> On 30.08.2013 23:46, Jim Fehlig wrote:
>   
>> The libxlDriverPrivate struct contains an variety of data with
>> varying access needs. Similar to the QEMU and LXC drivers,
>> move all the static config data into a dedicated libxlDriverConfig
>> object. The only locking requirement is to hold the driver lock
>> while obtaining an instance of libxlDriverConfig. Once a reference
>> is held on the config object, it can be used completely lockless
>> since it is immutable.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
>> ---
>>  src/libxl/libxl_conf.c   | 124 ++++++++++++++++++-
>>  src/libxl/libxl_conf.h   |  52 +++++---
>>  src/libxl/libxl_driver.c | 313 +++++++++++++++++++++++------------------------
>>  3 files changed, 309 insertions(+), 180 deletions(-)
>>     
[...]
>>  
>> @@ -168,6 +177,8 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
>>      virDomainDefPtr def = NULL;
>>      libxlSavefileHeader hdr;
>>      char *xml = NULL;
>> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>> +    int ret = -1;
>>  
>>      if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
>>          virReportSystemError(-fd,
>> @@ -207,23 +218,25 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
>>          goto error;
>>      }
>>  
>> -    if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
>> +    if (!(def = virDomainDefParseString(xml, cfg->caps, driver->xmlopt,
>>                                          1 << VIR_DOMAIN_VIRT_XEN,
>>                                          VIR_DOMAIN_XML_INACTIVE)))
>>          goto error;
>>  [...]
>>
>>     
>> -    VIR_FREE(xml);
>> -
>>      *ret_def = def;
>>      *ret_hdr = hdr;
>>  
>> -    return fd;
>> +    ret = fd;
>> +    goto cleanup;
>>  
>>  error:
>> -    VIR_FREE(xml);
>>      virDomainDefFree(def);
>>      VIR_FORCE_CLOSE(fd);
>> -    return -1;
>> +
>> +cleanup:
>> +    VIR_FREE(xml);
>> +    virObjectUnref(cfg);
>> +    return ret;
>>     
>
> In libvirt we rather have the 'error' label below the 'cleanup'. It's
> more common to jump from the 'error' to 'cleanup' then. So can you
> please swap these two?
>   

Hmm, looking at this again I think adding the cleanup label was
overkill.  I've changed the above two hunks to the following, which is a
bit simpler.

@@ -168,6 +177,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver,
const char *from,
     virDomainDefPtr def = NULL;
     libxlSavefileHeader hdr;
     char *xml = NULL;
+    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
 
     if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
         virReportSystemError(-fd,
@@ -207,12 +217,13 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver,
const char *from,
         goto error;
     }
 
-    if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
+    if (!(def = virDomainDefParseString(xml, cfg->caps, driver->xmlopt,
                                         1 << VIR_DOMAIN_VIRT_XEN,
                                         VIR_DOMAIN_XML_INACTIVE)))
         goto error;
 
     VIR_FREE(xml);
+    virObjectUnref(cfg);
 
     *ret_def = def;
     *ret_hdr = hdr;
@@ -221,6 +232,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver,
const char *from,
 
 error:
     VIR_FREE(xml);
+    virObjectUnref(cfg);
     virDomainDefFree(def);
     VIR_FORCE_CLOSE(fd);
     return -1;

Regards,
Jim

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list