From 2dce8c92f6cfc4fb5d6a455fe13cbc0ed6d72882 Mon Sep 17 00:00:00 2001 From: Boris Pavlovic Date: Fri, 17 May 2013 17:38:51 +0400 Subject: [PATCH] Remove usage of locals() for formatting from nova.api.* Using of locals() for formatting string is a nasty thing because: 1) It is not so clear as using explicit dicts 2) It could produce hidden errors during refactoring 3) Changing name of variable causes change in message 4) Creating a lot of unused variables fixes bug 1171936 Change-Id: I293d7ebb875f65cce322d4938d1ae323f3aded8d --- doc/ext/nova_todo.py | 8 ++--- doc/source/devref/il8n.rst | 13 +++----- nova/api/ec2/__init__.py | 20 +++++++----- nova/api/ec2/apirequest.py | 9 +++--- nova/api/ec2/cloud.py | 31 +++++++++++-------- nova/api/metadata/handler.py | 12 ++++--- nova/api/openstack/__init__.py | 10 +++--- nova/api/openstack/common.py | 10 +++--- .../compute/contrib/admin_actions.py | 8 ++--- .../openstack/compute/contrib/aggregates.py | 28 ++++++++--------- .../openstack/compute/contrib/floating_ips.py | 2 +- nova/api/openstack/compute/contrib/hosts.py | 5 +-- nova/api/openstack/compute/contrib/volumes.py | 9 ++++-- nova/api/openstack/compute/servers.py | 7 ++--- nova/api/openstack/extensions.py | 17 +++++----- nova/api/openstack/wsgi.py | 4 ++- nova/api/openstack/xmlutil.py | 7 ++--- 17 files changed, 109 insertions(+), 91 deletions(-) diff --git a/doc/ext/nova_todo.py b/doc/ext/nova_todo.py index 979a176cf2ba..6f66e2573aba 100644 --- a/doc/ext/nova_todo.py +++ b/doc/ext/nova_todo.py @@ -37,13 +37,13 @@ def process_todo_nodes(app, doctree, fromdocname): for todo_info in env.todo_all_todos: para = nodes.paragraph() - filename = env.doc2path(todo_info['docname'], base=None) - # Create a reference newnode = nodes.reference('', '') - line_info = todo_info['lineno'] - link = _('%(filename)s, line %(line_info)d') % locals() + filename = env.doc2path(todo_info['docname'], base=None) + link = (_('%(filename)s, line %(line_info)d') % + {'filename': filename, 'line_info': todo_info['lineno']}) + innernode = nodes.emphasis(link, link) newnode['refdocname'] = todo_info['docname'] diff --git a/doc/source/devref/il8n.rst b/doc/source/devref/il8n.rst index 900ea8a28cf9..3b5ea65e9623 100644 --- a/doc/source/devref/il8n.rst +++ b/doc/source/devref/il8n.rst @@ -9,14 +9,11 @@ in a ``_()`` function call. For example:: LOG.debug(_("block_device_mapping %s"), block_device_mapping) -If you have multiple arguments, the convention is to use named parameters. -It's common to use the ``locals()`` dict (which contains the names and values -of the local variables in the current scope) to do the string interpolation. -For example:: - - label = ... - sr_ref = ... - LOG.debug(_('Introduced %(label)s as %(sr_ref)s.') % locals()) +Do not use ``locals()`` for formatting messages because: +1. It is not as clear as using explicit dicts. +2. It could produce hidden errors during refactoring. +3. Changing the name of a variable causes a change in the message. +4. It creates a lot of otherwise unused variables. If you do not follow the project conventions, your code may cause the LocalizationTestCase.test_multiple_positional_format_placeholders test to fail diff --git a/nova/api/ec2/__init__.py b/nova/api/ec2/__init__.py index 7ce18685b985..b72c7d3a8c88 100644 --- a/nova/api/ec2/__init__.py +++ b/nova/api/ec2/__init__.py @@ -78,7 +78,7 @@ CONF.import_opt('use_forwarded_for', 'nova.api.auth') def ec2_error(req, request_id, code, message): """Helper to send an ec2_compatible error.""" - LOG.error(_('%(code)s: %(message)s') % locals()) + LOG.error(_('%(code)s: %(message)s'), {'code': code, 'message': message}) resp = webob.Response() resp.status = 400 resp.headers['Content-Type'] = 'text/xml' @@ -180,11 +180,12 @@ class Lockout(wsgi.Middleware): # NOTE(vish): To use incr, failures has to be a string. self.mc.set(failures_key, '1', time=CONF.lockout_window * 60) elif failures >= CONF.lockout_attempts: - lock_mins = CONF.lockout_minutes - msg = _('Access key %(access_key)s has had %(failures)d' - ' failed authentications and will be locked out' - ' for %(lock_mins)d minutes.') % locals() - LOG.warn(msg) + LOG.warn(_('Access key %(access_key)s has had %(failures)d ' + 'failed authentications and will be locked out ' + 'for %(lock_mins)d minutes.'), + {'access_key': access_key, + 'failures': failures, + 'lock_mins': CONF.lockout_minutes}) self.mc.set(failures_key, str(failures), time=CONF.lockout_minutes * 60) return res @@ -333,7 +334,8 @@ class Requestify(wsgi.Middleware): LOG.debug(_('action: %s'), action) for key, value in args.items(): - LOG.debug(_('arg: %(key)s\t\tval: %(value)s') % locals()) + LOG.debug(_('arg: %(key)s\t\tval: %(value)s'), + {'key': key, 'value': value}) # Success! api_request = apirequest.APIRequest(self.controller, action, @@ -409,7 +411,9 @@ class Authorizer(wsgi.Middleware): return self.application else: LOG.audit(_('Unauthorized request for controller=%(controller)s ' - 'and action=%(action)s') % locals(), context=context) + 'and action=%(action)s'), + {'controller': controller, 'action': action}, + context=context) raise webob.exc.HTTPUnauthorized() def _matches_any_role(self, context, roles): diff --git a/nova/api/ec2/apirequest.py b/nova/api/ec2/apirequest.py index ca1302fad353..b43b63287594 100644 --- a/nova/api/ec2/apirequest.py +++ b/nova/api/ec2/apirequest.py @@ -57,11 +57,10 @@ class APIRequest(object): method = getattr(self.controller, ec2utils.camelcase_to_underscore(self.action)) except AttributeError: - controller = self.controller - action = self.action - _error = _('Unsupported API request: controller = %(controller)s,' - ' action = %(action)s') % locals() - LOG.exception(_error) + LOG.exception(_('Unsupported API request: controller = ' + '%(controller)s, action = %(action)s'), + {'controller': self.controller, + 'action': self.action}) # TODO(gundlach): Raise custom exception, trap in apiserver, # and reraise as 400 error. raise exception.InvalidRequest() diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py index e2abde48ca85..2e55d8f1ecce 100644 --- a/nova/api/ec2/cloud.py +++ b/nova/api/ec2/cloud.py @@ -621,8 +621,7 @@ class CloudController(object): validprotocols = ['tcp', 'udp', 'icmp', '6', '17', '1'] if 'ip_protocol' in values and \ values['ip_protocol'] not in validprotocols: - protocol = values['ip_protocol'] - err = _("Invalid IP protocol %(protocol)s.") % locals() + err = _('Invalid IP protocol %s.') % values['ip_protocol'] raise exception.EC2APIError(message=err, code="400") def revoke_security_group_ingress(self, context, group_name=None, @@ -876,9 +875,12 @@ class CloudController(object): volume_id = ec2utils.ec2_vol_id_to_uuid(volume_id) instance_uuid = ec2utils.ec2_inst_id_to_uuid(context, instance_id) instance = self.compute_api.get(context, instance_uuid) - msg = _("Attach volume %(volume_id)s to instance %(instance_id)s" - " at %(device)s") % locals() - LOG.audit(msg, context=context) + LOG.audit(_('Attach volume %(volume_id)s to instance %(instance_id)s ' + 'at %(device)s'), + {'volume_id': volume_id, + 'instance_id': instance_id, + 'device': device}, + context=context) try: self.compute_api.attach_volume(context, instance, @@ -1238,7 +1240,7 @@ class CloudController(object): return {'publicIp': public_ip} def release_address(self, context, public_ip, **kwargs): - LOG.audit(_("Release address %s"), public_ip, context=context) + LOG.audit(_('Release address %s'), public_ip, context=context) try: self.network_api.release_floating_ip(context, address=public_ip) return {'return': "true"} @@ -1246,8 +1248,10 @@ class CloudController(object): raise exception.EC2APIError(_('Unable to release IP Address.')) def associate_address(self, context, instance_id, public_ip, **kwargs): - LOG.audit(_("Associate address %(public_ip)s to" - " instance %(instance_id)s") % locals(), context=context) + LOG.audit(_("Associate address %(public_ip)s to instance " + "%(instance_id)s"), + {'public_ip': public_ip, 'instance_id': instance_id}, + context=context) instance_uuid = ec2utils.ec2_inst_id_to_uuid(context, instance_id) instance = self.compute_api.get(context, instance_uuid) @@ -1506,9 +1510,10 @@ class CloudController(object): metadata['properties']['block_device_mapping'] = mappings image_id = self._register_image(context, metadata) - msg = _("Registered image %(image_location)s with" - " id %(image_id)s") % locals() - LOG.audit(msg, context=context) + LOG.audit(_('Registered image %(image_location)s with id ' + '%(image_id)s'), + {'image_location': image_location, 'image_id': image_id}, + context=context) return {'imageId': image_id} def describe_image_attribute(self, context, image_id, attribute, **kwargs): @@ -1615,10 +1620,10 @@ class CloudController(object): # CreateImage only supported for the analogue of EBS-backed instances if not self.compute_api.is_volume_backed_instance(context, instance, bdms): - root = instance['root_device_name'] msg = _("Invalid value '%(ec2_instance_id)s' for instanceId. " "Instance does not have a volume attached at root " - "(%(root)s)") % locals() + "(%(root)s)") % {'root': instance['root_device_name'], + 'ec2_instance_id': ec2_instance_id} raise exception.InvalidParameterValue(err=msg) # stop the instance if necessary diff --git a/nova/api/metadata/handler.py b/nova/api/metadata/handler.py index bbaeba524949..1eb30ee4ad7e 100644 --- a/nova/api/metadata/handler.py +++ b/nova/api/metadata/handler.py @@ -166,10 +166,14 @@ class MetadataRequestHandler(wsgi.Application): if expected_signature != signature: if instance_id: - w = _('X-Instance-ID-Signature: %(signature)s does not match ' - 'the expected value: %(expected_signature)s for id: ' - '%(instance_id)s. Request From: %(remote_address)s') - LOG.warn(w % locals()) + LOG.warn(_('X-Instance-ID-Signature: %(signature)s does not ' + 'match the expected value: %(expected_signature)s ' + 'for id: %(instance_id)s. Request From: ' + '%(remote_address)s'), + {'signature': signature, + 'expected_signature': expected_signature, + 'instance_id': instance_id, + 'remote_address': remote_address}) msg = _('Invalid proxy request signature.') raise webob.exc.HTTPForbidden(explanation=msg) diff --git a/nova/api/openstack/__init__.py b/nova/api/openstack/__init__.py index cc276234bb4c..b2c189e797a2 100644 --- a/nova/api/openstack/__init__.py +++ b/nova/api/openstack/__init__.py @@ -174,18 +174,20 @@ class APIRouter(base_wsgi.Router): def _setup_extensions(self, ext_mgr): for extension in ext_mgr.get_controller_extensions(): - ext_name = extension.extension.name collection = extension.collection controller = extension.controller + msg_format_dict = {'collection': collection, + 'ext_name': extension.extension.name} if collection not in self.resources: LOG.warning(_('Extension %(ext_name)s: Cannot extend ' - 'resource %(collection)s: No such resource') % - locals()) + 'resource %(collection)s: No such resource'), + msg_format_dict) continue LOG.debug(_('Extension %(ext_name)s extending resource: ' - '%(collection)s') % locals()) + '%(collection)s'), + msg_format_dict) resource = self.resources[collection] resource.register_actions(controller) diff --git a/nova/api/openstack/common.py b/nova/api/openstack/common.py index 24250c0af98e..bec919f4bea0 100644 --- a/nova/api/openstack/common.py +++ b/nova/api/openstack/common.py @@ -113,7 +113,8 @@ def status_from_state(vm_state, task_state='default'): if status == "UNKNOWN": LOG.error(_("status is UNKNOWN from vm_state=%(vm_state)s " "task_state=%(task_state)s. Bad upgrade or db " - "corrupted?") % locals()) + "corrupted?"), + {'vm_state': vm_state, 'task_state': task_state}) return status @@ -358,11 +359,12 @@ def raise_http_conflict_for_instance_invalid_state(exc, action): attr = exc.kwargs.get('attr') state = exc.kwargs.get('state') if attr and state: - msg = _("Cannot '%(action)s' while instance is in %(attr)s %(state)s") + msg = _("Cannot '%(action)s' while instance is in %(attr)s " + "%(state)s") % {'action': action, 'attr': attr, 'state': state} else: # At least give some meaningful message - msg = _("Instance is in an invalid state for '%(action)s'") - raise webob.exc.HTTPConflict(explanation=msg % locals()) + msg = _("Instance is in an invalid state for '%s'") % action + raise webob.exc.HTTPConflict(explanation=msg) class MetadataDeserializer(wsgi.MetadataXMLDeserializer): diff --git a/nova/api/openstack/compute/contrib/admin_actions.py b/nova/api/openstack/compute/contrib/admin_actions.py index ff595fad6dc4..3452136c032b 100644 --- a/nova/api/openstack/compute/contrib/admin_actions.py +++ b/nova/api/openstack/compute/contrib/admin_actions.py @@ -289,11 +289,11 @@ class AdminActionsController(wsgi.Controller): raise exc.HTTPBadRequest(explanation=ex.format_message()) except Exception: if host is None: - msg = _("Live migration of instance %(id)s to another host" - " failed") % locals() + msg = _("Live migration of instance %s to another host " + "failed") % id else: - msg = _("Live migration of instance %(id)s to host %(host)s" - " failed") % locals() + msg = _("Live migration of instance %(id)s to host %(host)s " + "failed") % {'id': id, 'host': host} LOG.exception(msg) # Return messages from scheduler raise exc.HTTPBadRequest(explanation=msg) diff --git a/nova/api/openstack/compute/contrib/aggregates.py b/nova/api/openstack/compute/contrib/aggregates.py index b73a50f392a2..b6b9f3561131 100644 --- a/nova/api/openstack/compute/contrib/aggregates.py +++ b/nova/api/openstack/compute/contrib/aggregates.py @@ -86,7 +86,7 @@ class AggregateController(object): try: aggregate = self.api.get_aggregate(context, id) except exception.AggregateNotFound: - LOG.info(_("Cannot show aggregate: %(id)s") % locals()) + LOG.info(_("Cannot show aggregate: %s"), id) raise exc.HTTPNotFound return self._marshall_aggregate(aggregate) @@ -112,7 +112,7 @@ class AggregateController(object): try: aggregate = self.api.update_aggregate(context, id, updates) except exception.AggregateNotFound: - LOG.info(_("Cannot update aggregate: %(id)s") % locals()) + LOG.info(_('Cannot update aggregate: %s'), id) raise exc.HTTPNotFound return self._marshall_aggregate(aggregate) @@ -124,7 +124,7 @@ class AggregateController(object): try: self.api.delete_aggregate(context, id) except exception.AggregateNotFound: - LOG.info(_("Cannot delete aggregate: %(id)s") % locals()) + LOG.info(_('Cannot delete aggregate: %s'), id) raise exc.HTTPNotFound def action(self, req, id, body): @@ -137,7 +137,7 @@ class AggregateController(object): try: return _actions[action](req, id, data) except KeyError: - msg = _("Aggregates does not have %s action") % action + msg = _('Aggregates does not have %s action') % action raise exc.HTTPBadRequest(explanation=msg) raise exc.HTTPBadRequest(explanation=_("Invalid request body")) @@ -150,13 +150,13 @@ class AggregateController(object): try: aggregate = self.api.add_host_to_aggregate(context, id, host) except (exception.AggregateNotFound, exception.ComputeHostNotFound): - LOG.info(_("Cannot add host %(host)s in aggregate " - "%(id)s") % locals()) + LOG.info(_('Cannot add host %(host)s in aggregate %(id)s'), + {'host': host, 'id': id}) raise exc.HTTPNotFound except (exception.AggregateHostExists, exception.InvalidAggregateAction): - LOG.info(_("Cannot add host %(host)s in aggregate " - "%(id)s") % locals()) + LOG.info(_('Cannot add host %(host)s in aggregate %(id)s'), + {'host': host, 'id': id}) raise exc.HTTPConflict return self._marshall_aggregate(aggregate) @@ -169,12 +169,12 @@ class AggregateController(object): aggregate = self.api.remove_host_from_aggregate(context, id, host) except (exception.AggregateNotFound, exception.AggregateHostNotFound, exception.ComputeHostNotFound): - LOG.info(_("Cannot remove host %(host)s in aggregate " - "%(id)s") % locals()) + LOG.info(_('Cannot remove host %(host)s in aggregate %(id)s'), + {'host': host, 'id': id}) raise exc.HTTPNotFound except exception.InvalidAggregateAction: - LOG.info(_("Cannot remove host %(host)s in aggregate " - "%(id)s") % locals()) + LOG.info(_('Cannot remove host %(host)s in aggregate %(id)s'), + {'host': host, 'id': id}) raise exc.HTTPConflict return self._marshall_aggregate(aggregate) @@ -193,8 +193,8 @@ class AggregateController(object): aggregate = self.api.update_aggregate_metadata(context, id, metadata) except exception.AggregateNotFound: - LOG.info(_("Cannot set metadata %(metadata)s in aggregate " - "%(id)s") % locals()) + LOG.info(_('Cannot set metadata %(metadata)s in aggregate %(id)s'), + {'metadata': metadata, 'id': id}) raise exc.HTTPNotFound return self._marshall_aggregate(aggregate) diff --git a/nova/api/openstack/compute/contrib/floating_ips.py b/nova/api/openstack/compute/contrib/floating_ips.py index d2cf99431d60..88abf9e9550a 100644 --- a/nova/api/openstack/compute/contrib/floating_ips.py +++ b/nova/api/openstack/compute/contrib/floating_ips.py @@ -309,7 +309,7 @@ class FloatingIPActionController(wsgi.Controller): return webob.Response(status_int=202) else: msg = _("Floating ip %(address)s is not associated with instance " - "%(id)s.") % locals() + "%(id)s.") % {'address': address, 'id': id} raise webob.exc.HTTPUnprocessableEntity(explanation=msg) diff --git a/nova/api/openstack/compute/contrib/hosts.py b/nova/api/openstack/compute/contrib/hosts.py index a896678f0a6e..e0ea1203b20c 100644 --- a/nova/api/openstack/compute/contrib/hosts.py +++ b/nova/api/openstack/compute/contrib/hosts.py @@ -202,8 +202,9 @@ class HostController(object): def _set_host_maintenance(self, context, host_name, mode=True): """Start/Stop host maintenance window. On start, it triggers guest VMs evacuation.""" - LOG.audit(_("Putting host %(host_name)s in maintenance " - "mode %(mode)s.") % locals()) + LOG.audit(_("Putting host %(host_name)s in maintenance mode " + "%(mode)s."), + {'host_name': host_name, 'mode': mode}) try: result = self.api.set_host_maintenance(context, host_name, mode) except NotImplementedError: diff --git a/nova/api/openstack/compute/contrib/volumes.py b/nova/api/openstack/compute/contrib/volumes.py index 5b2097365a46..badd1525d2f3 100644 --- a/nova/api/openstack/compute/contrib/volumes.py +++ b/nova/api/openstack/compute/contrib/volumes.py @@ -397,9 +397,12 @@ class VolumeAttachmentController(wsgi.Controller): self._validate_volume_id(volume_id) - msg = _("Attach volume %(volume_id)s to instance %(server_id)s" - " at %(device)s") % locals() - LOG.audit(msg, context=context) + LOG.audit(_("Attach volume %(volume_id)s to instance %(server_id)s " + "at %(device)s"), + {'volume_id': volume_id, + 'device': device, + 'server_id': server_id}, + context=context) try: instance = self.compute_api.get(context, server_id) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 30f8e6fe87c3..35512456a58f 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -1404,9 +1404,8 @@ def remove_invalid_options(context, search_options, allowed_search_options): return # Otherwise, strip out all unknown options unknown_options = [opt for opt in search_options - if opt not in allowed_search_options] - unk_opt_str = ", ".join(unknown_options) - log_msg = _("Removing options '%(unk_opt_str)s' from query") % locals() - LOG.debug(log_msg) + if opt not in allowed_search_options] + LOG.debug(_("Removing options '%s' from query"), + ", ".join(unknown_options)) for opt in unknown_options: search_options.pop(opt, None) diff --git a/nova/api/openstack/extensions.py b/nova/api/openstack/extensions.py index dcf6149e519b..83e4b7ba5e4e 100644 --- a/nova/api/openstack/extensions.py +++ b/nova/api/openstack/extensions.py @@ -275,7 +275,8 @@ class ExtensionManager(object): self.load_extension(ext_factory) except Exception as exc: LOG.warn(_('Failed to load extension %(ext_factory)s: ' - '%(exc)s') % locals()) + '%(exc)s'), + {'ext_factory': ext_factory, 'exc': exc}) class ControllerExtension(object): @@ -344,19 +345,18 @@ def load_standard_extensions(ext_mgr, logger, path, package, ext_list=None): ext_mgr.load_extension(classpath) except Exception as exc: logger.warn(_('Failed to load extension %(classpath)s: ' - '%(exc)s') % locals()) + '%(exc)s'), + {'classpath': classpath, 'exc': exc}) # Now, let's consider any subdirectories we may have... subdirs = [] for dname in dirnames: # Skip it if it does not have __init__.py - if not os.path.exists(os.path.join(dirpath, dname, - '__init__.py')): + if not os.path.exists(os.path.join(dirpath, dname, '__init__.py')): continue # If it has extension(), delegate... - ext_name = ("%s%s.%s.extension" % - (package, relpkg, dname)) + ext_name = "%s%s.%s.extension" % (package, relpkg, dname) try: ext = importutils.import_class(ext_name) except ImportError: @@ -367,8 +367,9 @@ def load_standard_extensions(ext_mgr, logger, path, package, ext_list=None): try: ext(ext_mgr) except Exception as exc: - logger.warn(_('Failed to load extension %(ext_name)s: ' - '%(exc)s') % locals()) + logger.warn(_('Failed to load extension %(ext_name)s:' + '%(exc)s'), + {'ext_name': ext_name, 'exc': exc}) # Update the list of directories we'll explore... dirnames[:] = subdirs diff --git a/nova/api/openstack/wsgi.py b/nova/api/openstack/wsgi.py index 03674d767476..9c5f9855d0af 100644 --- a/nova/api/openstack/wsgi.py +++ b/nova/api/openstack/wsgi.py @@ -916,7 +916,9 @@ class Resource(wsgi.Application): return Fault(webob.exc.HTTPBadRequest(explanation=msg)) if body: - LOG.debug(_("Action: '%(action)s', body: %(body)s") % locals()) + msg = _("Action: '%(action)s', body: " + "%(body)s") % {'action': action, 'body': body} + LOG.debug(msg) LOG.debug(_("Calling method %s") % meth) # Now, deserialize the request body... diff --git a/nova/api/openstack/xmlutil.py b/nova/api/openstack/xmlutil.py index 9bcce808c536..04f5e28e303a 100644 --- a/nova/api/openstack/xmlutil.py +++ b/nova/api/openstack/xmlutil.py @@ -739,10 +739,9 @@ class MasterTemplate(Template): # Make sure we have a tree match if slave.root.tag != self.root.tag: - slavetag = slave.root.tag - mastertag = self.root.tag - msg = _("Template tree mismatch; adding slave %(slavetag)s " - "to master %(mastertag)s") % locals() + msg = _("Template tree mismatch; adding slave %(slavetag)s to " + "master %(mastertag)s") % {'slavetag': slave.root.tag, + 'mastertag': self.root.tag} raise ValueError(msg) # Make sure slave applies to this template