A shallow copy of an FS object leaves all the new object's task objects referring to the copied object. This patch set should fix that.
The first two patches enforce that an FSTask has only one instance attribute, its FS object, which is important for the fix, and also gets rid of some boilerplate initializers that existed in the expectation that there might someday be more instance attributes in a filesystem task.
From: mulhern amulhern@redhat.com
It defines the initializer, which should not vary among filesystem tasks.
Include a convenience class for simple unimplemented filesystem tasks.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/tasks/fstask.py | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 blivet/tasks/fstask.py
diff --git a/blivet/tasks/fstask.py b/blivet/tasks/fstask.py new file mode 100644 index 0000000..46dc3d6 --- /dev/null +++ b/blivet/tasks/fstask.py @@ -0,0 +1,47 @@ +# fstask.py +# Superclass for filesystem tasks. +# +# Copyright (C) 2015 Red Hat, Inc. +# +# This copyrighted material is made available to anyone wishing to use, +# modify, copy, or redistribute it subject to the terms and conditions of +# the GNU General Public License v.2, or (at your option) any later version. +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY expressed or implied, including the implied warranties of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General +# Public License for more details. You should have received a copy of the +# GNU General Public License along with this program; if not, write to the +# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA +# 02110-1301, USA. Any Red Hat trademarks that are incorporated in the +# source code or documentation are not subject to the GNU General Public +# License and may only be used or replicated with the express permission of +# Red Hat, Inc. +# +# Red Hat Author(s): Anne Mulhern amulhern@redhat.com + +import abc + +from six import add_metaclass + +from . import task + +@add_metaclass(abc.ABCMeta) +class FSTask(task.Task): + """ An abstract class that encapsulates the fact that all FSTasks + have a single master object: the filesystem that they belong to. + """ + description = "parent of all filesystem tasks" + + def __init__(self, an_fs): + """ Initializer. + + :param FS an_fs: a filesystem object + """ + self.fs = an_fs + +class UnimplementedFSTask(FSTask, task.UnimplementedTask): + """ A convenience class for unimplemented filesystem tasks. + Useful in the usual case where an Unimplemented task has + no special methods that it is required to implement. + """ + pass
From: mulhern amulhern@redhat.com
Inherit from these classes wherever possible in filesystem task classes.
Get rid of all initializers in subclasses of FSTask.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/tasks/fsck.py | 20 ++++---------------- blivet/tasks/fsinfo.py | 20 ++++---------------- blivet/tasks/fsminsize.py | 20 ++++---------------- blivet/tasks/fsmkfs.py | 17 ++--------------- blivet/tasks/fsmount.py | 6 ++---- blivet/tasks/fsreadlabel.py | 20 ++++---------------- blivet/tasks/fsresize.py | 17 ++--------------- blivet/tasks/fssize.py | 29 +++++------------------------ blivet/tasks/fssync.py | 20 ++++---------------- blivet/tasks/fswritelabel.py | 20 ++++---------------- 10 files changed, 35 insertions(+), 154 deletions(-)
diff --git a/blivet/tasks/fsck.py b/blivet/tasks/fsck.py index 80d8661..ec97c96 100644 --- a/blivet/tasks/fsck.py +++ b/blivet/tasks/fsck.py @@ -27,12 +27,13 @@ from .. import util
from . import availability +from . import fstask from . import task
_UNKNOWN_RC_MSG = "Unknown return code: %d"
@add_metaclass(abc.ABCMeta) -class FSCK(task.BasicApplication): +class FSCK(task.BasicApplication, fstask.FSTask): """An abstract class that represents actions associated with checking consistency of a filesystem. """ @@ -41,13 +42,6 @@ class FSCK(task.BasicApplication): options = abc.abstractproperty( doc="Options for invoking the application.")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - # IMPLEMENTATION methods
@abc.abstractmethod @@ -147,11 +141,5 @@ class NTFSFSCK(FSCK): def _errorMessage(self, rc): return _UNKNOWN_RC_MSG % (rc,) if rc != 0 else None
-class UnimplementedFSCK(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSCK(fstask.UnimplementedFSTask): + pass diff --git a/blivet/tasks/fsinfo.py b/blivet/tasks/fsinfo.py index 13ed555..5962cb7 100644 --- a/blivet/tasks/fsinfo.py +++ b/blivet/tasks/fsinfo.py @@ -27,10 +27,11 @@ from .. import util
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSInfo(task.BasicApplication): +class FSInfo(task.BasicApplication, fstask.FSTask): """ An abstract class that represents an information gathering app. """
description = "filesystem info" @@ -38,13 +39,6 @@ class FSInfo(task.BasicApplication): options = abc.abstractproperty( doc="Options for invoking the application.")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - @property def _infoCommand(self): """ Returns the command for reading filesystem information. @@ -96,11 +90,5 @@ class XFSInfo(FSInfo): ext = availability.XFSDB_APP options = ["-c", "sb 0", "-c", "p dblocks", "-c", "p blocksize"]
-class UnimplementedFSInfo(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSInfo(fstask.UnimplementedFSTask): + pass diff --git a/blivet/tasks/fsminsize.py b/blivet/tasks/fsminsize.py index 5044190..999240e 100644 --- a/blivet/tasks/fsminsize.py +++ b/blivet/tasks/fsminsize.py @@ -28,23 +28,17 @@ from ..size import Size
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSMinSize(task.BasicApplication): +class FSMinSize(task.BasicApplication, fstask.FSTask): """ An abstract class that represents min size information extraction. """
description = "minimum filesystem size"
options = abc.abstractproperty(doc="Options for use with app.")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - def _resizeCommand(self): return [str(self.ext)] + self.options + [self.fs.device]
@@ -177,11 +171,5 @@ def doTask(self): raise FSError("Unable to discover minimum size of filesystem on %s" % self.fs.device) return minSize
-class UnimplementedFSMinSize(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSMinSize(fstask.UnimplementedFSTask): + pass diff --git a/blivet/tasks/fsmkfs.py b/blivet/tasks/fsmkfs.py index a2110a9..542e61d 100644 --- a/blivet/tasks/fsmkfs.py +++ b/blivet/tasks/fsmkfs.py @@ -27,10 +27,11 @@ from .. import util
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSMkfsTask(task.Task): +class FSMkfsTask(fstask.FSTask):
canLabel = abc.abstractproperty(doc="whether this task labels")
@@ -44,13 +45,6 @@ class FSMkfs(task.BasicApplication, FSMkfsTask):
args = abc.abstractproperty(doc="options for creating filesystem")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - # IMPLEMENTATION methods
@property @@ -219,13 +213,6 @@ def args(self):
class UnimplementedFSMkfs(task.UnimplementedTask, FSMkfsTask):
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - @property def canLabel(self): return False diff --git a/blivet/tasks/fsmount.py b/blivet/tasks/fsmount.py index 552faed..9b674d5 100644 --- a/blivet/tasks/fsmount.py +++ b/blivet/tasks/fsmount.py @@ -27,9 +27,10 @@ from ..formats import fslib
from . import availability +from . import fstask from . import task
-class FSMount(task.BasicApplication): +class FSMount(task.BasicApplication, fstask.FSTask): """An abstract class that represents filesystem mounting actions. """ description = "mount a filesystem"
@@ -39,9 +40,6 @@ class FSMount(task.BasicApplication):
ext = availability.MOUNT_APP
- def __init__(self, an_fs): - self.fs = an_fs - # TASK methods
@property diff --git a/blivet/tasks/fsreadlabel.py b/blivet/tasks/fsreadlabel.py index 59e3e12..ec00f73 100644 --- a/blivet/tasks/fsreadlabel.py +++ b/blivet/tasks/fsreadlabel.py @@ -28,10 +28,11 @@ from .. import util
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSReadLabel(task.BasicApplication): +class FSReadLabel(task.BasicApplication, fstask.FSTask): """ An abstract class that represents reading a filesystem's label. """ description = "read filesystem label"
@@ -40,13 +41,6 @@ class FSReadLabel(task.BasicApplication):
args = abc.abstractproperty(doc="arguments for reading a label.")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - # IMPLEMENTATION methods
@property @@ -123,11 +117,5 @@ class XFSReadLabel(FSReadLabel): def args(self): return ["-l", self.fs.device]
-class UnimplementedFSReadLabel(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSReadLabel(fstask.UnimplementedFSTask): + pass diff --git a/blivet/tasks/fsresize.py b/blivet/tasks/fsresize.py index ef12686..6fb5e0e 100644 --- a/blivet/tasks/fsresize.py +++ b/blivet/tasks/fsresize.py @@ -28,10 +28,11 @@ from ..import util
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSResizeTask(task.Task): +class FSResizeTask(fstask.FSTask): """ The abstract properties that any resize task must have. """
unit = abc.abstractproperty(doc="Resize unit.") @@ -45,13 +46,6 @@ class FSResize(task.BasicApplication, FSResizeTask):
args = abc.abstractproperty(doc="Resize arguments.")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - # IMPLEMENTATION methods
@abc.abstractmethod @@ -135,13 +129,6 @@ def args(self):
class UnimplementedFSResize(task.UnimplementedTask, FSResizeTask):
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - @property def unit(self): raise NotImplementedError() diff --git a/blivet/tasks/fssize.py b/blivet/tasks/fssize.py index b28083e..aa82ac0 100644 --- a/blivet/tasks/fssize.py +++ b/blivet/tasks/fssize.py @@ -29,26 +29,20 @@ from .. import util
from . import availability +from . import fstask from . import task
_tags = ("count", "size") _Tags = namedtuple("_Tags", _tags)
@add_metaclass(abc.ABCMeta) -class FSSize(task.Task): +class FSSize(fstask.FSTask): """ An abstract class that represents size information extraction. """ description = "current filesystem size"
tags = abc.abstractproperty( doc="Strings used for extracting components of size.")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - # TASK methods
@property @@ -121,18 +115,11 @@ class ReiserFSSize(FSSize): class XFSSize(FSSize): tags = _Tags(size="blocksize =", count="dblocks =")
-class TmpFSSize(task.BasicApplication): +class TmpFSSize(task.BasicApplication, fstask.FSTask): description = "current filesystem size"
ext = availability.DF_APP
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - @property def _sizeCommand(self): return [str(self.ext), self.fs.systemMountpoint, "--output=size"] @@ -156,11 +143,5 @@ def doTask(self): return Size("%s KiB" % lines[1])
-class UnimplementedFSSize(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSSize(fstask.UnimplementedFSTask): + pass diff --git a/blivet/tasks/fssync.py b/blivet/tasks/fssync.py index e83aa12..e08288d 100644 --- a/blivet/tasks/fssync.py +++ b/blivet/tasks/fssync.py @@ -27,21 +27,15 @@ from .. import util
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSSync(task.BasicApplication): +class FSSync(task.BasicApplication, fstask.FSTask): """ An abstract class that represents syncing a filesystem. """
description = "filesystem syncing"
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - @abc.abstractmethod def doTask(self): raise NotImplementedError() @@ -79,11 +73,5 @@ def doTask(self, root="/"): if error_msg: raise FSError(error_msg)
-class UnimplementedFSSync(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSSync(fstask.UnimplementedFSTask): + pass diff --git a/blivet/tasks/fswritelabel.py b/blivet/tasks/fswritelabel.py index af64d92..50eacdf 100644 --- a/blivet/tasks/fswritelabel.py +++ b/blivet/tasks/fswritelabel.py @@ -27,23 +27,17 @@ from ..errors import FSWriteLabelError
from . import availability +from . import fstask from . import task
@add_metaclass(abc.ABCMeta) -class FSWriteLabel(task.BasicApplication): +class FSWriteLabel(task.BasicApplication, fstask.FSTask): """ An abstract class that represents writing a label for a filesystem. """
description = "write filesystem label"
args = abc.abstractproperty(doc="arguments for writing a label")
- def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs - # IMPLEMENTATION methods
@property @@ -106,11 +100,5 @@ class XFSWriteLabel(FSWriteLabel): def args(self): return ["-L", self.fs.label if self.fs.label != "" else "--", self.fs.device]
-class UnimplementedFSWriteLabel(task.UnimplementedTask): - - def __init__(self, an_fs): - """ Initializer. - - :param FS an_fs: a filesystem object - """ - self.fs = an_fs +class UnimplementedFSWriteLabel(fstask.UnimplementedFSTask): + pass
From: mulhern amulhern@redhat.com
Signed-off-by: mulhern amulhern@redhat.com --- blivet/formats/fs.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py index e2c5ef3..79bb68c 100644 --- a/blivet/formats/fs.py +++ b/blivet/formats/fs.py @@ -100,19 +100,7 @@ def __init__(self, **kwargs):
DeviceFormat.__init__(self, **kwargs)
- # Create task objects - self._info = self._infoClass(self) - self._fsck = self._fsckClass(self) - self._mkfs = self._mkfsClass(self) - self._mount = self._mountClass(self) - self._readlabel = self._readlabelClass(self) - self._resize = self._resizeClass(self) - self._sync = self._syncClass(self) - self._writelabel = self._writelabelClass(self) - - # These two may depend on info class, so create them after - self._minsize = self._minsizeClass(self) - self._sizeinfo = self._sizeinfoClass(self) + self._createTaskObjects()
self._current_info = None # info obtained by _info task
@@ -143,6 +131,22 @@ def __init__(self, **kwargs): if self.supported: self.loadModule()
+ def _createTaskObjects(self): + """ Create task objects belonging to this master object. """ + # pylint: disable=attribute-defined-outside-init + self._info = self._infoClass(self) + self._fsck = self._fsckClass(self) + self._mkfs = self._mkfsClass(self) + self._mount = self._mountClass(self) + self._readlabel = self._readlabelClass(self) + self._resize = self._resizeClass(self) + self._sync = self._syncClass(self) + self._writelabel = self._writelabelClass(self) + + # These two may depend on info class, so create them after + self._minsize = self._minsizeClass(self) + self._sizeinfo = self._sizeinfoClass(self) + def __repr__(self): s = DeviceFormat.__repr__(self) s += (" mountpoint = %(mountpoint)s mountopts = %(mountopts)s\n"
From: mulhern amulhern@redhat.com
Signed-off-by: mulhern amulhern@redhat.com --- blivet/formats/fs.py | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py index 79bb68c..e698db0 100644 --- a/blivet/formats/fs.py +++ b/blivet/formats/fs.py @@ -147,6 +147,16 @@ def _createTaskObjects(self): self._minsize = self._minsizeClass(self) self._sizeinfo = self._sizeinfoClass(self)
+ def __copy__(self): + # Constructs new task objects for the copied FS. + # This ensures that each task object refers back to the newly + # constructed object, result, rather than self. + cls = self.__class__ + result = cls.__new__(cls) + result.__dict__.update(self.__dict__) + result._createTaskObjects() + return result + def __repr__(self): s = DeviceFormat.__repr__(self) s += (" mountpoint = %(mountpoint)s mountopts = %(mountopts)s\n"
I know we discussed this before you did it, but I wonder if the correct change is to instead change all places where we do a shallow copy on a format to do a deep copy and leave the formats and tasks themselves as they are. After all, this would make a shallow copy of a format more like a deep copy, wouldn't it?
The tasks objects are somewhat special, because they refer back to their FS object. So, there could be some reason for treating them especially.
In favor of using deepcopy everywhere: Doing so may correct some subtle logical bugs in our code having nothing to do with tasks. It avoids implementing a ```__copy__``` method, which may itself be buggy.
Against using deepcopy everywhere: It is expensive and probably a bit buggy itself. We may find that we have to implement a version of ```__deepcopy__``` and then use variable_copy(), like in some of the Device classes. This would happen if traversal reaches some kind of object that can not itself be deepcopied.
I think it is a difficult call. But the conservative approach is to try using deepcopy everywhere, knowing that the other approach is a viable alternative. This way, no new code is added to solve the problem.
The first two patches are probably worth considering separately. They discard a bunch of boilerplate which I think has been revealed as quite pointless.
Superseded by #167.
Closed.
anaconda-patches@lists.fedorahosted.org
-
dashea