From 3d56b65c898d7997819f2627a2fb722bd0c33b69 Mon Sep 17 00:00:00 2001 From: Pete Zaitcev Date: Thu, 14 Aug 2014 13:57:53 -0600 Subject: [PATCH] Fix crash when downloading a pseudo-directory If a user creates an object with name ending with a slash, then downloading such container ends in a traceback like this: .............. test5g.file [auth 1.516s, headers 1.560s, total 244.565s, 22.089 MB/s] Traceback (most recent call last): File "/usr/lib/python2.6/site-packages/swiftclient/multithreading.py", lin result = self.func(item, *self.args, **self.kwargs) File "/usr/lib/python2.6/site-packages/swiftclient/shell.py", line 403, in fp = open(path, 'wb') IOError: [Errno 21] Is a directory: 'first-pseudo-folder/' The proposed fix is not to save this object. Note that the contents of the object are available with --output option, as before. Only the crash is fixed. Even though we do not use the contents, we download the object and check its Etag, in case. We also create a corresponding directory, in case the pseudo-directory contains no objects. The format of printout is changed, so user realizes easier when pseudo-directory convention is in effect. Note that this is not a compatibility issue because previously there was crash in such case. Change-Id: I3352f7a4eaf9970961af0cc84c4706fc1eab281d --- swiftclient/shell.py | 34 ++++++++++++++++++++++------------ tests/unit/test_shell.py | 11 ++++++++--- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/swiftclient/shell.py b/swiftclient/shell.py index ef153c71..f967009c 100755 --- a/swiftclient/shell.py +++ b/swiftclient/shell.py @@ -24,7 +24,7 @@ from hashlib import md5 from optparse import OptionParser, SUPPRESS_HELP from os import environ, listdir, makedirs, utime, _exit as os_exit -from os.path import dirname, getmtime, getsize, isdir, join, \ +from os.path import basename, dirname, getmtime, getsize, isdir, join, \ sep as os_path_sep from random import shuffle from sys import argv as sys_argv, exit, stderr, stdout @@ -381,7 +381,9 @@ def _download_object(queue_arg, conn): content_length = None etag = headers.get('etag') md5sum = None - make_dir = not options.no_download and out_file != "-" + pseudodir = False + no_file = options.no_download + make_dir = not no_file and out_file != "-" if content_type.split(';', 1)[0] == 'text/directory': if make_dir and not isdir(path): mkdirs(path) @@ -397,24 +399,28 @@ def _download_object(queue_arg, conn): dirpath = dirname(path) if make_dir and dirpath and not isdir(dirpath): mkdirs(dirpath) - if not options.no_download: + if not no_file: if out_file == "-": fp = stdout elif out_file: fp = open(out_file, 'wb') else: - fp = open(path, 'wb') + if basename(path): + fp = open(path, 'wb') + else: + pseudodir = True + no_file = True read_length = 0 if 'x-object-manifest' not in headers and \ 'x-static-large-object' not in headers: md5sum = md5() for chunk in body: - if not options.no_download: + if not no_file: fp.write(chunk) read_length += len(chunk) if md5sum: md5sum.update(chunk) - if not options.no_download: + if not no_file: fp.close() if md5sum and md5sum.hexdigest() != etag: thread_manager.error('%s: md5sum != etag, %s != %s', @@ -424,8 +430,7 @@ def _download_object(queue_arg, conn): '%s: read_length != content_length, %d != %d', path, read_length, content_length) if 'x-object-meta-mtime' in headers and not options.out_file \ - and not options.no_download: - + and not no_file: mtime = float(headers['x-object-meta-mtime']) utime(path, (mtime, mtime)) if options.verbose: @@ -434,10 +439,15 @@ def _download_object(queue_arg, conn): headers_receipt = headers_receipt - start_time total_time = finish_time - start_time download_time = total_time - auth_time - time_str = ('auth %.3fs, headers %.3fs, total %.3fs, ' - '%.3f MB/s' % ( - auth_time, headers_receipt, total_time, - float(read_length) / download_time / 1000000)) + if pseudodir: + time_str = ( + 'auth %.3fs, headers %.3fs, total %.3fs, pseudo' % ( + auth_time, headers_receipt, total_time)) + else: + time_str = ( + 'auth %.3fs, headers %.3fs, total %.3fs, %.3f MB/s' % ( + auth_time, headers_receipt, total_time, + float(read_length) / download_time / 1000000)) if conn.attempts > 1: thread_manager.print_msg('%s [%s after %d attempts]', path, time_str, conn.attempts) diff --git a/tests/unit/test_shell.py b/tests/unit/test_shell.py index 83113f0c..f4a569cf 100644 --- a/tests/unit/test_shell.py +++ b/tests/unit/test_shell.py @@ -183,6 +183,7 @@ def test_download(self, connection): # Test downloading whole container connection.return_value.get_container.side_effect = [ [None, [{'name': 'object'}]], + [None, [{'name': 'pseudo/'}]], [None, []], ] connection.return_value.auth_end_time = 0 @@ -191,9 +192,13 @@ def test_download(self, connection): with mock.patch(BUILTIN_OPEN) as mock_open: argv = ["", "download", "container"] swiftclient.shell.main(argv) - connection.return_value.get_object.assert_called_with( - 'container', 'object', headers={}, resp_chunk_size=65536) - mock_open.assert_called_with('object', 'wb') + calls = [mock.call('container', 'object', + headers={}, resp_chunk_size=65536), + mock.call('container', 'pseudo/', + headers={}, resp_chunk_size=65536)] + connection.return_value.get_object.assert_has_calls( + calls, any_order=True) + mock_open.assert_called_once_with('object', 'wb') # Test downloading single object with mock.patch(BUILTIN_OPEN) as mock_open: