[PATCH V2 0/2] Storage pool support unicode correctly
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> V1 -> V2 rebase add test case ShaoHe Feng (2): Storage pool support unicode correctly update test caset for storage pool support unicode src/kimchi/model/libvirtstoragepool.py | 8 ++++---- src/kimchi/model/storagepools.py | 8 ++++---- src/kimchi/model/templates.py | 6 +++--- tests/test_model.py | 4 ++-- tests/test_rest.py | 10 ++++++---- 5 files changed, 19 insertions(+), 17 deletions(-) -- 1.8.4.2
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> For unicode, we have handle the incoming text from kimchi server and outgoing text to kimchi server correctly. But we do not handle the incoming text from libvirt and outgoing text to libvirt correctly. For some variables of storage pool, such as pool name and pool xml do not follow our principle. https://github.com/kimchi-project/kimchi/wiki/support-unicode This patch will improve the follow: 1. Handle storage pool xml and pool name as unicode in kimchi, and decode them when pass them to libvirt. 2. Decode the pool name from libvirt. We should guarantee: All text strings, everywhere should be of type unicode, not str. If we're handling text, and our variable is a str, it's a bug! Test this patch: 1. create a pool $ curl -u <user> -H 'Accept: application/json' \ -H 'Content-type: application/json' \ http://localhost:8000/storagepools/ -X POST -d ' { "name":"kīмсhī-pool", "type":"dir", "path":"/tmp/test" } ' 2. get this pool $ curl -u <user> -H 'Accept: application/json' \ -H 'Content-type: application/json' \ http://localhost:8000/storagepools/kīмсhī-pool 3. delete this pool $ curl -u <user> -H 'Accept: application/json' \ -H 'Content-type: application/json' -X DELETE \ http://localhost:8000/storagepools/kīмсhī-pool Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/model/libvirtstoragepool.py | 8 ++++---- src/kimchi/model/storagepools.py | 8 ++++---- src/kimchi/model/templates.py | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/kimchi/model/libvirtstoragepool.py b/src/kimchi/model/libvirtstoragepool.py index f4dbf2e..9c8027f 100644 --- a/src/kimchi/model/libvirtstoragepool.py +++ b/src/kimchi/model/libvirtstoragepool.py @@ -68,7 +68,7 @@ class DirPoolDef(StoragePoolDef): # name: # type: # path: - xml = """ + xml = u""" <pool type='dir'> <name>{name}</name> <target> @@ -125,7 +125,7 @@ class NetfsPoolDef(StoragePoolDef): # source[path]: poolArgs = copy.deepcopy(self.poolArgs) poolArgs['path'] = self.path - xml = """ + xml = u""" <pool type='netfs'> <name>{name}</name> <source> @@ -161,7 +161,7 @@ class LogicalPoolDef(StoragePoolDef): poolArgs['source']['devices'] = ''.join(devices) poolArgs['path'] = self.path - xml = """ + xml = u""" <pool type='logical'> <name>{name}</name> <source> @@ -241,7 +241,7 @@ class IscsiPoolDef(StoragePoolDef): 'auth': self._format_auth(poolArgs)}) poolArgs['path'] = '/dev/disk/by-id' - xml = """ + xml = u""" <pool type='iscsi'> <name>{name}</name> <source> diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index 233a8a7..53eac64 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -53,7 +53,7 @@ class StoragePoolsModel(object): conn = self.conn.get() names = conn.listStoragePools() names += conn.listDefinedStoragePools() - return sorted(names) + return sorted(map(lambda x: x.decode('utf-8'), names)) except libvirt.libvirtError as e: raise OperationFailed(e.get_error_message()) @@ -69,7 +69,7 @@ class StoragePoolsModel(object): task_id = self._do_deep_scan(params) poolDef = StoragePoolDef.create(params) poolDef.prepare(conn) - xml = poolDef.xml + xml = poolDef.xml.encode("utf-8") except KeyError, key: raise MissingParameter(key) @@ -100,7 +100,7 @@ class StoragePoolsModel(object): def _clean_scan(self, pool_name): try: conn = self.conn.get() - pool = conn.storagePoolLookupByName(pool_name) + pool = conn.storagePoolLookupByName(pool_name.encode("utf-8")) pool.destroy() with self.objstore as session: session.delete('scanning', pool_name) @@ -141,7 +141,7 @@ class StoragePoolModel(object): def get_storagepool(name, conn): conn = conn.get() try: - return conn.storagePoolLookupByName(name) + return conn.storagePoolLookupByName(name.encode("utf-8")) except libvirt.libvirtError as e: if e.get_error_code() == libvirt.VIR_ERR_NO_STORAGE_POOL: raise NotFoundError("Storage Pool '%s' not found" % name) diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index 03632a6..a5c73cc 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -43,7 +43,7 @@ class TemplatesModel(object): if pool_uri: pool_name = pool_name_from_uri(pool_uri) try: - conn.storagePoolLookupByName(pool_name) + conn.storagePoolLookupByName(pool_name.encode("utf-8")) except Exception as e: err = "Storagepool specified is not valid: %s." raise InvalidParameter(err % e.message) @@ -99,7 +99,7 @@ class TemplateModel(object): pool_name = pool_name_from_uri(pool_uri) try: conn = self.conn.get() - conn.storagePoolLookupByName(pool_name) + conn.storagePoolLookupByName(pool_name.encode("utf-8")) except Exception as e: err = "Storagepool specified is not valid: %s." raise InvalidParameter(err % e.message) @@ -131,7 +131,7 @@ class LibvirtVMTemplate(VMTemplate): pool_name = pool_name_from_uri(pool_uri) try: conn = self.conn.get() - pool = conn.storagePoolLookupByName(pool_name) + pool = conn.storagePoolLookupByName(pool_name.encode("utf-8")) except libvirt.libvirtError: err = 'Storage specified by template does not exist' raise InvalidParameter(err) -- 1.8.4.2
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 02/10/2014 04:28 AM, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
For unicode, we have handle the incoming text from kimchi server and outgoing text to kimchi server correctly.
But we do not handle the incoming text from libvirt and outgoing text to libvirt correctly.
For some variables of storage pool, such as pool name and pool xml do not follow our principle. https://github.com/kimchi-project/kimchi/wiki/support-unicode
This patch will improve the follow: 1. Handle storage pool xml and pool name as unicode in kimchi, and decode them when pass them to libvirt. 2. Decode the pool name from libvirt.
We should guarantee: All text strings, everywhere should be of type unicode, not str. If we're handling text, and our variable is a str, it's a bug!
Test this patch: 1. create a pool $ curl -u <user> -H 'Accept: application/json' \ -H 'Content-type: application/json' \ http://localhost:8000/storagepools/ -X POST -d ' { "name":"kīмсhī-pool", "type":"dir", "path":"/tmp/test" } '
2. get this pool $ curl -u <user> -H 'Accept: application/json' \ -H 'Content-type: application/json' \ http://localhost:8000/storagepools/kīмсhī-pool
3. delete this pool $ curl -u <user> -H 'Accept: application/json' \ -H 'Content-type: application/json' -X DELETE \ http://localhost:8000/storagepools/kīмсhī-pool
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/model/libvirtstoragepool.py | 8 ++++---- src/kimchi/model/storagepools.py | 8 ++++---- src/kimchi/model/templates.py | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/kimchi/model/libvirtstoragepool.py b/src/kimchi/model/libvirtstoragepool.py index f4dbf2e..9c8027f 100644 --- a/src/kimchi/model/libvirtstoragepool.py +++ b/src/kimchi/model/libvirtstoragepool.py @@ -68,7 +68,7 @@ class DirPoolDef(StoragePoolDef): # name: # type: # path: - xml = """ + xml = u""" <pool type='dir'> <name>{name}</name> <target> @@ -125,7 +125,7 @@ class NetfsPoolDef(StoragePoolDef): # source[path]: poolArgs = copy.deepcopy(self.poolArgs) poolArgs['path'] = self.path - xml = """ + xml = u""" <pool type='netfs'> <name>{name}</name> <source> @@ -161,7 +161,7 @@ class LogicalPoolDef(StoragePoolDef): poolArgs['source']['devices'] = ''.join(devices) poolArgs['path'] = self.path
- xml = """ + xml = u""" <pool type='logical'> <name>{name}</name> <source> @@ -241,7 +241,7 @@ class IscsiPoolDef(StoragePoolDef): 'auth': self._format_auth(poolArgs)}) poolArgs['path'] = '/dev/disk/by-id'
- xml = """ + xml = u""" <pool type='iscsi'> <name>{name}</name> <source> diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index 233a8a7..53eac64 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -53,7 +53,7 @@ class StoragePoolsModel(object): conn = self.conn.get() names = conn.listStoragePools() names += conn.listDefinedStoragePools() - return sorted(names) + return sorted(map(lambda x: x.decode('utf-8'), names)) except libvirt.libvirtError as e: raise OperationFailed(e.get_error_message())
@@ -69,7 +69,7 @@ class StoragePoolsModel(object): task_id = self._do_deep_scan(params) poolDef = StoragePoolDef.create(params) poolDef.prepare(conn) - xml = poolDef.xml + xml = poolDef.xml.encode("utf-8") except KeyError, key: raise MissingParameter(key)
@@ -100,7 +100,7 @@ class StoragePoolsModel(object): def _clean_scan(self, pool_name): try: conn = self.conn.get() - pool = conn.storagePoolLookupByName(pool_name) + pool = conn.storagePoolLookupByName(pool_name.encode("utf-8")) pool.destroy() with self.objstore as session: session.delete('scanning', pool_name) @@ -141,7 +141,7 @@ class StoragePoolModel(object): def get_storagepool(name, conn): conn = conn.get() try: - return conn.storagePoolLookupByName(name) + return conn.storagePoolLookupByName(name.encode("utf-8")) except libvirt.libvirtError as e: if e.get_error_code() == libvirt.VIR_ERR_NO_STORAGE_POOL: raise NotFoundError("Storage Pool '%s' not found" % name) diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index 03632a6..a5c73cc 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -43,7 +43,7 @@ class TemplatesModel(object): if pool_uri: pool_name = pool_name_from_uri(pool_uri) try: - conn.storagePoolLookupByName(pool_name) + conn.storagePoolLookupByName(pool_name.encode("utf-8")) except Exception as e: err = "Storagepool specified is not valid: %s." raise InvalidParameter(err % e.message) @@ -99,7 +99,7 @@ class TemplateModel(object): pool_name = pool_name_from_uri(pool_uri) try: conn = self.conn.get() - conn.storagePoolLookupByName(pool_name) + conn.storagePoolLookupByName(pool_name.encode("utf-8")) except Exception as e: err = "Storagepool specified is not valid: %s." raise InvalidParameter(err % e.message) @@ -131,7 +131,7 @@ class LibvirtVMTemplate(VMTemplate): pool_name = pool_name_from_uri(pool_uri) try: conn = self.conn.get() - pool = conn.storagePoolLookupByName(pool_name) + pool = conn.storagePoolLookupByName(pool_name.encode("utf-8")) except libvirt.libvirtError: err = 'Storage specified by template does not exist' raise InvalidParameter(err)
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> update test case for it to avoid regression. Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- tests/test_model.py | 4 ++-- tests/test_rest.py | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index 4a6d2fe..fe754da 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -186,10 +186,10 @@ class ModelTests(unittest.TestCase): poolDefs = [ {'type': 'dir', - 'name': 'unitTestDirPool', + 'name': u'kīмсhīUnitTestDirPool', 'path': '/tmp/kimchi-images'}, {'type': 'iscsi', - 'name': 'unitTestISCSIPool', + 'name': u'kīмсhīUnitTestISCSIPool', 'source': {'host': '127.0.0.1', 'target': 'iqn.2013-12.localhost.kimchiUnitTest'}}] diff --git a/tests/test_rest.py b/tests/test_rest.py index 0ed293b..c4e92bf 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- # # Project Kimchi # @@ -588,7 +589,7 @@ class RestTests(unittest.TestCase): # Now add a couple of StoragePools to the mock model for i in xrange(5): - name = 'storagepool-%i' % i + name = 'kīмсhī-storagepool-%i' % i req = json.dumps({'name': name, 'capacity': 1024, 'allocated': 512, @@ -597,7 +598,7 @@ class RestTests(unittest.TestCase): resp = self.request('/storagepools', req, 'POST') self.assertEquals(201, resp.status) - req = json.dumps({'name': 'storagepool-1', + req = json.dumps({'name': 'kīмсhī-storagepool-1', 'capacity': 1024, 'allocated': 512, 'path': '/var/lib/libvirt/images/%i' % i, @@ -617,9 +618,10 @@ class RestTests(unittest.TestCase): storagepools = json.loads(self.request('/storagepools').read()) self.assertEquals(7, len(storagepools)) - resp = self.request('/storagepools/storagepool-1') + resp = self.request('/storagepools/kīмсhī-storagepool-1') storagepool = json.loads(resp.read()) - self.assertEquals('storagepool-1', storagepool['name']) + self.assertEquals('kīмсhī-storagepool-1', + storagepool['name'].encode("utf-8")) self.assertEquals('inactive', storagepool['state']) self.assertIn('source', storagepool) -- 1.8.4.2
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 02/10/2014 04:28 AM, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
update test case for it to avoid regression.
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- tests/test_model.py | 4 ++-- tests/test_rest.py | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index 4a6d2fe..fe754da 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -186,10 +186,10 @@ class ModelTests(unittest.TestCase):
poolDefs = [ {'type': 'dir', - 'name': 'unitTestDirPool', + 'name': u'kīмсhīUnitTestDirPool', 'path': '/tmp/kimchi-images'}, {'type': 'iscsi', - 'name': 'unitTestISCSIPool', + 'name': u'kīмсhīUnitTestISCSIPool', 'source': {'host': '127.0.0.1', 'target': 'iqn.2013-12.localhost.kimchiUnitTest'}}]
diff --git a/tests/test_rest.py b/tests/test_rest.py index 0ed293b..c4e92bf 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- # # Project Kimchi # @@ -588,7 +589,7 @@ class RestTests(unittest.TestCase):
# Now add a couple of StoragePools to the mock model for i in xrange(5): - name = 'storagepool-%i' % i + name = 'kīмсhī-storagepool-%i' % i req = json.dumps({'name': name, 'capacity': 1024, 'allocated': 512, @@ -597,7 +598,7 @@ class RestTests(unittest.TestCase): resp = self.request('/storagepools', req, 'POST') self.assertEquals(201, resp.status)
- req = json.dumps({'name': 'storagepool-1', + req = json.dumps({'name': 'kīмсhī-storagepool-1', 'capacity': 1024, 'allocated': 512, 'path': '/var/lib/libvirt/images/%i' % i, @@ -617,9 +618,10 @@ class RestTests(unittest.TestCase): storagepools = json.loads(self.request('/storagepools').read()) self.assertEquals(7, len(storagepools))
- resp = self.request('/storagepools/storagepool-1') + resp = self.request('/storagepools/kīмсhī-storagepool-1') storagepool = json.loads(resp.read()) - self.assertEquals('storagepool-1', storagepool['name']) + self.assertEquals('kīмсhī-storagepool-1', + storagepool['name'].encode("utf-8")) self.assertEquals('inactive', storagepool['state']) self.assertIn('source', storagepool)
On Mon, 2014-02-10 at 14:28 +0800, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
update test case for it to avoid regression.
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- tests/test_model.py | 4 ++-- tests/test_rest.py | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index 4a6d2fe..fe754da 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -186,10 +186,10 @@ class ModelTests(unittest.TestCase):
poolDefs = [ {'type': 'dir', - 'name': 'unitTestDirPool', + 'name': u'kīмсhīUnitTestDirPool', 'path': '/tmp/kimchi-images'}, {'type': 'iscsi', - 'name': 'unitTestISCSIPool', + 'name': u'kīмсhīUnitTestISCSIPool', 'source': {'host': '127.0.0.1', 'target': 'iqn.2013-12.localhost.kimchiUnitTest'}}]
diff --git a/tests/test_rest.py b/tests/test_rest.py index 0ed293b..c4e92bf 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- # # Project Kimchi # @@ -588,7 +589,7 @@ class RestTests(unittest.TestCase):
# Now add a couple of StoragePools to the mock model for i in xrange(5): - name = 'storagepool-%i' % i + name = 'kīмсhī-storagepool-%i' % i I've never seen that kind of letter 'i' (ī) before. Is that intentional? Does it matter?
req = json.dumps({'name': name, 'capacity': 1024, 'allocated': 512, @@ -597,7 +598,7 @@ class RestTests(unittest.TestCase): resp = self.request('/storagepools', req, 'POST') self.assertEquals(201, resp.status)
- req = json.dumps({'name': 'storagepool-1', + req = json.dumps({'name': 'kīмсhī-storagepool-1', 'capacity': 1024, 'allocated': 512, 'path': '/var/lib/libvirt/images/%i' % i, @@ -617,9 +618,10 @@ class RestTests(unittest.TestCase): storagepools = json.loads(self.request('/storagepools').read()) self.assertEquals(7, len(storagepools))
- resp = self.request('/storagepools/storagepool-1') + resp = self.request('/storagepools/kīмсhī-storagepool-1') storagepool = json.loads(resp.read()) - self.assertEquals('storagepool-1', storagepool['name']) + self.assertEquals('kīмсhī-storagepool-1', + storagepool['name'].encode("utf-8")) self.assertEquals('inactive', storagepool['state']) self.assertIn('source', storagepool)
On 02/11/2014 08:07 AM, Christy Perez wrote:
On Mon, 2014-02-10 at 14:28 +0800, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
update test case for it to avoid regression.
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- tests/test_model.py | 4 ++-- tests/test_rest.py | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index 4a6d2fe..fe754da 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -186,10 +186,10 @@ class ModelTests(unittest.TestCase):
poolDefs = [ {'type': 'dir', - 'name': 'unitTestDirPool', + 'name': u'kīмсhīUnitTestDirPool', 'path': '/tmp/kimchi-images'}, {'type': 'iscsi', - 'name': 'unitTestISCSIPool', + 'name': u'kīмсhīUnitTestISCSIPool', 'source': {'host': '127.0.0.1', 'target': 'iqn.2013-12.localhost.kimchiUnitTest'}}]
diff --git a/tests/test_rest.py b/tests/test_rest.py index 0ed293b..c4e92bf 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- # # Project Kimchi # @@ -588,7 +589,7 @@ class RestTests(unittest.TestCase):
# Now add a couple of StoragePools to the mock model for i in xrange(5): - name = 'storagepool-%i' % i + name = 'kīмсhī-storagepool-%i' % i I've never seen that kind of letter 'i' (ī) before. Is that intentional? Does it matter?
yes, intentional. Characters in "kīмсhī" are all digraphs, is used for our test. "kīмсhī" looks like "kimchi". If I put Chinese or Portuguese here, maybe others from different counties may not know it.
req = json.dumps({'name': name, 'capacity': 1024, 'allocated': 512, @@ -597,7 +598,7 @@ class RestTests(unittest.TestCase): resp = self.request('/storagepools', req, 'POST') self.assertEquals(201, resp.status)
- req = json.dumps({'name': 'storagepool-1', + req = json.dumps({'name': 'kīмсhī-storagepool-1', 'capacity': 1024, 'allocated': 512, 'path': '/var/lib/libvirt/images/%i' % i, @@ -617,9 +618,10 @@ class RestTests(unittest.TestCase): storagepools = json.loads(self.request('/storagepools').read()) self.assertEquals(7, len(storagepools))
- resp = self.request('/storagepools/storagepool-1') + resp = self.request('/storagepools/kīмсhī-storagepool-1') storagepool = json.loads(resp.read()) - self.assertEquals('storagepool-1', storagepool['name']) + self.assertEquals('kīмсhī-storagepool-1', + storagepool['name'].encode("utf-8")) self.assertEquals('inactive', storagepool['state']) self.assertIn('source', storagepool)
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
participants (4)
-
Aline Manera -
Christy Perez -
shaohef＠linux.vnet.ibm.com -
Sheldon