From 1cfb7da7ce3f93765ba2e267897b5b7825a0ba75 Mon Sep 17 00:00:00 2001
From: Ivan Kolodyazhny <e0ne@e0ne.info>
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