[libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage

[Lei Li]

On 08/03/2011 05:29 AM, Daniel P. Berrange wrote:
> On Wed, Aug 03, 2011 at 12:52:50AM +0800, Lei Li wrote:
>> On 08/02/2011 07:11 PM, Daniel P. Berrange wrote:
>>> On Mon, Aug 01, 2011 at 02:12:51PM -0600, Eric Blake wrote:
>>>> On 07/31/2011 10:58 PM, Lei Li wrote:
>>>>> Make sure the unique storage pool defined and create from different directory to avoid inconsistent version of volume pool created.
>>>> Wrap your commit messages; typically at 70 columns or so (since 'git
>>>> log' adds some indentation, but you want the end result to still fit
>>>> in 80 columns for legibility).
>>>>
>>>>> Signed-off-by: Lei Li<lilei at linux.vnet.ibm.com>
>>>>> ---
>>>>>   src/conf/storage_conf.c      |   36 ++++++++++++++++++++++++++++++++++++
>>>>>   src/conf/storage_conf.h      |    4 ++++
>>>>>   src/libvirt_private.syms     |    2 ++
>>>>>   src/storage/storage_driver.c |    6 ++++++
>>>>>   4 files changed, 48 insertions(+), 0 deletions(-)
>>>>>
>>>>> +virStoragePoolObjPtr
>>>>> +virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
>>>>> +                            const char *path) {
>>>>> +    unsigned int i;
>>>>> +
>>>>> +    for (i = 0 ; i<    pools->count ; i++) {
>>>>> +        virStoragePoolObjLock(pools->objs[i]);
>>>>> +        if (STREQ(pools->objs[i]->def->target.path, path))
>>>>> +            return pools->objs[i];
>>>>> +        virStoragePoolObjUnlock(pools->objs[i]);
>>>>> +    }
>>>>> +
>>>>> +    return NULL;
>>>>> +}
>>>> This one is good; in fact, we may even want to expose it as a public
>>>> API, parallel to other virStoragePoolLookupBy* functions (but not
>>>> until after 0.9.4 is released)
>>> No, this API is flawed because def->target.path is not required to
>>> be unique for all types of storage pool.
>>>
>>> Daniel
>> Yes, in the beginning it seems like target->path is not required to be unique. But for this bug https://bugzilla.redhat.com/show_bug.cgi?id=611823
>> you reported, you said that "For example, if two directory pools point to the same directory, and one pool is used to create a volume,
>>   the other pool will remain unaware of the new volume until it is refreshed."  And I have test it when use 'virsh pool-define/create' it will create more
>> than two pools not two have the same directory. I think maybe you should look at the description of the bug first.
>> This API virStoragePoolObjFindByPath() just provide a method to search pool obj by path and can be use to avoid duplicate target path to fix this bug you mentioned.
> Ah a slight misunderstanding here. There are quite a few different pieces
> of metadata with storage pools, and in some cases they are the same.
>
>   - name/uuid - usual unique metadata for a storage pool
>   - source - the data for the source varies according to storage pool type
>       - hostname
>       - directory path
>       - adaptor
>       - device path
>       - source name
>       - initiator IQN
>
>   - target - a path that controls how storage volume paths are formed
>
> I think your misunderstanding is because for 'directory' storage pools,
> the source directory path is actually the same as the target path.
>
> So if you want to do uniqueness checking for storage pools, you need
> todo it based on the source information, rather than the target path.
> The checks will of course need to be slightly different for each
> storage pool type.
>
> Regards,
> Daniel
Hi Daniel,

I seriously considered your comments and look at the document about storage pool and volume XML format.
Yes, there are kinds type of pools, but the main goal of the bug #611823 you reported is to avoid an
inconsistent view of it's volume created on the same pool. The source directory info for each type of pool
maybe different, but if the target.path controls how storage volume paths are formed, why should we just
check the target.path to avoid this issue?


-- 
Lei