From 1cfb7da7ce3f93765ba2e267897b5b7825a0ba75 Mon Sep 17 00:00:00 2001 From: Ivan Kolodyazhny Date: Tue, 15 Jul 2014 00:13:54 +0300 Subject: [PATCH] Use immutable default values for args Default mutable values (e.g. arg1=[], arg2={}) could have side effects in Python. So using None as a default value is safer. Added hacking checks for default mutable args. Closes: bug #1327473 Change-Id: I7055e534b91df794550de6c3b243324e582d4430 --- cinder/api/xmlutil.py | 3 ++- cinder/hacking/checks.py | 9 +++++++++ cinder/scheduler/driver.py | 2 +- cinder/scheduler/filter_scheduler.py | 3 ++- cinder/tests/api/v2/stubs.py | 3 ++- cinder/tests/brick/test_brick_connector.py | 3 ++- cinder/tests/test_coraid.py | 2 +- cinder/tests/test_vmware_vmdk.py | 4 ++-- cinder/tests/test_volume_utils.py | 3 ++- cinder/transfer/api.py | 3 ++- cinder/volume/drivers/netapp/utils.py | 3 ++- cinder/volume/drivers/vmware/vmdk.py | 7 ++++--- cinder/volume/qos_specs.py | 3 ++- cinder/volume/volume_types.py | 6 ++++-- 14 files changed, 37 insertions(+), 17 deletions(-) diff --git a/cinder/api/xmlutil.py b/cinder/api/xmlutil.py index cf96968d6..e37452be0 100644 --- a/cinder/api/xmlutil.py +++ b/cinder/api/xmlutil.py @@ -430,7 +430,7 @@ class TemplateElement(object): # We have fully rendered the element; return it return rootelem - def render(self, parent, obj, patches=[], nsmap=None): + def render(self, parent, obj, patches=None, nsmap=None): """Render an object. Renders an object against this template node. Returns a list @@ -447,6 +447,7 @@ class TemplateElement(object): the etree.Element instances. """ + patches = patches or [] # First, get the datum we're rendering data = None if obj is None else self.selector(obj) diff --git a/cinder/hacking/checks.py b/cinder/hacking/checks.py index 9a113a28d..4e80e4961 100644 --- a/cinder/hacking/checks.py +++ b/cinder/hacking/checks.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import re """ Guidelines for writing new hacking checks @@ -45,5 +46,13 @@ def no_translate_debug_logs(logical_line, filename): yield(0, "N319 Don't translate debug level logs") +def no_mutable_default_args(logical_line): + msg = "N322: Method's default argument shouldn't be mutable!" + mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") + if mutable_default_args.match(logical_line): + yield (0, msg) + + def factory(register): register(no_translate_debug_logs) + register(no_mutable_default_args) diff --git a/cinder/scheduler/driver.py b/cinder/scheduler/driver.py index b64f77646..289dde69f 100644 --- a/cinder/scheduler/driver.py +++ b/cinder/scheduler/driver.py @@ -68,7 +68,7 @@ class Scheduler(object): """Check if the specified host passes the filters.""" raise NotImplementedError(_("Must implement host_passes_filters")) - def find_retype_host(self, context, request_spec, filter_properties={}, + def find_retype_host(self, context, request_spec, filter_properties=None, migration_policy='never'): """Find a host that can accept the volume with its new type.""" raise NotImplementedError(_("Must implement find_retype_host")) diff --git a/cinder/scheduler/filter_scheduler.py b/cinder/scheduler/filter_scheduler.py index 7af95d5b8..0382a3bc7 100644 --- a/cinder/scheduler/filter_scheduler.py +++ b/cinder/scheduler/filter_scheduler.py @@ -99,9 +99,10 @@ class FilterScheduler(driver.Scheduler): % {'id': request_spec['volume_id'], 'host': host}) raise exception.NoValidHost(reason=msg) - def find_retype_host(self, context, request_spec, filter_properties={}, + def find_retype_host(self, context, request_spec, filter_properties=None, migration_policy='never'): """Find a host that can accept the volume with its new type.""" + filter_properties = filter_properties or {} current_host = request_spec['volume_properties']['host'] # The volume already exists on this host, and so we shouldn't check if diff --git a/cinder/tests/api/v2/stubs.py b/cinder/tests/api/v2/stubs.py index 0008c8d0a..7b0d631ae 100644 --- a/cinder/tests/api/v2/stubs.py +++ b/cinder/tests/api/v2/stubs.py @@ -119,8 +119,9 @@ def stub_volume_get_all(context, search_opts=None, marker=None, limit=None, def stub_volume_get_all_by_project(self, context, marker, limit, sort_key, - sort_dir, filters={}, + sort_dir, filters=None, viewable_admin_meta=False): + filters = filters or {} return [stub_volume_get(self, context, '1')] diff --git a/cinder/tests/brick/test_brick_connector.py b/cinder/tests/brick/test_brick_connector.py index 3728a00b6..0c88410ad 100644 --- a/cinder/tests/brick/test_brick_connector.py +++ b/cinder/tests/brick/test_brick_connector.py @@ -473,7 +473,8 @@ class AoEConnectorTestCase(ConnectorTestCase): 'FixedIntervalLoopingCall', FakeFixedIntervalLoopingCall) - def _mock_path_exists(self, aoe_path, mock_values=[]): + def _mock_path_exists(self, aoe_path, mock_values=None): + mock_values = mock_values or [] self.mox.StubOutWithMock(os.path, 'exists') for value in mock_values: os.path.exists(aoe_path).AndReturn(value) diff --git a/cinder/tests/test_coraid.py b/cinder/tests/test_coraid.py index f93a23258..63edb9581 100644 --- a/cinder/tests/test_coraid.py +++ b/cinder/tests/test_coraid.py @@ -250,7 +250,7 @@ class CoraidDriverTestCase(test.TestCase): self.driver = coraid.CoraidDriver(configuration=configuration) self.driver.do_setup({}) - def mock_volume_types(self, repositories=[]): + def mock_volume_types(self, repositories=None): if not repositories: repositories = [fake_repository_name] self.mox.StubOutWithMock(volume_types, 'get_volume_type_extra_specs') diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py index 08afb6d7f..96726bf72 100644 --- a/cinder/tests/test_vmware_vmdk.py +++ b/cinder/tests/test_vmware_vmdk.py @@ -89,8 +89,8 @@ class FakeObject(object): class FakeManagedObjectReference(object): - def __init__(self, lis=[]): - self.ManagedObjectReference = lis + def __init__(self, lis=None): + self.ManagedObjectReference = lis or [] class FakeDatastoreSummary(object): diff --git a/cinder/tests/test_volume_utils.py b/cinder/tests/test_volume_utils.py index af5c59002..a2fe38427 100644 --- a/cinder/tests/test_volume_utils.py +++ b/cinder/tests/test_volume_utils.py @@ -57,8 +57,9 @@ class UsageInfoTestCase(test.TestCase): self.volume_size = 0 self.context = context.RequestContext(self.user_id, self.project_id) - def _create_volume(self, params={}): + def _create_volume(self, params=None): """Create a test volume.""" + params = params or {} vol = {} vol['snapshot_id'] = self.snapshot_id vol['user_id'] = self.user_id diff --git a/cinder/transfer/api.py b/cinder/transfer/api.py index 1ec533aa5..ac1b716e7 100644 --- a/cinder/transfer/api.py +++ b/cinder/transfer/api.py @@ -68,7 +68,8 @@ class API(base.Base): LOG.error(msg) self.db.transfer_destroy(context, transfer_id) - def get_all(self, context, filters={}): + def get_all(self, context, filters=None): + filters = filters or {} volume_api.check_policy(context, 'get_all_transfers') if context.is_admin and 'all_tenants' in filters: transfers = self.db.transfer_get_all(context) diff --git a/cinder/volume/drivers/netapp/utils.py b/cinder/volume/drivers/netapp/utils.py index c4d183656..6410c4f5f 100644 --- a/cinder/volume/drivers/netapp/utils.py +++ b/cinder/volume/drivers/netapp/utils.py @@ -266,12 +266,13 @@ def get_volume_extra_specs(volume): return specs -def check_apis_on_cluster(na_server, api_list=[]): +def check_apis_on_cluster(na_server, api_list=None): """Checks api availability and permissions on cluster. Checks api availability and permissions for executing user. Returns a list of failed apis. """ + api_list = api_list or [] failed_apis = [] if api_list: api_version = na_server.get_api_version() diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index 4ced19cfc..f1201e1cc 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -438,7 +438,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): profile_id = profile.uniqueId return profile_id - def _create_backing(self, volume, host, create_params={}): + def _create_backing(self, volume, host, create_params=None): """Create volume backing under the given host. :param volume: Volume object @@ -447,6 +447,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): backing VM creation :return: Reference to the created backing """ + create_params = create_params or {} # Get datastores and resource pool of the host (datastores, resource_pool) = self.volumeops.get_dss_rp(host) # Pick a folder and datastore to create the volume backing on @@ -524,7 +525,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): LOG.error(msg) raise error_util.VimException(msg) - def _create_backing_in_inventory(self, volume, create_params={}): + def _create_backing_in_inventory(self, volume, create_params=None): """Creates backing under any suitable host. The method tries to pick datastore that can fit the volume under @@ -535,7 +536,7 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): backing VM creation :return: Reference to the created backing """ - + create_params = create_params or {} retrv_result = self.volumeops.get_hosts() while retrv_result: hosts = retrv_result.objects diff --git a/cinder/volume/qos_specs.py b/cinder/volume/qos_specs.py index be60dd648..8c7fa000a 100644 --- a/cinder/volume/qos_specs.py +++ b/cinder/volume/qos_specs.py @@ -228,12 +228,13 @@ def disassociate_all(context, specs_id): type_id=None) -def get_all_specs(context, inactive=False, search_opts={}): +def get_all_specs(context, inactive=False, search_opts=None): """Get all non-deleted qos specs. Pass inactive=True as argument and deleted volume types would return as well. """ + search_opts = search_opts or {} qos_specs = db.qos_specs_get_all(context, inactive) if search_opts: diff --git a/cinder/volume/volume_types.py b/cinder/volume/volume_types.py index 42c92170f..2d550ad0e 100644 --- a/cinder/volume/volume_types.py +++ b/cinder/volume/volume_types.py @@ -33,8 +33,9 @@ CONF = cfg.CONF LOG = logging.getLogger(__name__) -def create(context, name, extra_specs={}): +def create(context, name, extra_specs=None): """Creates volume types.""" + extra_specs = extra_specs or {} try: type_ref = db.volume_type_create(context, dict(name=name, @@ -55,12 +56,13 @@ def destroy(context, id): db.volume_type_destroy(context, id) -def get_all_types(context, inactive=0, search_opts={}): +def get_all_types(context, inactive=0, search_opts=None): """Get all non-deleted volume_types. Pass true as argument if you want deleted volume types returned also. """ + search_opts = search_opts or {} vol_types = db.volume_type_get_all(context, inactive) if search_opts: -- 2.45.2