launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10288
[Merge] lp:~allenap/maas/merge-python-tx-tftp into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/merge-python-tx-tftp into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/merge-python-tx-tftp/+merge/116741
Merge https://github.com/allenap/python-tx-tftp/tree/tsize-support and https://github.com/allenap/python-tx-tftp/tree/files-beneath-base into contrib, and update TFTPBackend because get_reader() always returns a Deferred now.
--
https://code.launchpad.net/~allenap/maas/merge-python-tx-tftp/+merge/116741
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/merge-python-tx-tftp into lp:maas.
=== modified file 'contrib/python-tx-tftp/tftp/backend.py'
--- contrib/python-tx-tftp/tftp/backend.py 2012-07-05 14:36:27 +0000
+++ contrib/python-tx-tftp/tftp/backend.py 2012-07-25 20:02:33 +0000
@@ -3,6 +3,7 @@
'''
from os import fstat
from tftp.errors import Unsupported, FileExists, AccessViolation, FileNotFound
+from tftp.util import deferred
from twisted.python.filepath import FilePath, InsecurePath
import shutil
import tempfile
@@ -33,8 +34,7 @@
@raise BackendError: for any other errors, that were encountered while
attempting to construct a reader
- @return: an object, that provides L{IReader}, or a L{Deferred} that
- will fire with an L{IReader}
+ @return: a L{Deferred} that will fire with an L{IReader}
"""
@@ -57,8 +57,7 @@
@raise BackendError: for any other errors, that were encountered while
attempting to construct a writer
- @return: an object, that provides L{IWriter}, or a L{Deferred} that
- will fire with an L{IWriter}
+ @return: a L{Deferred} that will fire with an L{IWriter}
"""
@@ -195,6 +194,9 @@
def __init__(self, file_path):
if file_path.exists():
raise FileExists(file_path)
+ file_dir = file_path.parent()
+ if not file_dir.exists():
+ file_dir.makedirs()
self.file_path = file_path
self.destination_file = self.file_path.open('w')
self.temp_destination = tempfile.TemporaryFile()
@@ -257,34 +259,34 @@
self.base = FilePath(base_path)
self.can_read, self.can_write = can_read, can_write
+ @deferred
def get_reader(self, file_name):
"""
@see: L{IBackend.get_reader}
- @return: an object, providing L{IReader}
- @rtype: L{FilesystemReader}
+ @rtype: L{Deferred}, yielding a L{FilesystemReader}
"""
if not self.can_read:
raise Unsupported("Reading not supported")
try:
- target_path = self.base.child(file_name)
+ target_path = self.base.descendant(file_name.split("/"))
except InsecurePath, e:
raise AccessViolation("Insecure path: %s" % e)
return FilesystemReader(target_path)
+ @deferred
def get_writer(self, file_name):
"""
@see: L{IBackend.get_writer}
- @return: an object, providing L{IWriter}
- @rtype: L{FilesystemWriter}
+ @rtype: L{Deferred}, yielding a L{FilesystemWriter}
"""
if not self.can_write:
raise Unsupported("Writing not supported")
try:
- target_path = self.base.child(file_name)
+ target_path = self.base.descendant(file_name.split("/"))
except InsecurePath, e:
raise AccessViolation("Insecure path: %s" % e)
return FilesystemWriter(target_path)
=== modified file 'contrib/python-tx-tftp/tftp/test/test_backend.py'
--- contrib/python-tx-tftp/tftp/test/test_backend.py 2012-07-05 14:36:27 +0000
+++ contrib/python-tx-tftp/tftp/test/test_backend.py 2012-07-25 20:02:33 +0000
@@ -2,11 +2,11 @@
@author: shylent
'''
from tftp.backend import (FilesystemSynchronousBackend, FilesystemReader,
- FilesystemWriter)
+ FilesystemWriter, IReader, IWriter)
from tftp.errors import Unsupported, AccessViolation, FileNotFound, FileExists
from twisted.python.filepath import FilePath
+from twisted.internet.defer import inlineCallbacks
from twisted.trial import unittest
-import os.path
import shutil
import tempfile
@@ -19,29 +19,57 @@
def setUp(self):
- self.temp_dir = tempfile.mkdtemp()
- self.existing_file_name = os.path.join(self.temp_dir, 'foo')
- with open(self.existing_file_name, 'w') as f:
- f.write(self.test_data)
-
- def test_unsupported(self):
- b = FilesystemSynchronousBackend(self.temp_dir, can_read=False)
- self.assertRaises(Unsupported, b.get_reader, 'foo')
- self.assert_(b.get_writer('bar'),
- "A writer should be dispatched")
- b = FilesystemSynchronousBackend(self.temp_dir, can_write=False)
- self.assertRaises(Unsupported, b.get_writer, 'bar')
- self.assert_(b.get_reader('foo'),
- "A reader should be dispatched")
-
- def test_insecure(self):
- b = FilesystemSynchronousBackend(self.temp_dir)
- self.assertRaises(AccessViolation, b.get_reader, '../foo')
- b = FilesystemSynchronousBackend(self.temp_dir)
- self.assertRaises(AccessViolation, b.get_writer, '../foo')
+ self.temp_dir = FilePath(tempfile.mkdtemp())
+ self.existing_file_name = self.temp_dir.descendant(("dir", "foo"))
+ self.existing_file_name.parent().makedirs()
+ self.existing_file_name.setContent(self.test_data)
+
+ @inlineCallbacks
+ def test_read_supported_by_default(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ reader = yield b.get_reader('dir/foo')
+ self.assertTrue(IReader.providedBy(reader))
+
+ @inlineCallbacks
+ def test_write_supported_by_default(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ writer = yield b.get_writer('dir/bar')
+ self.assertTrue(IWriter.providedBy(writer))
+
+ def test_read_unsupported(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path, can_read=False)
+ return self.assertFailure(b.get_reader('dir/foo'), Unsupported)
+
+ def test_write_unsupported(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path, can_write=False)
+ return self.assertFailure(b.get_writer('dir/bar'), Unsupported)
+
+ def test_insecure_reader(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ return self.assertFailure(
+ b.get_reader('../foo'), AccessViolation)
+
+ def test_insecure_writer(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ return self.assertFailure(
+ b.get_writer('../foo'), AccessViolation)
+
+ @inlineCallbacks
+ def test_read_ignores_leading_and_trailing_slashes(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ reader = yield b.get_reader('/dir/foo/')
+ segments_from_root = reader.file_path.segmentsFrom(self.temp_dir)
+ self.assertEqual(["dir", "foo"], segments_from_root)
+
+ @inlineCallbacks
+ def test_write_ignores_leading_and_trailing_slashes(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ writer = yield b.get_writer('/dir/bar/')
+ segments_from_root = writer.file_path.segmentsFrom(self.temp_dir)
+ self.assertEqual(["dir", "bar"], segments_from_root)
def tearDown(self):
- shutil.rmtree(self.temp_dir)
+ shutil.rmtree(self.temp_dir.path)
class Reader(unittest.TestCase):
@@ -94,6 +122,7 @@
r.read(3)
r.finish()
self.failUnless(r.file_obj.closed,
+
"The session has been finished, so the file object should be in the closed state")
r.finish()
@@ -110,12 +139,19 @@
def setUp(self):
self.temp_dir = FilePath(tempfile.mkdtemp())
self.existing_file_name = self.temp_dir.child('foo')
- with self.existing_file_name.open('w') as f:
- f.write(self.test_data)
+ self.existing_file_name.setContent(self.test_data)
def test_write_existing_file(self):
self.assertRaises(FileExists, FilesystemWriter, self.temp_dir.child('foo'))
+ def test_write_to_non_existent_directory(self):
+ new_directory = self.temp_dir.child("new")
+ new_file = new_directory.child("baz")
+ self.assertFalse(new_directory.exists())
+ FilesystemWriter(new_file).finish()
+ self.assertTrue(new_directory.exists())
+ self.assertTrue(new_file.exists())
+
def test_finished_write(self):
w = FilesystemWriter(self.temp_dir.child('bar'))
w.write(self.test_data)
=== modified file 'contrib/python-tx-tftp/tftp/test/test_protocol.py'
--- contrib/python-tx-tftp/tftp/test/test_protocol.py 2012-07-05 14:36:27 +0000
+++ contrib/python-tx-tftp/tftp/test/test_protocol.py 2012-07-25 20:02:33 +0000
@@ -202,15 +202,19 @@
self.clock = clock
def get_reader(self, file_name):
- reader = super(FilesystemAsyncBackend, self).get_reader(file_name)
+ d_get = super(FilesystemAsyncBackend, self).get_reader(file_name)
d = Deferred()
- self.clock.callLater(0, d.callback, reader)
+ # d_get has already fired, so don't chain d_get to d until later,
+ # otherwise d will be fired too early.
+ self.clock.callLater(0, d_get.chainDeferred, d)
return d
def get_writer(self, file_name):
- writer = super(FilesystemAsyncBackend, self).get_writer(file_name)
+ d_get = super(FilesystemAsyncBackend, self).get_writer(file_name)
d = Deferred()
- self.clock.callLater(0, d.callback, writer)
+ # d_get has already fired, so don't chain d_get to d until later,
+ # otherwise d will be fired too early.
+ self.clock.callLater(0, d_get.chainDeferred, d)
return d
@@ -224,7 +228,7 @@
self.backend = FilesystemAsyncBackend(self.tmp_dir_path, self.clock)
self.tftp = TFTP(self.backend, self.clock)
- def test_get_reader_can_defer(self):
+ def test_get_reader_defers(self):
rrq_datagram = RRQDatagram('nonempty', 'NetASCiI', {})
rrq_addr = ('127.0.0.1', 1069)
rrq_mode = "octet"
@@ -234,7 +238,7 @@
self.assertTrue(d.called)
self.assertTrue(IReader.providedBy(d.result.backend))
- def test_get_writer_can_defer(self):
+ def test_get_writer_defers(self):
wrq_datagram = WRQDatagram('foobar', 'NetASCiI', {})
wrq_addr = ('127.0.0.1', 1069)
wrq_mode = "octet"
=== modified file 'contrib/python-tx-tftp/tftp/util.py'
--- contrib/python-tx-tftp/tftp/util.py 2012-06-27 13:38:02 +0000
+++ contrib/python-tx-tftp/tftp/util.py 2012-07-25 20:02:33 +0000
@@ -1,10 +1,12 @@
'''
@author: shylent
'''
+from functools import wraps
from twisted.internet import reactor
-
-
-__all__ = ['SequentialCall', 'Spent', 'Cancelled']
+from twisted.internet.defer import maybeDeferred
+
+
+__all__ = ['SequentialCall', 'Spent', 'Cancelled', 'deferred']
class Spent(Exception):
@@ -120,3 +122,15 @@
def active(self):
"""Whether or not this L{SequentialCall} object is considered active"""
return not (self._spent or self._cancelled)
+
+
+def deferred(func):
+ """Decorates a function to ensure that it always returns a `Deferred`.
+
+ This also serves a secondary documentation purpose; functions decorated
+ with this are readily identifiable as asynchronous.
+ """
+ @wraps(func)
+ def wrapper(*args, **kwargs):
+ return maybeDeferred(func, *args, **kwargs)
+ return wrapper
=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py 2012-07-06 15:58:34 +0000
+++ src/provisioningserver/tests/test_tftp.py 2012-07-25 20:02:33 +0000
@@ -118,6 +118,7 @@
]
self.assertItemsEqual(query_expected, query)
+ @inlineCallbacks
def test_get_reader_regular_file(self):
# TFTPBackend.get_reader() returns a regular FilesystemReader for
# paths not matching re_config_file.
@@ -125,7 +126,7 @@
temp_file = self.make_file(name="example", contents=data)
temp_dir = path.dirname(temp_file)
backend = TFTPBackend(temp_dir, "http://nowhere.example.com/";)
- reader = backend.get_reader("example")
+ reader = yield backend.get_reader("example")
self.addCleanup(reader.finish)
self.assertEqual(len(data), reader.size)
self.assertEqual(data, reader.read(len(data)))
=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py 2012-07-06 16:04:53 +0000
+++ src/provisioningserver/tftp.py 2012-07-25 20:02:33 +0000
@@ -22,6 +22,7 @@
urlparse,
)
+from provisioningserver.utils import deferred
from tftp.backend import (
FilesystemSynchronousBackend,
IReader,
@@ -91,6 +92,7 @@
# maas_client._ascii_url() for inspiration.
return url.geturl().encode("ascii")
+ @deferred
def get_reader(self, file_name):
"""See `IBackend.get_reader()`.
