[PATCH 2/2] Use _netdev mount option as needed. (#1166509)
David Lehman
dlehman at redhat.com
Wed Jan 7 17:53:24 UTC 2015
On 01/07/2015 07:13 AM, Anne Mulhern wrote:
>
>
>
>
> ----- 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.
When I wrote it I mistakenly thought we appended mountopts to the
default mount options in _getOptions. We do not, so this is a no-op.
Dropping the whole btrfs.py portion of the patch. It probably shouldn't
have been included at all given that its only relation is that it also
fiddles with mount options.
David
>
>>
>> - 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
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
More information about the anaconda-patches
mailing list