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

David Lehman dlehman at redhat.com
Wed Jan 7 17:59:53 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.
>
>>
>> -        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?

Yes, it would. I was thinking of the _mountable class attribute. I'll 
use hasattr(self._format, "mountpoint").

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

Good grief. I'm still doing that after all this time.

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

Right. Both of these are results of my misconception that we append 
mountopts to the default mount options in _getOptions. I'll just use 
options throughout.

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