[blivet:master 09/21] Simplify NoDevFS.type.

[Anne Mulhern]

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index 6fede96..158b7c4 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1494,10 +1494,7 @@ class NoDevFS(FS):

@property
     def type(self):
-        if self.device != self._type:
-            return self.device
-        else:
-            return self._type
+        return self.device

@property
     def mountType(self):

[blivet:master 10/21] Do not set self.exists to True in TmpFS.__init__().

[Anne Mulhern]

This behavior is implemented in NoDevFS.__init__().

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index 158b7c4..74c469e 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1544,7 +1544,6 @@ class TmpFS(NoDevFS):

def __init__(self, **kwargs):
         NoDevFS.__init__(self, **kwargs)
-        self.exists = True
         self._device = "tmpfs"

# according to the following Kernel ML thread:

[blivet:master 10/21] Do not set self.exists to True in TmpFS.__init__().

[David Lehman]

On 01/15/2015 12:39 PM, mulhern wrote:
> This behavior is implemented in NoDevFS.__init__().
> Signed-off-by: mulhern
>
> blivet/formats/fs.py | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> index 158b7c4..74c469e 100644
> --- a/blivet/formats/fs.py
> +++ b/blivet/formats/fs.py
> @@ -1544,7 +1544,6 @@ class TmpFS(NoDevFS):
>   def __init__(self, **kwargs):
>       NoDevFS.__init__(self, **kwargs)
>    self.exists = True
>     self._device = "tmpfs"
>
>     # according to the following Kernel ML thread:

Am I right to think that an active status is equivalent to existence for 
tmpfs?

[blivet:master 10/21] Do not set self.exists to True in TmpFS.__init__().

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Tuesday, January 20, 2015 1:25:56 PM
> Subject: Re: [blivet:master 10/21] Do not set self.exists to True in	TmpFS.__init__().
>
> On 01/15/2015 12:39 PM, mulhern wrote:
> > This behavior is implemented in NoDevFS.__init__().
> > Signed-off-by: mulhern
> >
> > blivet/formats/fs.py | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> > index 158b7c4..74c469e 100644
> > --- a/blivet/formats/fs.py
> > +++ b/blivet/formats/fs.py
> > @@ -1544,7 +1544,6 @@ class TmpFS(NoDevFS):
> >   def __init__(self, **kwargs):
> >       NoDevFS.__init__(self, **kwargs)
> >    self.exists = True
> >     self._device = "tmpfs"
> >
> >     # according to the following Kernel ML thread:
>
> Am I right to think that an active status is equivalent to existence for
> tmpfs?
> _______________________________________________
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

I do not know.

If you are arguing that a tmpfs does not exist unless and until it is mounted,
I would say that that makes some sense to me.

But what I'm doing here is just getting rid of another variant of a "pointless override",
not actually changing any behavior.

- mulhern

[blivet:master 10/21] Do not set self.exists to True in TmpFS.__init__().

[David Lehman]

On 01/20/2015 01:30 PM, Anne Mulhern wrote:
> ----- Original Message -----
> > From: "David Lehman" 
> > To: [e-mail]
> > Sent: Tuesday, January 20, 2015 1:25:56 PM
> > Subject: Re: [blivet:master 10/21] Do not set self.exists to True in	TmpFS.__init__().
> >
> > On 01/15/2015 12:39 PM, mulhern wrote:
> > > This behavior is implemented in NoDevFS.__init__().
> > > Signed-off-by: mulhern
> > >
> > > blivet/formats/fs.py | 1 -
> > >    1 file changed, 1 deletion(-)
> > >
> > > diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> > > index 158b7c4..74c469e 100644
> > > --- a/blivet/formats/fs.py
> > > +++ b/blivet/formats/fs.py
> > > @@ -1544,7 +1544,6 @@ class TmpFS(NoDevFS):
> > >    def __init__(self, **kwargs):
> > >        NoDevFS.__init__(self, **kwargs)
> > >    self.exists = True
> > >      self._device = "tmpfs"
> > >
> > >      # according to the following Kernel ML thread:
> >
> > Am I right to think that an active status is equivalent to existence for
> > tmpfs?
> > _______________________________________________
> > anaconda-patches mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
> I do not know.
>
> If you are arguing that a tmpfs does not exist unless and until it is mounted,
> I would say that that makes some sense to me.
>
> But what I'm doing here is just getting rid of another variant of a "pointless override",
> not actually changing any behavior.

Fair enough. Ack.
> mulhern
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

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

[Anne Mulhern]

* 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.
---
 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

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

[David Lehman]

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

[blivet:master 14/21] Remove _getExistingSize() methods with body pass.

[Anne Mulhern]

pass is equivalent to "return None" and we have a long-standing objection
to assigning None to fields that refer to Size objects.

For both classes, the effect of
self._size = self._getExistingSize()

is now

self._size = self._size, i.e., a null op

instead of

self._size = None

assuming self._size is some Size before the op.

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index 4474a61..a83e093 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1495,9 +1495,6 @@ class NoDevFS(FS):
     def mountType(self):
         return self.device  # this is probably set to the real/specific fstype

-    def _getExistingSize(self, info=None):
-        pass
-
 register_device_format(NoDevFS)

@@ -1655,9 +1652,6 @@ class BindFS(FS):
     def mountable(self):
         return True

-    def _getExistingSize(self, info=None):
-        pass
-
 register_device_format(BindFS)

[blivet:master 15/21] Make _getExistingSize() method more generally useful.

[Anne Mulhern]

If it gathers data only if self._size is not 0, then it is necessary
to set self._size to 0 to get it to run, which can prove awkward.

This doesn't change any observable behavior, since self._getExistingSize()
is always called when self._size is 0 anyway.

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index a83e093..bd0f081 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -305,11 +305,11 @@ class FS(DeviceFormat):
             :returns: size of existing fs in MiB.
             :rtype: float.
         """
-        size = self._size
+        if not self._existingSizeFields:
+            return Size(0)

-        if self.exists and not size:
-            if not self._existingSizeFields:
-                return Size(0)
+        size = Size(0)
+        if self.exists:
             if info is None:
                 info = self._getFSInfo()

[blivet:master 16/21] Add TmpFS._getExistingSize() method.

[Anne Mulhern]

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index bd0f081..c1bc3ed 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1577,6 +1577,31 @@ class TmpFS(NoDevFS):
         """
         pass

+    def _getExistingSize(self, info=None):
+        """ Get current size of tmpfs filesystem using df.
+
+            :param NoneType info: a dummy parameter
+            :rtype: Size
+            :returns: the current size of the filesystem, 0 if not found.
+        """
+        if not self.status:
+            return Size(0)
+
+        df = ["df", self._mountpoint, "--output=size"]
+        try:
+            (ret, out) = util.run_program_and_capture_output(df)
+        except OSError:
+            return Size(0)
+
+        if ret:
+            return Size(0)
+
+        lines = out.split()
+        if len(lines) != 2 or lines[0] != "1K-blocks":
+            return Size(0)
+
+        return Size("%s KiB" % lines[1])
+
     @property
     def mountable(self):
         return True

[blivet:master 17/21] Rewrite TmpFS class definition.

[Anne Mulhern]

Principle changes are:

* Abstract calculating the format for the size mount option into
_sizeOption method.

* Substitute FS.mountopts for TmpFS._options instance variable.

* Whether mounting or remounting always use default options if no
special options set and always use size mount option if required.

* do not set size to what the tmpfs default usually is. When it is
necessary to get the actual size, TmpFS._getExistingSize() will be used.
---
 blivet/formats/fs.py | 63 +++++++++++++++++++---------------------------------
 1 file changed, 23 insertions(+), 40 deletions(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index c1bc3ed..b1172d1 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1550,22 +1550,9 @@ class TmpFS(NoDevFS):
         # just use the default maxsize, which is 0, this disables
         # resizing but other operations such as mounting should work fine

-        # check if fixed filesystem size has been specified,
-        # if no size is specified, tmpfs will by default
-        # be limited to half of the system RAM
-        # (sizes of all tmpfs mounts are independent)
-        fsoptions = kwargs.get("mountopts")
-        self._size_option = ""
-        if fsoptions:
-            # some mount options were specified, replace the default value
-            self._options = fsoptions
-        if self._size:
-            # absolute size for the tmpfs mount has been specified
-            self._size_option = "size=%dm" % self._size.convertTo(MiB)
-        else:
-            # if no size option is specified, the tmpfs mount size will be 50%
-            # of system RAM by default
-            self._size = util.total_memory()/2
+        # if the size is 0, which is probably not set, accept the default
+        # size when mounting.
+        self._accept_default_size = not(self._size)

def create(self, **kwargs):
         """ A filesystem is created automatically once tmpfs is mounted. """
@@ -1606,6 +1593,15 @@ class TmpFS(NoDevFS):
     def mountable(self):
         return True

+    def _sizeOption(self, size):
+        """ Returns a size option string appropriate for mounting tmpfs.
+
+            :param Size size: any size
+            :returns: size option
+            :rtype: str
+        """
+        return "size=%dm" % size.convertTo(MiB)
+
     def _getOptions(self):
         # if the size option string is defined,
         # append it to options
@@ -1614,11 +1610,12 @@ class TmpFS(NoDevFS):
         # when the filesystem is resized;
         # replacing the size option in the otherwise free-form
         # options string would get messy fast)
-        opts = ",".join([o for o in [self._options, self._size_option] if o])
-        return opts or "defaults"
-
-    def _setOptions(self, options):
-        self._options = options
+        opts = super(TmpFS, self)._getOptions()
+        if self._accept_default_size:
+            size_opt = None
+        else:
+            size_opt = self._sizeOption(self._size)
+        return ",".join(o for o in (opts, size_opt) if o)

@property
     def free(self):
@@ -1643,29 +1640,15 @@ class TmpFS(NoDevFS):
     def resizeArgs(self):
         # a live tmpfs mount can be resized by remounting it with
         # the mount command
-
-        # add the remount flag, size and any options
-        remount_options = 'remount,size=%dm' % self.targetSize.convertTo(MiB)
-        # if any mount options are defined, append them
-        if self._options:
-            remount_options = "%s,%s" % (remount_options, self._options)
-        return ['-o', remount_options, self._type, self._mountpoint]
+        options = ("remount", self._sizeOption(self.targetSize), self.mountopts or ",".join(self.defaultMountOptions))
+        return ['-o', ",".join(options), self._type, self._mountpoint]

def doResize(self):
-        # we need to override doResize, because the
-        # self._size_option string needs to be updated in case
-        # the tmpfs mount is successfully resized, using the new
-        # self._size value
+        # Override superclass method to record whether mount options
+        # should include an explicit size specification.
         original_size = self._size
         FS.doResize(self)
-        # after a successful resize, self._size
-        # is set to be equal to self.targetSize,
-        # so it can be used to check if resize took place
-        if original_size != self._size:
-            # update the size option string
-            # -> please note that resizing always sets the
-            # size of this tmpfs mount to an absolute value
-            self._size_option = "size=%dm" % self._size.convertTo(MiB)
+        self._accept_default_size = self._accept_default_size and original_size == self._size

register_device_format(TmpFS)

[blivet:master 17/21] Rewrite TmpFS class definition.

[David Lehman]

On 01/15/2015 12:39 PM, mulhern wrote:
> Principle changes are:
> Abstract calculating the format for the size mount option into
>
> _sizeOption method.
>
> Substitute FS.mountopts for TmpFS._options instance variable.
>
> Whether mounting or remounting always use default options if no
>
> special options set and always use size mount option if required.
> do not set size to what the tmpfs default usually is. When it is
> necessary to get the actual size, TmpFS._getExistingSize() will be used.
>
> blivet/formats/fs.py | 63 +++++++++++++++++++---------------------------------
>   1 file changed, 23 insertions(+), 40 deletions(-)
>
> diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> index c1bc3ed..b1172d1 100644
> --- a/blivet/formats/fs.py
> +++ b/blivet/formats/fs.py
> @@ -1550,22 +1550,9 @@ class TmpFS(NoDevFS):
>           # just use the default maxsize, which is 0, this disables
>           # resizing but other operations such as mounting should work fine
>    # check if fixed filesystem size has been specified,
>    # if no size is specified, tmpfs will by default
>    # be limited to half of the system RAM
>    # (sizes of all tmpfs mounts are independent)
>    fsoptions = kwargs.get("mountopts")
>    self._size_option = ""
>    if fsoptions:
>        # some mount options were specified, replace the default value
>        self._options = fsoptions
>    if self._size:
>        # absolute size for the tmpfs mount has been specified
>        self._size_option = "size=%dm" % self._size.convertTo(MiB)
>    else:
>        # if no size option is specified, the tmpfs mount size will be 50%
>        # of system RAM by default
>        self._size = util.total_memory()/2
>    # if the size is 0, which is probably not set, accept the default
>    # size when mounting.
>    self._accept_default_size = not(self._size)
>
> def create(self, **kwargs):
>     """ A filesystem is created automatically once tmpfs is mounted. """
>
> @@ -1606,6 +1593,15 @@ class TmpFS(NoDevFS):
>       def mountable(self):
>           return True
> def _sizeOption(self, size):
>    """ Returns a size option string appropriate for mounting tmpfs.
>        :param Size size: any size
>        :returns: size option
>        :rtype: str
>    """
>    return "size=%dm" % size.convertTo(MiB)
> def _getOptions(self):
>     # if the size option string is defined,
>     # append it to options
>
> @@ -1614,11 +1610,12 @@ class TmpFS(NoDevFS):
>           # when the filesystem is resized;
>           # replacing the size option in the otherwise free-form
>           # options string would get messy fast)
>    opts = ",".join([o for o in [self._options, self._size_option] if o])
>    return opts or "defaults"
> def _setOptions(self, options):
>    self._options = options
>    opts = super(TmpFS, self)._getOptions()
>    if self._accept_default_size:
>        size_opt = None
>    else:
>        size_opt = self._sizeOption(self._size)
>    return ",".join(o for o in (opts, size_opt) if o)
>
> @property
> def free(self):
>
> @@ -1643,29 +1640,15 @@ class TmpFS(NoDevFS):
>       def resizeArgs(self):
>           # a live tmpfs mount can be resized by remounting it with
>           # the mount command
>    # add the remount flag, size and any options
>    remount_options = 'remount,size=%dm' % self.targetSize.convertTo(MiB)
>    # if any mount options are defined, append them
>    if self._options:
>        remount_options = "%s,%s" % (remount_options, self._options)
>    return ['-o', remount_options, self._type, self._mountpoint]
>    options = ("remount", self._sizeOption(self.targetSize), self.mountopts or ",".join(self.defaultMountOptions))
>    return ['-o', ",".join(options), self._type, self._mountpoint]
>
> def doResize(self):
>    # we need to override doResize, because the
>    # self._size_option string needs to be updated in case
>    # the tmpfs mount is successfully resized, using the new
>    # self._size value
>    # Override superclass method to record whether mount options
>    # should include an explicit size specification.
>     original_size = self._size
>     FS.doResize(self)
>    # after a successful resize, self._size
>    # is set to be equal to self.targetSize,
>    # so it can be used to check if resize took place
>    if original_size != self._size:
>        # update the size option string
>        # -> please note that resizing always sets the
>        # size of this tmpfs mount to an absolute value
>        self._size_option = "size=%dm" % self._size.convertTo(MiB)
>    self._accept_default_size = self._accept_default_size and original_size == self._size
>
> register_device_format(TmpFS)

All these tmpfs changes are taking way more time/effort than is called 
for. The main thing you need to do is make sure that the supported 
anaconda/kickstart functionality still works with your changes.

David

[blivet:master 18/21] Add TmpFS._resizefsUnit and use appropriately.

[Anne Mulhern]

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index b1172d1..9effed6 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1526,6 +1526,7 @@ class TmpFS(NoDevFS):
     # remounting can be used to change
     # the size of a live tmpfs mount
     _resizefs = "mount"
+    _resizefsUnit = MiB
     # as tmpfs is part of the Linux kernel,
     # it is Linux-native
     _linuxNative = True
@@ -1600,7 +1601,8 @@ class TmpFS(NoDevFS):
             :returns: size option
             :rtype: str
         """
-        return "size=%dm" % size.convertTo(MiB)
+        FMT = {KiB: "%dk", MiB: "%dm", GiB: "%dg"}[self._resizefsUnit]
+        return "size=%s" % (FMT % size.convertTo(self._resizefsUnit))

def _getOptions(self):
         # if the size option string is defined,

[blivet:master 19/21] Don't notify the kernel if it was tmpfs filesystem that was resized.

[Anne Mulhern]

The kernel should notice a resize of a tmpfs filesystem, since that type
of filesystem resides in the kernel's memory.

And, our current method of notifying the kernel will result in an
exception being raised in the case of tmpfs.

Signed-off-by: mulhern 
---
 blivet/formats/fs.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index 9effed6..dc4d8a9 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -519,7 +519,8 @@ class FS(DeviceFormat):

# XXX must be a smarter way to do this
         self._size = self.targetSize
-        self.notifyKernel()
+        if not self.device == "tmpfs":
+            self.notifyKernel()

def _getCheckArgs(self):
         argv = []

[blivet:master 19/21] Don't notify the kernel if it was tmpfs filesystem that was resized.

[David Lehman]

On 01/15/2015 12:39 PM, mulhern wrote:
> The kernel should notice a resize of a tmpfs filesystem, since that type
> of filesystem resides in the kernel's memory.
>
> And, our current method of notifying the kernel will result in an
> exception being raised in the case of tmpfs.
> Signed-off-by: mulhern
>
> blivet/formats/fs.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> index 9effed6..dc4d8a9 100644
> --- a/blivet/formats/fs.py
> +++ b/blivet/formats/fs.py
> @@ -519,7 +519,8 @@ class FS(DeviceFormat):
>       # XXX must be a smarter way to do this
>       self._size = self.targetSize
>    self.notifyKernel()
>    if not self.device == "tmpfs":

if self.device != "tmpfs" reads better.
>        self.notifyKernel()
>
> def _getCheckArgs(self):
>     argv = []

[blivet:master 19/21] Don't notify the kernel if it was tmpfs filesystem that was resized.

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Tuesday, January 20, 2015 1:29:52 PM
> Subject: Re: [blivet:master 19/21] Don't notify the kernel if it was tmpfs	filesystem that was resized.
>
> On 01/15/2015 12:39 PM, mulhern wrote:
> > The kernel should notice a resize of a tmpfs filesystem, since that type
> > of filesystem resides in the kernel's memory.
> >
> > And, our current method of notifying the kernel will result in an
> > exception being raised in the case of tmpfs.
> > Signed-off-by: mulhern
> >
> > blivet/formats/fs.py | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> > index 9effed6..dc4d8a9 100644
> > --- a/blivet/formats/fs.py
> > +++ b/blivet/formats/fs.py
> > @@ -519,7 +519,8 @@ class FS(DeviceFormat):
> >       # XXX must be a smarter way to do this
> >       self._size = self.targetSize
> >    self.notifyKernel()
> >    if not self.device == "tmpfs":
>
> if self.device != "tmpfs" reads better.
> >        self.notifyKernel()
> >
> > def _getCheckArgs(self):
> >     argv = []
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

Done.

- mulhern

[blivet:master 20/21] Add an additional test for TmpFS.

[Anne Mulhern]

Signed-off-by: mulhern 
---
 tests/formats_test/fs_test.py | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/formats_test/fs_test.py b/tests/formats_test/fs_test.py
index 6bbafeb..8dd6569 100755
--- a/tests/formats_test/fs_test.py
+++ b/tests/formats_test/fs_test.py
@@ -1,7 +1,10 @@
 #!/usr/bin/python
+import os
+import tempfile
 import unittest

import blivet.formats.fs as fs
+from blivet.size import Size, ROUND_DOWN

from tests import loopbackedtestcase

@@ -116,5 +119,39 @@ class SimpleTmpFSTestCase(loopbackedtestcase.LoopBackedTestCase):
         self.assertEqual(an_fs.device, "tmpfs")
         self.assertTrue(an_fs.testMount())

+class ResizeTmpFSTestCase(loopbackedtestcase.LoopBackedTestCase):
+
+    def __init__(self, methodName='runTest'):
+        super(ResizeTmpFSTestCase, self).__init__(methodName=methodName)
+        self.an_fs = fs.TmpFS()
+        self.mountpoint = None
+
+    def setUp(self):
+        self.mountpoint = tempfile.mkdtemp()
+        self.an_fs.mountpoint = self.mountpoint
+        self.an_fs.mount()
+
+    def testResize(self):
+        self.an_fs.updateSizeInfo()
+        newsize = self.an_fs.currentSize * 2
+        self.an_fs.targetSize = newsize
+        self.assertIsNone(self.an_fs.doResize())
+        self.assertEqual(self.an_fs.size, newsize.roundToNearest(self.an_fs._resizefsUnit, rounding=ROUND_DOWN))
+
+    def testShrink(self):
+        # Can not shrink tmpfs, because its minimum size is its current size
+        self.an_fs.updateSizeInfo()
+        newsize = Size("2 MiB")
+        self.assertTrue(newsize < self.an_fs.currentSize)
+        with self.assertRaises(ValueError):
+            self.an_fs.targetSize = newsize
+
+    def teardown(self):
+        try:
+            self.an_fs.unmount()
+        except Exception: # pylint: disable=broad-except
+            pass
+        os.rmdir(self.mountpoint)
+
 if __name__ == "__main__":
     unittest.main()

[blivet:master 21/21] Set TmpFS._resizable to False.

[Anne Mulhern]

Hack the tests slightly to allow them to continue running.

Signed-off-by: mulhern 
---
 blivet/formats/fs.py          | 1 -
 tests/formats_test/fs_test.py | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index dc4d8a9..0bf3a66 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1523,7 +1523,6 @@ register_device_format(SysFS)
 class TmpFS(NoDevFS):
     _type = "tmpfs"
     _supported = True
-    _resizable = True
     # remounting can be used to change
     # the size of a live tmpfs mount
     _resizefs = "mount"
diff --git a/tests/formats_test/fs_test.py b/tests/formats_test/fs_test.py
index 8dd6569..69859ba 100755
--- a/tests/formats_test/fs_test.py
+++ b/tests/formats_test/fs_test.py
@@ -124,6 +124,7 @@ class ResizeTmpFSTestCase(loopbackedtestcase.LoopBackedTestCase):
     def __init__(self, methodName='runTest'):
         super(ResizeTmpFSTestCase, self).__init__(methodName=methodName)
         self.an_fs = fs.TmpFS()
+        self.an_fs.__class__._resizable = True
         self.mountpoint = None

def setUp(self):

[blivet:master 00/21] Patches about mounting filesystems (v2)

[Anne Mulhern]

New ones are:
* Simplify NoDevFS.type
* Do not set self.exists to True...
* Tidy up the definitions of the device...and all patches after it.

Looking at the git history makes me think that it has never been possible
to resize tmpfs before, since the code path on which resize was failing
previously, i.e., RuntimeError in notifyKernel(),
has existed unchanged since before the split from anaconda.

Because I believe that it has never worked and because I am still not
certain that the resizing that I have implemented for TmpFS actually
works within the framework that blivet requires for resizing actions, I
am setting TmpFS._resizable to False.

mulhern (21):
  Add a tiny test for TmpFS.
  Make error message more useful.
  Be less eager about processing all lines in /proc/meminfo.
  Simplify setting options variable.
  Simplify FS._getOptions().
  Tighten up FS.mountable().
  Do not bother to set device.format.mountopts.
  Set format's mountpoint if it has the mountpoint attribute.
  Simplify NoDevFS.type.
  Do not set self.exists to True in TmpFS.__init__().
  Do not redefine size property in TmpFS.
  Virtualize options property methods in DeviceFormat.options
    definition.
  Tidy up the definition of the device property throughout formats
    package.
  Remove _getExistingSize() methods with body pass.
  Make _getExistingSize() method more generally useful.
  Add TmpFS._getExistingSize() method.
  Rewrite TmpFS class definition.
  Add TmpFS._resizefsUnit and use appropriately.
  Don't notify the kernel if it was tmpfs filesystem that was resized.
  Add an additional test for TmpFS.
  Set TmpFS._resizable to False.

blivet/__init__.py            |  13 +--
 blivet/formats/__init__.py    |  24 ++++--
 blivet/formats/fs.py          | 189 ++++++++++++++++++------------------------
 blivet/formats/swap.py        |   3 -
 blivet/util.py                |   7 +-
 tests/formats_test/fs_test.py |  53 ++++++++++++
 6 files changed, 156 insertions(+), 133 deletions(-)

[David Lehman]

On 01/15/2015 12:39 PM, mulhern wrote:
> Signed-off-by: mulhern
>
> blivet/formats/fs.py | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> index 6fede96..158b7c4 100644
> --- a/blivet/formats/fs.py
> +++ b/blivet/formats/fs.py
> @@ -1494,10 +1494,7 @@ class NoDevFS(FS):
>   @property
>   def type(self):
>    if self.device != self._type:
>        return self.device
>    else:
>        return self._type
>    return self.device
>
> @property
> def mountType(self):

Please make sure this does not cause issues with special naming of nodev 
devices in blivet.devicetree.getActiveMounts. You can test this by 
setting flags.include_nodev early in examples/list_devices.py.

David

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Tuesday, January 20, 2015 1:32:39 PM
> Subject: Re: [blivet:master 09/21] Simplify NoDevFS.type.
>
> On 01/15/2015 12:39 PM, mulhern wrote:
> > Signed-off-by: mulhern
> >
> > blivet/formats/fs.py | 5 +----
> >   1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> > index 6fede96..158b7c4 100644
> > --- a/blivet/formats/fs.py
> > +++ b/blivet/formats/fs.py
> > @@ -1494,10 +1494,7 @@ class NoDevFS(FS):
> >   @property
> >   def type(self):
> >    if self.device != self._type:
> >        return self.device
> >    else:
> >        return self._type
> >    return self.device
> >
> > @property
> > def mountType(self):
>
> Please make sure this does not cause issues with special naming of nodev
> devices in blivet.devicetree.getActiveMounts. You can test this by
> setting flags.include_nodev early in examples/list_devices.py.
>
> David
> _______________________________________________
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

We know that in the else branch, self.device == self._type, so,
if the only thing we're interested in is the value, it doesn't
matter which is returned on that branch. Returning self.device is simpler
and clearer.

If there was a plan to mutate the returned object, then which was returned would
matter. But that would be wrong, and the returned objects are strings,
which are immutable, so it should be safe.

- mulhern