From 58841cab38c5d9d09184e28452d388add3c2eafa Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 21 Dec 2015 21:59:15 +0000 Subject: [PATCH 1/8] Refactor tahoe.cfg handling to configutil. Author: David Stainton Signed-off-by: Daira Hopwood --- src/allmydata/node.py | 19 +++------------ src/allmydata/test/test_configutil.py | 34 +++++++++++++++++++++++++++ src/allmydata/util/configutil.py | 29 +++++++++++++++++++++++ 3 files changed, 66 insertions(+), 16 deletions(-) create mode 100644 src/allmydata/test/test_configutil.py create mode 100644 src/allmydata/util/configutil.py diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 98ce3cb7c..f77c2ac26 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -12,6 +12,7 @@ from allmydata.util import fileutil, iputil, observer from allmydata.util.assertutil import precondition, _assert from allmydata.util.fileutil import abspath_expanduser_unicode from allmydata.util.encodingutil import get_filesystem_encoding, quote_output +from allmydata.util import configutil # Add our application versions to the data that Foolscap's LogPublisher # reports. @@ -133,27 +134,13 @@ class Node(service.MultiService): % (quote_output(fn), section, option)) return default - def set_config(self, section, option, value): - if not self.config.has_section(section): - self.config.add_section(section) - self.config.set(section, option, value) - assert self.config.get(section, option) == value - def read_config(self): self.error_about_old_config_files() self.config = ConfigParser.SafeConfigParser() tahoe_cfg = os.path.join(self.basedir, "tahoe.cfg") try: - f = open(tahoe_cfg, "rb") - try: - # Skip any initial Byte Order Mark. Since this is an ordinary file, we - # don't need to handle incomplete reads, and can assume seekability. - if f.read(3) != '\xEF\xBB\xBF': - f.seek(0) - self.config.readfp(f) - finally: - f.close() + self.config = configutil.get_config(tahoe_cfg) except EnvironmentError: if os.path.exists(tahoe_cfg): raise @@ -165,7 +152,7 @@ class Node(service.MultiService): # provide a value. try: file_tubport = fileutil.read(self._portnumfile).strip() - self.set_config("node", "tub.port", file_tubport) + configutil.set_config(self.config, "node", "tub.port", file_tubport) except EnvironmentError: if os.path.exists(self._portnumfile): raise diff --git a/src/allmydata/test/test_configutil.py b/src/allmydata/test/test_configutil.py new file mode 100644 index 000000000..14f87fbd8 --- /dev/null +++ b/src/allmydata/test/test_configutil.py @@ -0,0 +1,34 @@ +import os.path + +from twisted.trial import unittest + +from allmydata.util import configutil +from allmydata.test.no_network import GridTestMixin +from .test_cli import CLITestMixin + + +class ConfigUtilTests(CLITestMixin, GridTestMixin, unittest.TestCase): + + def test_config_utils(self): + self.basedir = "cli/ConfigUtilTests/test-config-utils" + self.set_up_grid() + tahoe_cfg = os.path.join(self.get_clientdir(i=0), "tahoe.cfg") + + # test that at least one option was read correctly + config = configutil.get_config(tahoe_cfg) + self.failUnlessEqual(config.get("node", "nickname"), "client-0") + + # test that set_config can mutate an existing option + configutil.set_config(config, "node", "nickname", "Alice!") + configutil.write_config(tahoe_cfg, config) + + config = configutil.get_config(tahoe_cfg) + self.failUnlessEqual(config.get("node", "nickname"), "Alice!") + + # test that set_config can set a new option + descriptor = "Twas brillig, and the slithy toves Did gyre and gimble in the wabe" + configutil.set_config(config, "node", "descriptor", descriptor) + configutil.write_config(tahoe_cfg, config) + + config = configutil.get_config(tahoe_cfg) + self.failUnlessEqual(config.get("node", "descriptor"), descriptor) diff --git a/src/allmydata/util/configutil.py b/src/allmydata/util/configutil.py new file mode 100644 index 000000000..19f712dce --- /dev/null +++ b/src/allmydata/util/configutil.py @@ -0,0 +1,29 @@ + +from ConfigParser import SafeConfigParser + + +def get_config(tahoe_cfg): + config = SafeConfigParser() + f = open(tahoe_cfg, "rb") + try: + # Skip any initial Byte Order Mark. Since this is an ordinary file, we + # don't need to handle incomplete reads, and can assume seekability. + if f.read(3) != '\xEF\xBB\xBF': + f.seek(0) + config.readfp(f) + finally: + f.close() + return config + +def set_config(config, section, option, value): + if not config.has_section(section): + config.add_section(section) + config.set(section, option, value) + assert config.get(section, option) == value + +def write_config(tahoe_cfg, config): + f = open(tahoe_cfg, "wb") + try: + config.write(f) + finally: + f.close() From 46719a8bcfc478932adf2d5acd5aee0930a27bfd Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:01:07 +0000 Subject: [PATCH 2/8] Aliases are Unicode. Signed-off-by: Daira Hopwood --- src/allmydata/scripts/tahoe_add_alias.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/allmydata/scripts/tahoe_add_alias.py b/src/allmydata/scripts/tahoe_add_alias.py index f3ed15c4a..30794429e 100644 --- a/src/allmydata/scripts/tahoe_add_alias.py +++ b/src/allmydata/scripts/tahoe_add_alias.py @@ -1,6 +1,9 @@ import os.path import codecs + +from allmydata.util.assertutil import precondition + from allmydata import uri from allmydata.scripts.common_http import do_http, check_http_error from allmydata.scripts.common import get_aliases @@ -29,6 +32,7 @@ def add_line_to_aliasfile(aliasfile, alias, cap): def add_alias(options): nodedir = options['node-directory'] alias = options.alias + precondition(isinstance(alias, unicode), alias=alias) cap = options.cap stdout = options.stdout stderr = options.stderr @@ -56,6 +60,7 @@ def create_alias(options): # mkdir+add_alias nodedir = options['node-directory'] alias = options.alias + precondition(isinstance(alias, unicode), alias=alias) stdout = options.stdout stderr = options.stderr if u":" in alias: From 48917186f26f6fd7bc5f0c60e5177775f87ddd67 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:08:22 +0000 Subject: [PATCH 3/8] URIs are strs. Signed-off-by: Daira Hopwood --- src/allmydata/uri.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/uri.py b/src/allmydata/uri.py index 372e0b8be..4d6db4829 100644 --- a/src/allmydata/uri.py +++ b/src/allmydata/uri.py @@ -730,7 +730,7 @@ ALLEGED_IMMUTABLE_PREFIX = 'imm.' def from_string(u, deep_immutable=False, name=u""): if not isinstance(u, str): - raise TypeError("unknown URI type: %s.." % str(u)[:100]) + raise TypeError("URI must be str: %r" % (u,)) # We allow and check ALLEGED_READONLY_PREFIX or ALLEGED_IMMUTABLE_PREFIX # on all URIs, even though we would only strictly need to do so for caps of From 394a4e25e64e0744ddcaf3dbd15ad5238e2b8047 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:46:16 +0000 Subject: [PATCH 4/8] Require arguments to do_cli to be strs. Signed-off-by: Daira Hopwood --- src/allmydata/test/test_cli.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_cli.py b/src/allmydata/test/test_cli.py index b59a2b1b1..bed358711 100644 --- a/src/allmydata/test/test_cli.py +++ b/src/allmydata/test/test_cli.py @@ -36,7 +36,7 @@ from twisted.python import usage from allmydata.util.assertutil import precondition from allmydata.util.encodingutil import listdir_unicode, unicode_platform, \ - get_io_encoding, get_filesystem_encoding + get_io_encoding, get_filesystem_encoding, unicode_to_argv timeout = 480 # deep_check takes 360s on Zandr's linksys box, others take > 240s @@ -49,8 +49,14 @@ def parse_options(basedir, command, args): class CLITestMixin(ReallyEqualMixin): def do_cli(self, verb, *args, **kwargs): + precondition(not [True for arg in args if not isinstance(arg, str)], + "arguments to do_cli must be strs -- convert using unicode_to_argv", args=args) + + # client_num is used to execute client CLI commands on a specific client. + client_num = kwargs.get("client_num", 0) + nodeargs = [ - "--node-directory", self.get_clientdir(), + "--node-directory", unicode_to_argv(self.get_clientdir(i=client_num)), ] argv = nodeargs + [verb] + list(args) stdin = kwargs.get("stdin", "") From 1f7069e622844e5404b3b33c9897e147a95c637f Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:34:41 +0000 Subject: [PATCH 5/8] test_encodingutil: use self.patch rather than modifying encodingutil.io_encoding directly. Signed-off-by: Daira Hopwood --- src/allmydata/test/test_encodingutil.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/allmydata/test/test_encodingutil.py b/src/allmydata/test/test_encodingutil.py index 1b700e995..9d6fe8f33 100644 --- a/src/allmydata/test/test_encodingutil.py +++ b/src/allmydata/test/test_encodingutil.py @@ -400,13 +400,13 @@ class QuoteOutput(ReallyEqualMixin, unittest.TestCase): check(u"\n", u"\"\\x0a\"", quote_newlines=True) def test_quote_output_default(self): - encodingutil.io_encoding = 'ascii' + self.patch(encodingutil, 'io_encoding', 'ascii') self.test_quote_output_ascii(None) - encodingutil.io_encoding = 'latin1' + self.patch(encodingutil, 'io_encoding', 'latin1') self.test_quote_output_latin1(None) - encodingutil.io_encoding = 'utf-8' + self.patch(encodingutil, 'io_encoding', 'utf-8') self.test_quote_output_utf8(None) From 56bf458355efc8b0844f679896538e41f361f793 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:03:35 +0000 Subject: [PATCH 6/8] Find the node-directory option correctly even if we are in a subcommand. Signed-off-by: Daira Hopwood --- src/allmydata/scripts/common.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/allmydata/scripts/common.py b/src/allmydata/scripts/common.py index d6246fc05..3bfe97500 100644 --- a/src/allmydata/scripts/common.py +++ b/src/allmydata/scripts/common.py @@ -57,9 +57,14 @@ class BasedirOptions(BaseOptions): ] def parseArgs(self, basedir=None): - if self.parent['node-directory'] and self['basedir']: + # This finds the node-directory option correctly even if we are in a subcommand. + root = self.parent + while root.parent is not None: + root = root.parent + + if root['node-directory'] and self['basedir']: raise usage.UsageError("The --node-directory (or -d) and --basedir (or -C) options cannot both be used.") - if self.parent['node-directory'] and basedir: + if root['node-directory'] and basedir: raise usage.UsageError("The --node-directory (or -d) option and a basedir argument cannot both be used.") if self['basedir'] and basedir: raise usage.UsageError("The --basedir (or -C) option and a basedir argument cannot both be used.") @@ -68,13 +73,14 @@ class BasedirOptions(BaseOptions): b = argv_to_abspath(basedir) elif self['basedir']: b = argv_to_abspath(self['basedir']) - elif self.parent['node-directory']: - b = argv_to_abspath(self.parent['node-directory']) + elif root['node-directory']: + b = argv_to_abspath(root['node-directory']) elif self.default_nodedir: b = self.default_nodedir else: raise usage.UsageError("No default basedir available, you must provide one with --node-directory, --basedir, or a basedir argument") self['basedir'] = b + self['node-directory'] = b def postOptions(self): if not self['basedir']: From e88e07a2781506a5eb661008ed6f5afde5ff5d4d Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:27:35 +0000 Subject: [PATCH 7/8] Improved error handling and cosmetics for ctypes calls on Windows. Signed-off-by: Daira Hopwood --- src/allmydata/util/fileutil.py | 31 +++++++++-------- src/allmydata/windows/fixups.py | 59 ++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/allmydata/util/fileutil.py b/src/allmydata/util/fileutil.py index a2c3841f7..152cae0bb 100644 --- a/src/allmydata/util/fileutil.py +++ b/src/allmydata/util/fileutil.py @@ -4,6 +4,11 @@ Futz with files like a pro. import sys, exceptions, os, stat, tempfile, time, binascii +if sys.platform == "win32": + from ctypes import WINFUNCTYPE, WinError, windll, POINTER, byref, c_ulonglong, \ + create_unicode_buffer, get_last_error + from ctypes.wintypes import BOOL, DWORD, LPCWSTR, LPWSTR + from twisted.python import log from pycryptopp.cipher.aes import AES @@ -335,16 +340,11 @@ def to_windows_long_path(path): have_GetDiskFreeSpaceExW = False if sys.platform == "win32": - from ctypes import WINFUNCTYPE, windll, POINTER, byref, c_ulonglong, create_unicode_buffer, \ - get_last_error - from ctypes.wintypes import BOOL, DWORD, LPCWSTR, LPWSTR - # GetEnvironmentVariableW = WINFUNCTYPE( - DWORD, - LPCWSTR, LPWSTR, DWORD, + DWORD, LPCWSTR, LPWSTR, DWORD, use_last_error=True - )(("GetEnvironmentVariableW", windll.kernel32)) + )(("GetEnvironmentVariableW", windll.kernel32)) try: # @@ -352,10 +352,9 @@ if sys.platform == "win32": # GetDiskFreeSpaceExW = WINFUNCTYPE( - BOOL, - LPCWSTR, PULARGE_INTEGER, PULARGE_INTEGER, PULARGE_INTEGER, + BOOL, LPCWSTR, PULARGE_INTEGER, PULARGE_INTEGER, PULARGE_INTEGER, use_last_error=True - )(("GetDiskFreeSpaceExW", windll.kernel32)) + )(("GetDiskFreeSpaceExW", windll.kernel32)) have_GetDiskFreeSpaceExW = True except Exception: @@ -403,8 +402,8 @@ def windows_getenv(name): err = get_last_error() if err == ERROR_ENVVAR_NOT_FOUND: return None - raise OSError("Windows error %d attempting to read size of environment variable %r" - % (err, name)) + raise OSError("WinError: %s\n attempting to read size of environment variable %r" + % (WinError(err), name)) if n == 1: # Avoid an ambiguity between a zero-length string and an error in the return value of the # call to GetEnvironmentVariableW below. @@ -416,8 +415,8 @@ def windows_getenv(name): err = get_last_error() if err == ERROR_ENVVAR_NOT_FOUND: return None - raise OSError("Windows error %d attempting to read environment variable %r" - % (err, name)) + raise OSError("WinError: %s\n attempting to read environment variable %r" + % (WinError(err), name)) if retval >= n: raise OSError("Unexpected result %d (expected less than %d) from GetEnvironmentVariableW attempting to read environment variable %r" % (retval, n, name)) @@ -459,8 +458,8 @@ def get_disk_stats(whichdir, reserved_space=0): byref(n_total), byref(n_free_for_root)) if retval == 0: - raise OSError("Windows error %d attempting to get disk statistics for %r" - % (get_last_error(), whichdir)) + raise OSError("WinError: %s\n attempting to get disk statistics for %r" + % (WinError(get_last_error()), whichdir)) free_for_nonroot = n_free_for_nonroot.value total = n_total.value free_for_root = n_free_for_root.value diff --git a/src/allmydata/windows/fixups.py b/src/allmydata/windows/fixups.py index eaf5d5eb9..490e08508 100644 --- a/src/allmydata/windows/fixups.py +++ b/src/allmydata/windows/fixups.py @@ -9,13 +9,18 @@ def initialize(): done = True import codecs, re - from ctypes import WINFUNCTYPE, windll, POINTER, byref, c_int + from ctypes import WINFUNCTYPE, WinError, windll, POINTER, byref, c_int, get_last_error from ctypes.wintypes import BOOL, HANDLE, DWORD, UINT, LPWSTR, LPCWSTR, LPVOID + from allmydata.util import log from allmydata.util.encodingutil import canonical_encoding # - SetErrorMode = WINFUNCTYPE(UINT, UINT)(("SetErrorMode", windll.kernel32)) + SetErrorMode = WINFUNCTYPE( + UINT, UINT, + use_last_error=True + )(("SetErrorMode", windll.kernel32)) + SEM_FAILCRITICALERRORS = 0x0001 SEM_NOOPENFILEERRORBOX = 0x8000 @@ -50,13 +55,27 @@ def initialize(): # # BOOL WINAPI GetConsoleMode(HANDLE hConsole, LPDWORD lpMode); - GetStdHandle = WINFUNCTYPE(HANDLE, DWORD)(("GetStdHandle", windll.kernel32)) + GetStdHandle = WINFUNCTYPE( + HANDLE, DWORD, + use_last_error=True + )(("GetStdHandle", windll.kernel32)) + STD_OUTPUT_HANDLE = DWORD(-11) STD_ERROR_HANDLE = DWORD(-12) - GetFileType = WINFUNCTYPE(DWORD, DWORD)(("GetFileType", windll.kernel32)) + + GetFileType = WINFUNCTYPE( + DWORD, DWORD, + use_last_error=True + )(("GetFileType", windll.kernel32)) + FILE_TYPE_CHAR = 0x0002 FILE_TYPE_REMOTE = 0x8000 - GetConsoleMode = WINFUNCTYPE(BOOL, HANDLE, POINTER(DWORD))(("GetConsoleMode", windll.kernel32)) + + GetConsoleMode = WINFUNCTYPE( + BOOL, HANDLE, POINTER(DWORD), + use_last_error=True + )(("GetConsoleMode", windll.kernel32)) + INVALID_HANDLE_VALUE = DWORD(-1).value def not_a_console(handle): @@ -88,11 +107,14 @@ def initialize(): real_stderr = False if real_stdout or real_stderr: + # # BOOL WINAPI WriteConsoleW(HANDLE hOutput, LPWSTR lpBuffer, DWORD nChars, # LPDWORD lpCharsWritten, LPVOID lpReserved); - WriteConsoleW = WINFUNCTYPE(BOOL, HANDLE, LPWSTR, DWORD, POINTER(DWORD), LPVOID) \ - (("WriteConsoleW", windll.kernel32)) + WriteConsoleW = WINFUNCTYPE( + BOOL, HANDLE, LPWSTR, DWORD, POINTER(DWORD), LPVOID, + use_last_error=True + )(("WriteConsoleW", windll.kernel32)) class UnicodeOutput: def __init__(self, hConsole, stream, fileno, name): @@ -139,8 +161,10 @@ def initialize(): # There is a shorter-than-documented limitation on the length of the string # passed to WriteConsoleW (see #1232). retval = WriteConsoleW(self._hConsole, text, min(remaining, 10000), byref(n), None) - if retval == 0 or n.value == 0: - raise IOError("WriteConsoleW returned %r, n.value = %r" % (retval, n.value)) + if retval == 0: + raise IOError("WriteConsoleW failed with WinError: %s" % (WinError(get_last_error()),)) + if n.value == 0: + raise IOError("WriteConsoleW returned %r, n.value = 0" % (retval,)) remaining -= n.value if remaining == 0: break text = text[n.value:] @@ -169,12 +193,23 @@ def initialize(): _complain("exception %r while fixing up sys.stdout and sys.stderr" % (e,)) # This works around . - GetCommandLineW = WINFUNCTYPE(LPWSTR)(("GetCommandLineW", windll.kernel32)) - CommandLineToArgvW = WINFUNCTYPE(POINTER(LPWSTR), LPCWSTR, POINTER(c_int)) \ - (("CommandLineToArgvW", windll.shell32)) + + # + GetCommandLineW = WINFUNCTYPE( + LPWSTR, + use_last_error=True + )(("GetCommandLineW", windll.kernel32)) + + # + CommandLineToArgvW = WINFUNCTYPE( + POINTER(LPWSTR), LPCWSTR, POINTER(c_int), + use_last_error=True + )(("CommandLineToArgvW", windll.shell32)) argc = c_int(0) argv_unicode = CommandLineToArgvW(GetCommandLineW(), byref(argc)) + if argv_unicode is None: + raise WinError(get_last_error()) # Because of (and similar limitations in # twisted), the 'bin/tahoe' script cannot invoke us with the actual Unicode arguments. From f2824459ebe604d4f99295651c086d282fa1f37b Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Mon, 28 Dec 2015 20:45:14 +0000 Subject: [PATCH 8/8] Reject path arguments that start with '-' with a usage error. Signed-off-by: Daira Hopwood --- src/allmydata/util/encodingutil.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/allmydata/util/encodingutil.py b/src/allmydata/util/encodingutil.py index d14b08f6b..61ce23ecb 100644 --- a/src/allmydata/util/encodingutil.py +++ b/src/allmydata/util/encodingutil.py @@ -88,12 +88,16 @@ def argv_to_unicode(s): raise usage.UsageError("Argument %s cannot be decoded as %s." % (quote_output(s), io_encoding)) -def argv_to_abspath(s): +def argv_to_abspath(s, **kwargs): """ Convenience function to decode an argv element to an absolute path, with ~ expanded. If this fails, raise a UsageError. """ - return abspath_expanduser_unicode(argv_to_unicode(s)) + decoded = argv_to_unicode(s) + if decoded.startswith(u'-'): + raise usage.UsageError("Path argument %s cannot start with '-'.\nUse %s if you intended to refer to a file." + % (quote_output(s), quote_output(os.path.join('.', s)))) + return abspath_expanduser_unicode(decoded, **kwargs) def unicode_to_argv(s, mangle=False): """