From 15c31c0373dc8f102320637908c1ed59e854a4df Mon Sep 17 00:00:00 2001 From: Peter Portante Date: Mon, 21 Oct 2013 18:19:30 -0400 Subject: [PATCH] Remove unnecessary "is not None" check From a review comment on https://review.openstack.org/30051 remove the "is not None" check, as the assignment in the try block will never assign None as value as the int() built-in will not return it. There was some concern of long-term maintenance of the DiskFile class's _quarantine method raising exceptions. If that routine were ever mistakenly changed to NOT raise an exception, subtle problems could creep into the code (see https://review.openstack.org/53237). We address this concern by raising an exception explicitly at the call sites of DiskFile._quarantine(). Change-Id: I1729a2d77a6b72b4494b24a8838b47ad5272c075 Signed-off-by: Peter Portante --- swift/obj/diskfile.py | 27 ++++++++++++++------------- swift/obj/mem_diskfile.py | 12 ++++++------ 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index f8c1d10037..fe607dfbe8 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -1016,12 +1016,12 @@ class DiskFile(object): :param data_file: full path of data file to quarantine :param msg: reason for quarantining to be included in the exception - :raises DiskFileQuarantined: + :returns: DiskFileQuarantined exception object """ self._quarantined_dir = self._threadpool.run_in_thread( quarantine_renamer, self._device_path, data_file) self._logger.increment('quarantines') - raise DiskFileQuarantined(msg) + return DiskFileQuarantined(msg) def _get_ondisk_file(self): """ @@ -1051,7 +1051,7 @@ class DiskFile(object): if err.errno == errno.ENOTDIR: # If there's a file here instead of a directory, quarantine # it; something's gone wrong somewhere. - self._quarantine( + raise self._quarantine( # hack: quarantine_renamer actually renames the directory # enclosing the filename you give it, but here we just # want this one file and not its parent. @@ -1122,7 +1122,7 @@ class DiskFile(object): hash_from_fs = os.path.basename(self._datadir) hash_from_name = hash_path(self._name.lstrip('/')) if hash_from_fs != hash_from_name: - self._quarantine( + raise self._quarantine( data_file, "Hash of name in metadata does not match directory name") @@ -1145,7 +1145,7 @@ class DiskFile(object): try: mname = self._metadata['name'] except KeyError: - self._quarantine(data_file, "missing name metadata") + raise self._quarantine(data_file, "missing name metadata") else: if mname != self._name: self._logger.error( @@ -1161,7 +1161,7 @@ class DiskFile(object): except ValueError: # Quarantine, the x-delete-at key is present but not an # integer. - self._quarantine( + raise self._quarantine( data_file, "bad metadata x-delete-at value %s" % ( self._metadata['X-Delete-At'])) else: @@ -1170,12 +1170,12 @@ class DiskFile(object): try: metadata_size = int(self._metadata['Content-Length']) except KeyError: - self._quarantine( + raise self._quarantine( data_file, "missing content-length in metadata") except ValueError: # Quarantine, the content-length key is present but not an # integer. - self._quarantine( + raise self._quarantine( data_file, "bad metadata content-length value %s" % ( self._metadata['Content-Length'])) fd = fp.fileno() @@ -1183,11 +1183,11 @@ class DiskFile(object): statbuf = os.fstat(fd) except OSError as err: # Quarantine, we can't successfully stat the file. - self._quarantine(data_file, "not stat-able: %s" % err) + raise self._quarantine(data_file, "not stat-able: %s" % err) else: obj_size = statbuf.st_size - if metadata_size is not None and obj_size != metadata_size: - self._quarantine( + if obj_size != metadata_size: + raise self._quarantine( data_file, "metadata content-length %s does" " not match actual object size %s" % ( metadata_size, statbuf.st_size)) @@ -1200,8 +1200,9 @@ class DiskFile(object): try: return read_metadata(source) except Exception as err: - self._quarantine(quarantine_filename, - "Exception reading metadata: %s" % err) + raise self._quarantine( + quarantine_filename, + "Exception reading metadata: %s" % err) def _construct_from_data_file(self, data_file, meta_file): """ diff --git a/swift/obj/mem_diskfile.py b/swift/obj/mem_diskfile.py index da206dea72..eec51f3b7e 100644 --- a/swift/obj/mem_diskfile.py +++ b/swift/obj/mem_diskfile.py @@ -286,7 +286,7 @@ class DiskFile(object): try: mname = self._metadata['name'] except KeyError: - self._quarantine(self._name, "missing name metadata") + raise self._quarantine(self._name, "missing name metadata") else: if mname != self._name: raise DiskFileCollision('Client path does not match path ' @@ -298,7 +298,7 @@ class DiskFile(object): except ValueError: # Quarantine, the x-delete-at key is present but not an # integer. - self._quarantine( + raise self._quarantine( self._name, "bad metadata x-delete-at value %s" % ( self._metadata['X-Delete-At'])) else: @@ -307,12 +307,12 @@ class DiskFile(object): try: metadata_size = int(self._metadata['Content-Length']) except KeyError: - self._quarantine( + raise self._quarantine( self._name, "missing content-length in metadata") except ValueError: # Quarantine, the content-length key is present but not an # integer. - self._quarantine( + raise self._quarantine( self._name, "bad metadata content-length value %s" % ( self._metadata['Content-Length'])) try: @@ -321,9 +321,9 @@ class DiskFile(object): fp.seek(0, 0) except OSError as err: # Quarantine, we can't successfully stat the file. - self._quarantine(self._name, "not stat-able: %s" % err) + raise self._quarantine(self._name, "not stat-able: %s" % err) if obj_size != metadata_size: - self._quarantine( + raise self._quarantine( self._name, "metadata content-length %s does" " not match actual object size %s" % ( metadata_size, obj_size))