[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH]: Allow arbitrary paths to virStorageVolLookupByPath



Daniel P. Berrange wrote:
> On Fri, Oct 31, 2008 at 12:04:34PM +0100, Chris Lalancette wrote:
>> Index: src/storage_driver.c
>> ===================================================================
>> RCS file: /data/cvs/libvirt/src/storage_driver.c,v
>> retrieving revision 1.13
>> diff -u -r1.13 storage_driver.c
>> --- a/src/storage_driver.c	21 Oct 2008 17:15:53 -0000	1.13
>> +++ b/src/storage_driver.c	31 Oct 2008 10:09:29 -0000
>> @@ -963,11 +963,25 @@
>>      virStorageDriverStatePtr driver =
>>          (virStorageDriverStatePtr)conn->storagePrivateData;
>>      unsigned int i;
>> +    char *stable_path;
>>  
>>      for (i = 0 ; i < driver->pools.count ; i++) {
>>          if (virStoragePoolObjIsActive(driver->pools.objs[i])) {
>> -            virStorageVolDefPtr vol =
>> -                virStorageVolDefFindByPath(driver->pools.objs[i], path);
>> +            virStorageVolDefPtr vol;
>> +            virStorageBackendPoolOptionsPtr options;
>> +
>> +            options = virStorageBackendPoolOptionsForType(driver->pools.objs[i]->def->type);
>> +            if (options == NULL)
>> +                continue;
>> +
>> +            if (options->flags & VIR_STORAGE_BACKEND_POOL_STABLE_PATH)
>> +                stable_path = virStorageBackendStablePath(conn, driver->pools.objs[i], (char *)path);
>> +            else
>> +                stable_path = (char *)path;
>> +
>> +            vol =  virStorageVolDefFindByPath(driver->pools.objs[i], stable_path);
>> +            if (stable_path != path)
>> +                VIR_FREE(stable_path);
> 
> This VIR_FREE check is slightly evil, how about something more like

Well, I originally stole this pointer comparison from storage_backend_iscsi.c
which does the same thing.  While I agree that the code below is more clear, it
actually has a subtle bug; if you pass in, say "/dev/sdc", and the pool target
path is of type "/dev", then virStorageBackendStablePath() will return the
original pointer, not a copy.  So in the below code, you would actually end up
freeing path, not a copy of path.

Personally, I think those are bad semantics for virStorageBackendStablePath;
assuming it succeeds, you should always be able to know that you have a copy,
regardless of whether the copy is the same as the original.  Should I change
virStorageBackendStablePath to those semantics, in which case your below code
would then be correct?

> 
>               if (options->flags & VIR_STORAGE_BACKEND_POOL_STABLE_PATH) {
>                   char *stablepath = virStorageBackendStablePath(conn, driver->pools.objs[i], path);
>                   vol =  virStorageVolDefFindByPath(driver->pools.objs[i], stable_path);
>                   VIR_FREE(stablepath);
>               } else {
>                   vol =  virStorageVolDefFindByPath(driver->pools.objs[i], path);
>               } 

-- 
Chris Lalancette


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]