diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 709c1fda5..033f9ec4c 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -538,8 +538,8 @@ class HTTPServer(object): methods=["PUT"], ) def add_or_renew_lease(self, request, authorization, storage_index): - """Update the lease for an immutable share.""" - if not list(self._storage_server._get_bucket_shares(storage_index)): + """Update the lease for an immutable or mutable share.""" + if not list(self._storage_server.get_shares(storage_index)): raise _HTTPError(http.NOT_FOUND) # Checking of the renewal secret is done by the backend. @@ -674,7 +674,9 @@ class HTTPServer(object): ): """Indicate that given share is corrupt, with a text reason.""" # TODO unit test all the paths - if not self._storage_server._share_exists(storage_index, share_number): + if share_number not in { + shnum for (shnum, _) in self._storage_server.get_shares(storage_index) + }: raise _HTTPError(http.NOT_FOUND) info = self._read_encoded(request, _SCHEMAS["advise_corrupt_share"]) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index bcf44dc30..07b82b4d8 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -3,7 +3,7 @@ Ported to Python 3. """ from __future__ import annotations from future.utils import bytes_to_native_str -from typing import Dict, Tuple +from typing import Dict, Tuple, Iterable import os, re @@ -321,7 +321,7 @@ class StorageServer(service.MultiService): # they asked about: this will save them a lot of work. Add or update # leases for all of them: if they want us to hold shares for this # file, they'll want us to hold leases for this file. - for (shnum, fn) in self._get_bucket_shares(storage_index): + for (shnum, fn) in self.get_shares(storage_index): alreadygot[shnum] = ShareFile(fn) if renew_leases: self._add_or_renew_leases(alreadygot.values(), lease_info) @@ -363,7 +363,7 @@ class StorageServer(service.MultiService): return set(alreadygot), bucketwriters def _iter_share_files(self, storage_index): - for shnum, filename in self._get_bucket_shares(storage_index): + for shnum, filename in self.get_shares(storage_index): with open(filename, 'rb') as f: header = f.read(32) if MutableShareFile.is_valid_header(header): @@ -416,10 +416,12 @@ class StorageServer(service.MultiService): """ self._call_on_bucket_writer_close.append(handler) - def _get_bucket_shares(self, storage_index): - """Return a list of (shnum, pathname) tuples for files that hold + def get_shares(self, storage_index) -> Iterable[(int, str)]: + """ + Return an iterable of (shnum, pathname) tuples for files that hold shares for this storage_index. In each tuple, 'shnum' will always be - the integer form of the last component of 'pathname'.""" + the integer form of the last component of 'pathname'. + """ storagedir = os.path.join(self.sharedir, storage_index_to_dir(storage_index)) try: for f in os.listdir(storagedir): @@ -431,12 +433,15 @@ class StorageServer(service.MultiService): pass def get_buckets(self, storage_index): + """ + Get ``BucketReaders`` for an immutable. + """ start = self._clock.seconds() self.count("get") si_s = si_b2a(storage_index) log.msg("storage: get_buckets %r" % si_s) bucketreaders = {} # k: sharenum, v: BucketReader - for shnum, filename in self._get_bucket_shares(storage_index): + for shnum, filename in self.get_shares(storage_index): bucketreaders[shnum] = BucketReader(self, filename, storage_index, shnum) self.add_latency("get", self._clock.seconds() - start) @@ -453,7 +458,7 @@ class StorageServer(service.MultiService): # since all shares get the same lease data, we just grab the leases # from the first share try: - shnum, filename = next(self._get_bucket_shares(storage_index)) + shnum, filename = next(self.get_shares(storage_index)) sf = ShareFile(filename) return sf.get_leases() except StopIteration: @@ -467,7 +472,7 @@ class StorageServer(service.MultiService): :return: An iterable of the leases attached to this slot. """ - for _, share_filename in self._get_bucket_shares(storage_index): + for _, share_filename in self.get_shares(storage_index): share = MutableShareFile(share_filename) return share.get_leases() return [] @@ -742,7 +747,7 @@ class StorageServer(service.MultiService): :return bool: ``True`` if a share with the given number exists at the given storage index, ``False`` otherwise. """ - for existing_sharenum, ignored in self._get_bucket_shares(storage_index): + for existing_sharenum, ignored in self.get_shares(storage_index): if existing_sharenum == shnum: return True return False diff --git a/src/allmydata/test/test_repairer.py b/src/allmydata/test/test_repairer.py index 88696000c..f9b93af72 100644 --- a/src/allmydata/test/test_repairer.py +++ b/src/allmydata/test/test_repairer.py @@ -717,7 +717,7 @@ class Repairer(GridTestMixin, unittest.TestCase, RepairTestMixin, ss = self.g.servers_by_number[0] # we want to delete the share corresponding to the server # we're making not-respond - share = next(ss._get_bucket_shares(self.c0_filenode.get_storage_index()))[0] + share = next(ss.get_shares(self.c0_filenode.get_storage_index()))[0] self.delete_shares_numbered(self.uri, [share]) return self.c0_filenode.check_and_repair(Monitor()) d.addCallback(_then) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 65d09de25..91d55790e 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -766,7 +766,7 @@ class Server(unittest.TestCase): writer.close() # It should have a lease granted at the current time. - shares = dict(ss._get_bucket_shares(storage_index)) + shares = dict(ss.get_shares(storage_index)) self.assertEqual( [first_lease], list( @@ -789,7 +789,7 @@ class Server(unittest.TestCase): writer.close() # The first share's lease expiration time is unchanged. - shares = dict(ss._get_bucket_shares(storage_index)) + shares = dict(ss.get_shares(storage_index)) self.assertEqual( [first_lease], list(