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

[Crístian Viana]

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?