[Kimchi-devel] [PATCH 7/8] Clone virtual machines

[Aline Manera]

On 11/04/2014 11:10 AM, Crístian Viana wrote:
> On 03-11-2014 14:47, Aline Manera wrote:+ if orig_pool['type'] in 
> ['dir', 'netfs', 'logical']:
>>> +                # if a volume in a pool 'dir', 'netfs' or 'logical' 
>>> cannot hold
>>> +                # a new volume with the same size, the pool 
>>> 'default' should
>>> +                # be used
>>> +                if orig_vol['capacity'] > orig_pool['available']:
>>> +                    kimchi_log.warning('storage pool \'%s\' 
>>> doesn\'t have '
>>> +                                       'enough free space to store 
>>> image '
>>> +                                       '\'%s\'; falling back to 
>>> \'default\'',
>>> +                                       orig_pool_name, path)
>>
>>> +                    new_pool_name = u'default'
>>> +                    new_pool = self.storagepool.lookup(u'default')
>>> +
>>> +                    # ...and if even the pool 'default' cannot hold 
>>> a new
>>> +                    # volume, raise an exception
>>> +                    if orig_vol['capacity'] > new_pool['available']:
>>> +                        domain_name = xpath_get_text(xml, 
>>> XPATH_DOMAIN_NAME)[0]
>>> +                        raise InvalidOperation('KCHVM0034E',
>>> +                                               {'name': domain_name})
>>
>> You do this same verification some lines below.
>> Couldn't you reorganize the code to only have one occurrence of it? 
>> Or in last case, create a function.
>
> I don't think it's worth it. Even though those "if" blocks look the 
> same, the only thing they use different variables in the boolean 
> condition and they have different bodies. There's not much to reuse on 
> that code.
>
> I can go from this:
>
> if orig_vol['capacity'] > orig_pool['available']:
>   kimchi_log.warning('storage pool \'%s\' doesn\'t have enough free 
> space to store image \'%s\'; falling back to \'default\'', 
> orig_pool_name, path)
>   new_pool_name = u'default'
>   new_pool = self.storagepool.lookup(u'default')
>
>   if orig_vol['capacity'] > new_pool['available']:
>     domain_name = xpath_get_text(xml, XPATH_DOMAIN_NAME)[0]
>     raise InvalidOperation('KCHVM0034E', {'name': domain_name})
>
> to this:
>
> @staticmethod
> def vol_fits_in_pool(vol, pool):
>     return vol['capacity'] > pool['available']
>
> if vol_fits_in_pool(orig_vol, orig_pool):
>   kimchi_log.warning('storage pool \'%s\' doesn\'t have enough free 
> space to store image \'%s\'; falling back to \'default\'', 
> orig_pool_name, path)
>   new_pool_name = u'default'
>   new_pool = self.storagepool.lookup(u'default')
>
>   if vol_fits_in_pool(orig_vol, new_pool):
>     domain_name = xpath_get_text(xml, XPATH_DOMAIN_NAME)[0]
>     raise InvalidOperation('KCHVM0034E', {'name': domain_name})
>
> Is it really worth it?

Nope! Keep it is as it is! =)