[Kimchi-devel] [PATCH v2 1/5] storagepool: dispatch _get_pool_xml() to respective _get_XXX_storagepool_xml() function

[Zhou Zheng Sheng]

on 2013/12/26 23:04, Aline Manera wrote:
> On 12/22/2013 08:55 AM, Zhou Zheng Sheng wrote:
>> In src/kimchi/model.py, _get_pool_xml() is to generate libvirt storage
>> pool XML definition from arguments provided by the POST data (a JSON
>> dict). It has to support various types of pool such as dir, netfs and
>> logical. Now it uses "if ... elif ... elif" to check the requested type
>> of pool and go in to the respective branch and generates the correct
>> XML.
>>
>> This is OK for now but in future we'll have more types of pool, so in
>> this patch we turn "if ... elif" into a dispatching dict, and put the
>> respective XML generating code to separated functions.
>>
>> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
>> ---
>>   src/kimchi/model.py | 121
>> +++++++++++++++++++++++++++++++---------------------
>>   1 file changed, 73 insertions(+), 48 deletions(-)
>>
>> diff --git a/src/kimchi/model.py b/src/kimchi/model.py
>> index 3bc5d6d..0a497b5 100644
>> --- a/src/kimchi/model.py
>> +++ b/src/kimchi/model.py
>> @@ -1430,60 +1430,85 @@ class LibvirtVMScreenshot(VMScreenshot):
>>           finally:
>>               os.close(fd)
>>
>> -def _get_pool_xml(**kwargs):
>> +
>> +def _get_dir_storagepool_xml(poolArgs):
>>       # Required parameters
>>       # name:
>>       # type:
>>       # path:
>> +    xml = """
>> +    <pool type='%(type)s'>
> 
> As you separated functions according to type, you can define the type on
> xml directly
> 
> <pool type='dir'>
> 
Correct, I'll change this part for dir, netfs and logical pool in v3.
Thanks very much!
>> +      <name>%(name)s</name>
>> +      <target>
>> +        <path>%(path)s</path>
>> +      </target>
>> +    </pool>
>> +    """ % poolArgs
>> +    return xml
>> +
>> +
>> +def _get_netfs_storagepool_xml(poolArgs):
>> +    # Required parameters
>> +    # name:
>> +    # type:
>> +    # nfsserver:
>>       # nfspath:
>> -    if kwargs['type'] == 'dir':
>> -        xml = """
>> -        <pool type='%(type)s'>
>> -          <name>%(name)s</name>
>> -          <target>
>> -            <path>%(path)s</path>
>> -          </target>
>> -        </pool>
>> -       """
>> -    elif kwargs['type'] == 'netfs':
>> -        path = '/var/lib/kimchi/nfs_mount/' + kwargs['name'];
>> -        if not os.path.exists(path):
>> -           os.makedirs(path)
>> -        kwargs.update({'path': path})
>> -        xml = """
>> -        <pool type='%(type)s'>
>> -          <name>%(name)s</name>
>> -          <source>
>> -            <host name='%(nfsserver)s'/>
>> -            <dir path='%(nfspath)s'/>
>> -          </source>
>> -          <target>
>> -            <path>%(path)s</path>
>> -          </target>
>> -        </pool>
>> -        """
>> -    elif kwargs['type'] == 'logical':
>> -        path = '/var/lib/kimchi/logical_mount/' + kwargs['name'];
>> -        if not os.path.exists(path):
>> -           os.makedirs(path)
>> -        xml = """
>> -        <pool type='%(type)s'>
>> -        <name>%(name)s</name>
>> -            <source>
>> -            %(devices)s
>> -            </source>
>> -        <target>
>> -            <path>%(path)s</path>
>> -        </target>
>> -        </pool>
>> -        """
>> -        devices = ''
>> -        for device_path in kwargs['devices']:
>> -            devices += "<device path=\""+device_path+"\" />"
> 
>> +    path = '/var/lib/kimchi/nfs_mount/' + poolArgs['name']
>> +    if not os.path.exists(path):
>> +        os.makedirs(path)
> 
> I think pool.build() creates everything needed to get pool created so
> you don't need the code above.
> 
Yes. This is not introduced by this patch series, it's from the existing
code. I'll test it and see if we can remove the mkdirs call.
>> +    poolArgs['path'] = path
>> +    xml = """
>> +    <pool type='%(type)s'>
> 
> Same as I commented above about the type
> 
> <pool type'netfs'>
> 
>> +      <name>%(name)s</name>
>> +      <source>
>> +        <host name='%(nfsserver)s'/>
>> +        <dir path='%(nfspath)s'/>
>> +      </source>
>> +      <target>
>> +        <path>%(path)s</path>
>> +      </target>
>> +    </pool>
>> +    """ % poolArgs
>> +    return xml
>> +
>> +
>> +def _get_logical_storagepool_xml(poolArgs):
>> +    # Required parameters
>> +    # name:
>> +    # type:
>> +    # devices:
> 
>> +    path = '/var/lib/kimchi/logical_mount/' + poolArgs['name']
>> +    if not os.path.exists(path):
>> +        os.makedirs(path)
>> +
> 
> poo.build() will care about it
> 
>> +    devices = []
>> +    for device_path in poolArgs['devices']:
>> +        devices.append('<device path="%s" />' % device_path)
>> +
>> +    poolArgs.update({'devices': ''.join(devices),
>> +                     'path': path})
>> +    xml = """
>> +    <pool type='%(type)s'>
> 
> <pool type='logical'>
> 
>> +    <name>%(name)s</name>
>> +        <source>
>> +        %(devices)s
>> +        </source>
>> +    <target>
>> +        <path>%(path)s</path>
>> +    </target>
>> +    </pool>
>> +    """ % poolArgs
>> +    return xml
>> +
>> +
>> +def _get_pool_xml(**kwargs):
>> +    getPoolXml = {
>> +        'dir': _get_dir_storagepool_xml,
>> +        'netfs': _get_netfs_storagepool_xml,
>> +        'logical': _get_logical_storagepool_xml,
>> +        }[kwargs['type']]
>> +    return getPoolXml(kwargs)
>>
>> -        kwargs.update({'devices': devices})
>> -        kwargs.update({'path': path})
>> -    return xml % kwargs
>>
>>   def _get_volume_xml(**kwargs):
>>       # Required parameters
> 


-- 
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397