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

[Aline Manera]

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'>

> +      <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.

> +    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