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

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?