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

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(a)linux.vnet.ibm.com&gt; > --- > 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