[blivet:master 13/21] Tidy up the definition of the device property throughout formats package.

David Lehman dlehman at redhat.com
Tue Jan 20 18:37:41 UTC 2015


On 01/15/2015 12:39 PM, mulhern wrote:
> * Eliminate some redundant definitions of the device property.
> The device property is defined using abstractions in DeviceFormat,
> so the appropriate NFS and TmpFS methods will be invoked appropriately.
> So the redundant definitions are totally unnecessary.
>
> * Omit NFS._getDevice().
> It pointlessly overrides DeviceFormat._getDevice().
>
> * Virtualize _deviceCheck(), not _setDevice().
> Currently, checking the device is the only thing that varies with the format.
> So, might as well virtualize that method, instead of _setDevice(), until
> that situaton changes. It's tidier. Note that a devspec of None is always
> the correct format; as before.
>
> * Do not explicitly set self._device in TmpFS constructor.
> NoDev constructor is already doing the right thing by setting the device
> to _type in its constructor.
>
> * Do not override _getDevice() in TmpFS.
> self._device == self._type == "tmpfs", so inheriting DeviceFormat._getDevice()
> works fine.

I would prefer if we could keep these changes just the tiniest bit 
focused. Most of this patch is just nit-picking. The changes don't 
justify the risk of regression they introduce or the time spent, for 
that matter. It would be better to just remove the redundant 
definitions, after making absolutely certain they are indeed redundant 
and save your time/energy for fixing real problems.

David

> ---
>   blivet/formats/__init__.py | 18 +++++++++++++-----
>   blivet/formats/fs.py       | 40 +++++++++++-----------------------------
>   2 files changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/blivet/formats/__init__.py b/blivet/formats/__init__.py
> index da1e829..6544753 100644
> --- a/blivet/formats/__init__.py
> +++ b/blivet/formats/__init__.py
> @@ -295,17 +295,25 @@ class DeviceFormat(ObjectID):
>          doc="fstab entry option string"
>       )
>
> -    def _setDevice(self, devspec):
> +    def _deviceCheck(self, devspec):
>           if devspec and not devspec.startswith("/"):
> -            raise ValueError("device must be a fully qualified path")
> +            return "device must be a fully qualified path"
> +        return None
> +
> +    def _setDevice(self, devspec):
> +        error_msg = self._deviceCheck(devspec)
> +        if error_msg:
> +            raise ValueError(devspec)
>           self._device = devspec # pylint: disable=attribute-defined-outside-init
>
>       def _getDevice(self):
>           return self._device
>
> -    device = property(lambda f: f._getDevice(),
> -                      lambda f,d: f._setDevice(d),
> -                      doc="Full path the device this format occupies")
> +    device = property(
> +       _getDevice,
> +       _setDevice,
> +       doc="Generally the path of the device this format occupies."
> +    )
>
>       @property
>       def name(self):
> diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> index 69ead04..4474a61 100644
> --- a/blivet/formats/fs.py
> +++ b/blivet/formats/fs.py
> @@ -1440,24 +1440,21 @@ class NFS(FS):
>       _modules = ["nfs"]
>
>       def _deviceCheck(self, devspec):
> +        """ Verifies that device spec has a proper format.
> +
> +            :param devspec: the device spec
> +            :type devspec: str or NoneType
> +            :rtype: str or NoneType
> +            :returns: an explanatory message if devspec fails check, else None
> +        """
>           if devspec is not None and ":" not in devspec:
> -            raise ValueError("device must be of the form <host>:<path>")
> +            return "device must be of the form <host>:<path>"
> +        return None
>
>       @property
>       def mountable(self):
>           return False
>
> -    def _setDevice(self, devspec):
> -        self._deviceCheck(devspec)
> -        self._device = devspec
> -
> -    def _getDevice(self):
> -        return self._device
> -
> -    device = property(lambda f: f._getDevice(),
> -                      lambda f,d: f._setDevice(d),
> -                      doc="Full path the device this format occupies")
> -
>   register_device_format(NFS)
>
>
> @@ -1487,8 +1484,8 @@ class NoDevFS(FS):
>           self.exists = True
>           self.device = self._type
>
> -    def _setDevice(self, devspec):
> -        self._device = devspec
> +    def _deviceCheck(self, devspec):
> +        return None
>
>       @property
>       def type(self):
> @@ -1542,7 +1539,6 @@ class TmpFS(NoDevFS):
>
>       def __init__(self, **kwargs):
>           NoDevFS.__init__(self, **kwargs)
> -        self._device = "tmpfs"
>
>           # according to the following Kernel ML thread:
>           # http://www.gossamer-threads.com/lists/linux/kernel/875278
> @@ -1621,20 +1617,6 @@ class TmpFS(NoDevFS):
>               free_space = self._size
>           return free_space
>
> -    def _getDevice(self):
> -        """ All the tmpfs mounts use the same "tmpfs" device. """
> -        return self._type
> -
> -    def _setDevice(self, value):
> -        # the DeviceFormat parent class does a
> -        # self.device = kwargs["device"]
> -        # assignment, so we need a setter for the
> -        # device property, but as the device is always the
> -        # same, nothing actually needs to be set
> -        pass
> -
> -    device = property(_getDevice, _setDevice)
> -
>       @property
>       def resizeArgs(self):
>           # a live tmpfs mount can be resized by remounting it with
>



More information about the anaconda-patches mailing list