[Kimchi-devel] [PATCH v3 1/5] storagepool: refactor _get_pool_xml()

[Royce Lv]

I would say I really like this re-factory! Make this quite clear.

Minor suggestion below.
On 2013年12月27日 18:20, 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. It also performs some preparations such as create target directory.
> This is OK for now but in future we'll have more types of pool, and add
> more validation and preparation code. So it needs a little refactor
>
> In this patch we define StoragePoolDef object along with a factory
> methoed named "create". The factory method instantiates DirPoolDef,
> NetfsPoolDef or LogicalPoolDef according to the requested pool type.
> Then we can call defObj.prepare() to check NFS mount and iSCSI
> login/password. We can call defObj.get_xml() to generate XML for the
> respective poolType.
>
> v2 -> v3
>    Call pool.build() for netfs, dir and logical pool. So do not need to
>    create dirs any longer. Since NFS and iSCSI pool need validation,
>    extract a StoragePoolDef object with "prepare" and "get_xml" methods.
>
> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> ---
>   src/kimchi/model.py | 120 +++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 90 insertions(+), 30 deletions(-)
>
> diff --git a/src/kimchi/model.py b/src/kimchi/model.py
> index a6790b8..4d7f9f2 100644
> --- a/src/kimchi/model.py
> +++ b/src/kimchi/model.py
> @@ -994,7 +994,9 @@ class Model(object):
>
>               if params['type'] == 'kimchi-iso':
>                   task_id = self._do_deep_scan(params)
> -            xml = _get_pool_xml(**params)
> +            poolDef = StoragePoolDef.create(params)
> +            poolDef.prepare()
> +            xml = poolDef.get_xml()
>           except KeyError, key:
>               raise MissingParameter(key)
>
> @@ -1009,7 +1011,7 @@ class Model(object):
>                   return name
>
>               pool = conn.storagePoolDefineXML(xml, 0)
> -            if params['type'] in ['logical', 'dir']:
> +            if params['type'] in ['logical', 'dir', 'netfs']:
>                   pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW)
>                   # autostart dir and logical storage pool created from kimchi
>                   pool.setAutostart(1)
> @@ -1430,28 +1432,74 @@ class LibvirtVMScreenshot(VMScreenshot):
>           finally:
>               os.close(fd)
>
> -def _get_pool_xml(**kwargs):
> -    # Required parameters
> -    # name:
> -    # type:
> -    # path:
> -    # nfspath:
> -    if kwargs['type'] == 'dir':
> +
> +class StoragePoolDef(object):
> +    @classmethod
> +    def create(cls, poolArgs):
> +        for klass in cls.__subclasses__():
> +            if poolArgs['type'] == klass.poolType:
> +                return klass(poolArgs)
> +        raise OperationFailed('Unsupported pool type: %s' % poolArgs['type'])
> +
> +    def __init__(self, poolArgs):
> +        self.poolArgs = poolArgs
> +
> +    def prepare(self):
> +        ''' Validate pool arguments and perform preparations. Operation which
> +        would cause side effect should be put here. Subclasses can optionally
> +        override this method, or it always succeeds by default. '''
> +        pass
> +
> +    def get_xml(self):
> +        ''' Generate storage pool XML string. It's an idempotent operation and
> +        should not cause any side effect. '''
> +        return self._get_xml_imp(copy.deepcopy(self.poolArgs))
> +
> +    def _get_xml_imp(self, poolArgs):
> +        ''' Subclasses have to override this method to actually generate the
> +        storage pool XML definition. '''
> +        raise OperationFailed('get_xml is not implemented: %s' % self)
> +
> +
> +class DirPoolDef(StoragePoolDef):
> +    poolType = 'dir'
> +
> +    def _get_xml_imp(self, poolArgs):
How about change _get_xml_imp to:
@property
def pool_xml(self):
....
and delete get_xml() function, So that we can get it easier and more clear.
> +        # Required parameters
> +        # name:
> +        # type:
> +        # path:
>           xml = """
> -        <pool type='%(type)s'>
> +        <pool type='dir'>
>             <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})
> +        """ % poolArgs
> +        return xml
> +
> +
> +class NetfsPoolDef(StoragePoolDef):
> +    poolType = 'netfs'
> +
> +    def __init__(self, poolArgs):
> +        super(NetfsPoolDef, self).__init__(poolArgs)
> +        self.path = '/var/lib/kimchi/nfs_mount/' + self.poolArgs['name']
> +
> +    def prepare(self):
> +        # TODO Verify the NFS export can be actually mounted.
> +        pass
> +
> +    def _get_xml_imp(self, poolArgs):
> +        # Required parameters
> +        # name:
> +        # type:
> +        # nfsserver:
> +        # nfspath:
> +        poolArgs['path'] = self.path
>           xml = """
> -        <pool type='%(type)s'>
> +        <pool type='netfs'>
>             <name>%(name)s</name>
>             <source>
>               <host name='%(nfsserver)s'/>
> @@ -1461,13 +1509,30 @@ def _get_pool_xml(**kwargs):
>               <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)
> +        """ % poolArgs
> +        return xml
> +
> +
> +class LogicalPoolDef(StoragePoolDef):
> +    poolType = 'logical'
> +
> +    def __init__(self, poolArgs):
> +        super(LogicalPoolDef, self).__init__(poolArgs)
> +        self.path = '/var/lib/kimchi/logical_mount/' + self.poolArgs['name']
> +
> +    def _get_xml_imp(self, poolArgs):
> +        # Required parameters
> +        # name:
> +        # type:
> +        # devices:
> +        devices = []
> +        for device_path in poolArgs['devices']:
> +            devices.append('<device path="%s" />' % device_path)
> +
> +        poolArgs.update({'devices': ''.join(devices),
> +                         'path': self.path})
>           xml = """
> -        <pool type='%(type)s'>
> +        <pool type='logical'>
>           <name>%(name)s</name>
>               <source>
>               %(devices)s
> @@ -1476,14 +1541,9 @@ def _get_pool_xml(**kwargs):
>               <path>%(path)s</path>
>           </target>
>           </pool>
> -        """
> -        devices = ''
> -        for device_path in kwargs['devices']:
> -            devices += "<device path=\""+device_path+"\" />"
> +        """ % poolArgs
> +        return xml
>
> -        kwargs.update({'devices': devices})
> -        kwargs.update({'path': path})
> -    return xml % kwargs
>
>   def _get_volume_xml(**kwargs):
>       # Required parameters