[PATCH 2/2] Use _netdev mount option as needed. (#1166509)

Anne Mulhern amulhern at redhat.com
Wed Jan 7 13:13:10 UTC 2015





----- Original Message -----
> From: "David Lehman" <dlehman at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Tuesday, January 6, 2015 3:15:35 PM
> Subject: [PATCH 2/2] Use _netdev mount option as needed. (#1166509)
> 
> The intention is to pass the option as needed when mounting filesystems
> in addition to putting it in /etc/fstab.
> ---
>  blivet/__init__.py          |  1 -
>  blivet/devices/btrfs.py     |  6 ++++--
>  blivet/devices/partition.py |  5 -----
>  blivet/devices/storage.py   | 20 ++++++++++++++++++++
>  tests/devices_test.py       | 30 ++++++++++++++++++++++++++++++
>  5 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index e785053..6742ca5 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -2803,7 +2803,6 @@ class FSSet(object):
>              options = options or "defaults"
>              for netdev in netdevs:
>                  if device.dependsOn(netdev):
> -                    options = options + ",_netdev"
>                      if root_on_netdev and mountpoint not in ["/", "/usr"]:
>                          options = options + ",x-initrd.mount"
>                      break
> diff --git a/blivet/devices/btrfs.py b/blivet/devices/btrfs.py
> index 13adbb4..9b5c9ca 100644
> --- a/blivet/devices/btrfs.py
> +++ b/blivet/devices/btrfs.py
> @@ -481,11 +481,13 @@ class BTRFSSubVolumeDevice(BTRFSDevice, RaidDevice):
>              return
>  
>          # propagate mount options specified for members via kickstart
> +        # start with options (not mountopts) so we don't lose the default
> mount
> +        # options
>          opts = "subvol=%s" % self.name
>          if self.volume.format.mountopts:
> -            opts = "%s,%s" % (self.volume.format.mountopts, opts)
> +            opts = "%s,%s" % (self.volume.format.options, opts)

I think the above changes don't change behavior, because you're only setting
opts if self.volume.format.mountopts. It would change behavior if opts was set
unconditionally, which may be the intention. 

>  
> -        self.format.mountopts = opts
> +        self.format.mountopts = "%s,%s" % (self.format.options, opts)

I think all above lines can be condensed to:

self.format.options = "%s,%s" % (self.volume.format.options, "subvol=%s" % self.name)

or some such thing (assuming my first conjecture is correct).

>  
>      @property
>      def volume(self):
> diff --git a/blivet/devices/partition.py b/blivet/devices/partition.py
> index 24f85b3..c4bd136 100644
> --- a/blivet/devices/partition.py
> +++ b/blivet/devices/partition.py
> @@ -408,11 +408,6 @@ class PartitionDevice(StorageDevice):
>          """ Is this device directly accessible? """
>          return self.isleaf and not self.isExtended
>  
> -    def _setFormat(self, fmt):
> -        """ Set the Device's format. """
> -        log_method_call(self, self.name)
> -        StorageDevice._setFormat(self, fmt)
> -
>      def _setBootable(self, bootable):
>          """ Set the bootable flag for this partition. """
>          if self.partedPartition:
> diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py
> index 7bcdc2f..962f83c 100644
> --- a/blivet/devices/storage.py
> +++ b/blivet/devices/storage.py
> @@ -38,6 +38,7 @@ import logging
>  log = logging.getLogger("blivet")
>  
>  from .device import Device
> +from .network import NetworkStorageDevice
>  
>  class StorageDevice(Device):
>      """ A generic storage device.
> @@ -511,6 +512,8 @@ class StorageDevice(Device):
>              self._partedDevice = None
>              self._targetSize = self.currentSize
>  
> +        self._updateNetDevMountOption()
> +
>      #
>      # destroy
>      #
> @@ -630,6 +633,23 @@ class StorageDevice(Device):
>  
>          self._format = fmt
>          self._format.device = self.path
> +        self._updateNetDevMountOption()
> +
> +    def _updateNetDevMountOption(self):

Could use a method docstring here.

> +        if not self._format.mountable:
> +            return

Would it be better just to check for presence of mountpoint attribute
or even not to check at all?

> +
> +        is_netdev = any(isinstance(a, NetworkStorageDevice)
> +                        for a in self.ancestors)
> +        has_netdev_option = "_netdev" in self._format.options.split(",")
> +        if not is_netdev and has_netdev_option:
> +            mountopts = self._format.mountopts.split(",")
> +            mountopts.remove("_netdev")
> +            self._format.mountopts = mountopts.join(",")

You want ",".join(mountopts) here.

And, I'm not sure why you use format.mountopts instead of format.options in the above.
Might be worth an explanatory comment.

> +        elif is_netdev and not has_netdev_option:
> +            # notice we're appending to options -- not mountopts -- so we
> get
> +            # the default options, too
> +            self._format.mountopts = self._format.options + ",_netdev"

I think you can assign to options instead of mountopts in the above code as well.

>  
>      def _getFormat(self):
>          return self._format
> diff --git a/tests/devices_test.py b/tests/devices_test.py
> index 3d7c28e..c526c74 100644
> --- a/tests/devices_test.py
> +++ b/tests/devices_test.py
> @@ -25,6 +25,7 @@ from blivet.devices import LVMVolumeGroupDevice
>  from blivet.devices import MDBiosRaidArrayDevice
>  from blivet.devices import MDContainerDevice
>  from blivet.devices import MDRaidArrayDevice
> +from blivet.devices import NetworkStorageDevice
>  from blivet.devices import OpticalDevice
>  from blivet.devices import PartitionDevice
>  from blivet.devices import StorageDevice
> @@ -1016,6 +1017,35 @@ class DeviceNameTestCase(unittest.TestCase):
>          for name in bad_names:
>              self.assertFalse(LVMLogicalVolumeDevice.isNameValid(name))
>  
> +class FakeNetDev(StorageDevice, NetworkStorageDevice):
> +    _type = "fakenetdev"
> +
> +class NetDevMountOptionTestCase(unittest.TestCase):
> +    def testNetDevSetting(self):
> +        """ Verify netdev mount option setting after format assignment. """
> +        netdev = FakeNetDev("net1")
> +        dev = StorageDevice("dev1", fmt=getFormat("ext4"))
> +        self.assertFalse("_netdev" in dev.format.options.split(","))
> +
> +        dev.parents.append(netdev)
> +        dev.format = getFormat("ext4")
> +        self.assertTrue("_netdev" in dev.format.options.split(","))
> +
> +    def testNetDevUpdate(self):
> +        """ Verify netdev mount option setting after device creation. """
> +        netdev = FakeNetDev("net1")
> +        dev = StorageDevice("dev1", fmt=getFormat("ext4"))
> +        self.assertFalse("_netdev" in dev.format.options.split(","))
> +
> +        dev.parents.append(netdev)
> +
> +        # these create methods shouldn't write anything to disk
> +        netdev.create()
> +        dev.create()
> +
> +        self.assertTrue("_netdev" in dev.format.options.split(","))
> +
> +
>  if __name__ == "__main__":
>      unittest.main()
>  
> --
> 1.9.3
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

- mulhern


More information about the anaconda-patches mailing list