[Kimchi-devel] [PATCH 3/3] issue #543: Generate unique vol names when downloading files with same name

[Royce Lv]

On 2014年12月10日 11:17, Crístian Viana wrote:
> If the user downloads multiple remote files with the same name, only the
> first one will be completed; the following attempts will fail because
> a volume with the name already exists.
>
> The following commands demonstrate the issue:
>
> $ kimchi_rest /storagepools/default/storagevolumes | grep name
>      "name":"60f46fdc-8ecc-4601-90d6-b3b44d35c337-0.img",
>      "name":"bdd22e7c-7794-4bdf-8d2b-0089242c8b16-0.img",
>      "name":"c16a7e02-6dcf-48c5-83de-1bb2d8a110a8-0.img",
>
> $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}'
> {
>    "status":"running",
>    "message":"OK",
>    "id":"2",
>    "target_uri":"/storagepools/default/storagevolumes/index.html"
> }
>
> $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}'
> {
>    "reason":"KCHVOL0001E: Storage volume index.html already exists",
>    "code":"400 Bad Request",
>    "call_stack":"Traceback (most recent call last):\n  File \"/usr/lib/python2.7/site-packages/cherrypy/_cprequest.py\", line 656, in respond\n    response.body = self.handler()\n  File \"/usr/lib/python2.7/site-packages/cherrypy/lib/encoding.py\", line 188, in __call__\n    self.body = self.oldhandler(*args, **kwargs)\n  File \"/usr/lib/python2.7/site-packages/cherrypy/_cpdispatch.py\", line 34, in __call__\n    return self.callable(*self.args, **self.kwargs)\n  File \"/home/vianac/LTC/kimchi/src/kimchi/control/base.py\", line 331, in index\n    raise cherrypy.HTTPError(400, e.message)\nHTTPError: (400, u'KCHVOL0001E: Storage volume index.html already exists')\n"
>
> Detect whether the file to be downloaded will have a name collision with
> the existing volumes and generate a different name in that case.
>
> The following commands demonstrate the bug fix:
>
> $ kimchi_rest /storagepools/default/storagevolumes | grep name
>      "name":"60f46fdc-8ecc-4601-90d6-b3b44d35c337-0.img",
>      "name":"bdd22e7c-7794-4bdf-8d2b-0089242c8b16-0.img",
>      "name":"c16a7e02-6dcf-48c5-83de-1bb2d8a110a8-0.img",
>
> $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}'
> {
>    "status":"running",
>    "message":"OK",
>    "id":"1",
>    "target_uri":"/storagepools/default/storagevolumes/index.html"
> }
>
> $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}'
> {
>    "status":"running",
>    "message":"OK",
>    "id":"2",
>    "target_uri":"/storagepools/default/storagevolumes/index-1.html"
> }
>
> $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}'
> {
>    "status":"running",
>    "message":"OK",
>    "id":"3",
>    "target_uri":"/storagepools/default/storagevolumes/index-2.html"
>
> Fix issue #543 ("Download a volume with same basename will result
> error").
>
> Signed-off-by: Crístian Viana <vianac at linux.vnet.ibm.com>
> ---
>   src/kimchi/model/storagevolumes.py | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py
> index 406b38b..0b8bc15 100644
> --- a/src/kimchi/model/storagevolumes.py
> +++ b/src/kimchi/model/storagevolumes.py
> @@ -76,6 +76,8 @@ class StorageVolumesModel(object):
>               except:
>                   raise InvalidParameter('KCHVOL0022E', {'url': url})
>   
> +        all_vol_names = self.get_list(pool_name)
> +
>           if name is None:
>               # the methods listed in 'REQUIRE_NAME_PARAMS' cannot have
>               # 'name' == None
> @@ -88,7 +90,13 @@ class StorageVolumesModel(object):
>               if create_param == 'file':
>                   name = os.path.basename(params['file'].filename)
>               elif create_param == 'url':
> -                name = os.path.basename(params['url'])
> +                basename = os.path.basename(params['url'])
> +                if basename in all_vol_names:
> +                    prefix, suffix = os.path.splitext(basename)
For filename like "imgzip.tar.gz", we will get prefix for "imgzip.tar" 
and suffix ".gz"
then name will be "imgzip.tar-clone-1.gz", because some image disk image 
may be compressed for download.

Besides, I suggest we put the split together with name generation as all 
the split seems all the same.
> +                    name = get_next_clone_name(all_vol_names,
> +                                               prefix, suffix, '')
> +                else:
> +                    name = basename
>               else:
>                   name = 'upload-%s' % int(time.time())
>               params['name'] = name
> @@ -106,7 +114,7 @@ class StorageVolumesModel(object):
>           if pool_info['state'] == 'inactive':
>               raise InvalidParameter('KCHVOL0003E', {'pool': pool_name,
>                                                      'volume': name})
> -        if name in self.get_list(pool_name):
> +        if name in all_vol_names:
>               raise InvalidParameter('KCHVOL0001E', {'name': name})
>   
>           params['pool'] = pool_name