]> review.fuel-infra Code Review - openstack-build/cinder-build.git/commitdiff
Use immutable default values for args
authorIvan Kolodyazhny <e0ne@e0ne.info>
Mon, 14 Jul 2014 21:13:54 +0000 (00:13 +0300)
committerIvan Kolodyazhny <e0ne@e0ne.info>
Wed, 16 Jul 2014 06:42:08 +0000 (09:42 +0300)
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

14 files changed:
cinder/api/xmlutil.py
cinder/hacking/checks.py
cinder/scheduler/driver.py
cinder/scheduler/filter_scheduler.py
cinder/tests/api/v2/stubs.py
cinder/tests/brick/test_brick_connector.py
cinder/tests/test_coraid.py
cinder/tests/test_vmware_vmdk.py
cinder/tests/test_volume_utils.py
cinder/transfer/api.py
cinder/volume/drivers/netapp/utils.py
cinder/volume/drivers/vmware/vmdk.py
cinder/volume/qos_specs.py
cinder/volume/volume_types.py

index cf96968d6f0f8f1d1109a61eff8625032f397ed3..e37452be07ef9cc69246a601d1ce6536b064838d 100644 (file)
@@ -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)
 
index 9a113a28daad7745063d0ef49d1f8a4cb1ae4242..4e80e4961ecdb49f6e98b204ba10f457304109be 100644 (file)
@@ -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)
index b64f776467f065407c190a531c934164545bea59..289dde69fbbddb638b8c414b291729745b3f8cf6 100644 (file)
@@ -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"))
index 7af95d5b8263f2950a05e7902a6ddff3c17fc04d..0382a3bc7f0a6324c9e4113a8ad45464228298dc 100644 (file)
@@ -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
index 0008c8d0a28f6170310cb52b97aebc2fdbcdeb10..7b0d631ae0e2ac472e391998dcdb8aaad38fb396 100644 (file)
@@ -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')]
 
 
index 3728a00b6f69cbe63c2d2ae247ecac3c7f98b7c3..0c88410ad070c754d50e552fd88d444dec030449 100644 (file)
@@ -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)
index f93a232581f1cf8aee97741da9a56ba242af989b..63edb9581bdeb87ade6c75eb3415f5ecd3683ba3 100644 (file)
@@ -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')
index 08afb6d7fd329fcdeb7291e1269a9f866fcf4765..96726bf720f546963665e1044cfa9e200aaee874 100644 (file)
@@ -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):
index af5c590023875e1a07391c78851f1d443d74e5fb..a2fe384279019c6158575b6525d79243541e61fb 100644 (file)
@@ -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
index 1ec533aa5dbc0cf8f3c314fd32c0276a177c3b4e..ac1b716e75b0aaec7ee7fbdb22b81cc99b60b9ed 100644 (file)
@@ -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)
index c4d1836565f87d51cbd65a5ee918373f85c55f24..6410c4f5f3eb1dc1ecbfb5791c9db4fa9e7d5740 100644 (file)
@@ -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()
index 4ced19cfcc087ca96c9b70876d9f98b97e9f1acd..f1201e1cce9eedd27305c8cacd10777c7bb1dcb9 100644 (file)
@@ -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
index be60dd648e4671baf73fe64e0632993683496121..8c7fa000a62a7d66a3b363e3de73101fccaada91 100644 (file)
@@ -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:
index 42c92170f9c65783296f08d4e924551828ef0d76..2d550ad0e6516d50a04b2262f70c81528db82cff 100644 (file)
@@ -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: