From 9c9d71c069dd00fe668fd33173e211b592d3a00d Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Tue, 28 Jan 2020 13:40:09 +0000 Subject: [PATCH 1/8] storage/reflink: omit redundant comment The is_dirty() one-liner is defined right above. --- qubes/storage/reflink.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 52783d60..05cbba2b 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -206,7 +206,7 @@ class ReflinkVolume(qubes.storage.Volume): @_locked def start(self): self._remove_incomplete_files() - if self.is_dirty(): # implies self.save_on_stop + if self.is_dirty(): return self if self.snap_on_start: # pylint: disable=protected-access From 8f4c90c37a5f8c59fede2e8e5e7cb10fc37598b5 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Tue, 28 Jan 2020 13:40:10 +0000 Subject: [PATCH 2/8] storage/reflink: _remove_incomplete_{files -> images}() --- qubes/storage/reflink.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 05cbba2b..6dc86ca8 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -179,14 +179,14 @@ class ReflinkVolume(qubes.storage.Volume): oldest to newest; remove empty VM directory. ''' self.pool._volumes.pop(self, None) # pylint: disable=protected-access - self._remove_incomplete_files() + self._remove_incomplete_images() self._prune_revisions(keep=0) _remove_file(self._path_clean) _remove_file(self._path_dirty) _remove_empty_dir(os.path.dirname(self._path_dirty)) return self - def _remove_incomplete_files(self): + def _remove_incomplete_images(self): for tmp in glob.iglob(glob.escape(self._path_vid) + '*.img*~*'): _remove_file(tmp) _remove_file(self._path_import) @@ -205,7 +205,7 @@ class ReflinkVolume(qubes.storage.Volume): @_coroutinized @_locked def start(self): - self._remove_incomplete_files() + self._remove_incomplete_images() if self.is_dirty(): return self if self.snap_on_start: From 12d882b35598fa7c647a7bdd3ade8e6ec2d8100b Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Tue, 28 Jan 2020 13:40:11 +0000 Subject: [PATCH 3/8] storage/reflink: factor out _remove_all_images() --- qubes/storage/reflink.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 6dc86ca8..ed6d758e 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -175,16 +175,16 @@ class ReflinkVolume(qubes.storage.Volume): @_coroutinized @_locked def remove(self): - ''' Drop volume object from pool; remove volume images from - oldest to newest; remove empty VM directory. - ''' self.pool._volumes.pop(self, None) # pylint: disable=protected-access + self._remove_all_images() + _remove_empty_dir(os.path.dirname(self._path_dirty)) + return self + + def _remove_all_images(self): self._remove_incomplete_images() self._prune_revisions(keep=0) _remove_file(self._path_clean) _remove_file(self._path_dirty) - _remove_empty_dir(os.path.dirname(self._path_dirty)) - return self def _remove_incomplete_images(self): for tmp in glob.iglob(glob.escape(self._path_vid) + '*.img*~*'): From 56f6a6ef65110959964e31ebe597e01dec461ce4 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Tue, 28 Jan 2020 13:40:13 +0000 Subject: [PATCH 4/8] storage/reflink: get VM dir from less arbitrary-looking path --- qubes/storage/reflink.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index ed6d758e..28a6993f 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -177,7 +177,7 @@ class ReflinkVolume(qubes.storage.Volume): def remove(self): self.pool._volumes.pop(self, None) # pylint: disable=protected-access self._remove_all_images() - _remove_empty_dir(os.path.dirname(self._path_dirty)) + _remove_empty_dir(os.path.dirname(self._path_vid)) return self def _remove_all_images(self): From 6659ed8d3964e61c25b412fc3d837c2ef80fcfe9 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Tue, 28 Jan 2020 13:40:14 +0000 Subject: [PATCH 5/8] storage/reflink: delete all images at beginning of create() Ensure that there are no leftover image files for the volume, e.g. from an unsuccessful removal of a previous incarnation of this vid, or from an messily restored pool filesystem backup. We don't want to preserve any stale data (revisions) or metadata (size) in the new incarnation. --- qubes/storage/reflink.py | 1 + 1 file changed, 1 insertion(+) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 28a6993f..c626da87 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -154,6 +154,7 @@ class ReflinkVolume(qubes.storage.Volume): @_coroutinized @_locked def create(self): + self._remove_all_images() if self.save_on_stop and not self.snap_on_start: _create_sparse_file(self._path_clean, self._get_size()) return self From 749ce477df2435150be0f60edd31a323e9ada836 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Tue, 28 Jan 2020 13:40:15 +0000 Subject: [PATCH 6/8] storage/reflink: don't bother using _get_size() in create() Only the nominal size is available at this point. --- qubes/storage/reflink.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index c626da87..508c6715 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -156,7 +156,7 @@ class ReflinkVolume(qubes.storage.Volume): def create(self): self._remove_all_images() if self.save_on_stop and not self.snap_on_start: - _create_sparse_file(self._path_clean, self._get_size()) + _create_sparse_file(self._path_clean, self._size) return self @_coroutinized From 21971d6d0a56da65ddbdcfbd3ff973d61cab77b0 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 5 Feb 2020 17:26:43 +0000 Subject: [PATCH 7/8] storage/reflink: comment on _get_size() use in start() --- qubes/storage/reflink.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 508c6715..59175c32 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -215,6 +215,9 @@ class ReflinkVolume(qubes.storage.Volume): if self.snap_on_start or self.save_on_stop: _copy_file(self._path_clean, self._path_dirty) else: + # Preferably use the size of a leftover image, in case + # the volume was previously resized - but then a crash + # prevented qubes.xml serialization of the new size. _create_sparse_file(self._path_dirty, self._get_size()) return self From ada27ee431165b4323461aaaecff58e483a515e9 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 5 Feb 2020 17:26:44 +0000 Subject: [PATCH 8/8] storage/reflink: trivial style tweaks Avoid early return for short and not deeply nested functions. --- qubes/storage/reflink.py | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 59175c32..c22ba8fa 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -207,18 +207,17 @@ class ReflinkVolume(qubes.storage.Volume): @_locked def start(self): self._remove_incomplete_images() - if self.is_dirty(): - return self - if self.snap_on_start: - # pylint: disable=protected-access - _copy_file(self.source._path_clean, self._path_clean) - if self.snap_on_start or self.save_on_stop: - _copy_file(self._path_clean, self._path_dirty) - else: - # Preferably use the size of a leftover image, in case - # the volume was previously resized - but then a crash - # prevented qubes.xml serialization of the new size. - _create_sparse_file(self._path_dirty, self._get_size()) + if not self.is_dirty(): + if self.snap_on_start: + # pylint: disable=protected-access + _copy_file(self.source._path_clean, self._path_clean) + if self.snap_on_start or self.save_on_stop: + _copy_file(self._path_clean, self._path_dirty) + else: + # Preferably use the size of a leftover image, in case + # the volume was previously resized - but then a crash + # prevented qubes.xml serialization of the new size. + _create_sparse_file(self._path_dirty, self._get_size()) return self @_coroutinized @@ -309,14 +308,13 @@ class ReflinkVolume(qubes.storage.Volume): @_coroutinized @_locked def import_volume(self, src_volume): - if not self.save_on_stop: - return self - try: - success = False - _copy_file(src_volume.export(), self._path_import) - success = True - finally: - self._import_data_end(success) + if self.save_on_stop: + try: + success = False + _copy_file(src_volume.export(), self._path_import) + success = True + finally: + self._import_data_end(success) return self def _path_revision(self, number, timestamp=None):