Fixes a bug where mountpoint of btrfs top-level volume was not found in mountsCache.
Two parts of fix are: * Make MAIN_VOLUME_ID default subvolspec for btrfs format. If making a subvol it should always be overridden with subvol's subvolspec. * Make mountsCache use consistent types for subvolspec in keys.
Symptom of bug was uncaught RuntimeError "btrfs subvol create requires mounted volume" while executing storage actions.
From: mulhern amulhern@redhat.com
Externally to the mountsCache it seems that sometimes the subvolspec is a str, sometimes an int. Make mountsCache store and retrieve treating subvolspec always as a str.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/mounts.py | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/blivet/mounts.py b/blivet/mounts.py index d7d6851..46bc894 100644 --- a/blivet/mounts.py +++ b/blivet/mounts.py @@ -21,6 +21,7 @@ # from collections import defaultdict from . import util +from .devicelibs import btrfs
import logging log = logging.getLogger("blivet") @@ -40,7 +41,7 @@ def getMountpoints(self, devspec, subvolspec=None): :param devscpec: device specification, eg. "/dev/vda1" :type devspec: str :param subvolspec: btrfs subvolume specification, eg. ID or name - :type subvolspec: str + :type subvolspec: object (may be NoneType) :returns: list of mountpoints (path) :rtype: list of str or empty list
@@ -50,6 +51,9 @@ def getMountpoints(self, devspec, subvolspec=None): """ self._cacheCheck()
+ if subvolspec is not None: + subvolspec = str(subvolspec) + return self.mountpoints[(devspec, subvolspec)]
def isMountpoint(self, path): @@ -61,6 +65,23 @@ def isMountpoint(self, path):
return any(path in p for p in self.mountpoints.values())
+ def _getSubvolSpec(self, devspec, mountpoint): + """ Get the subvolume specification for this btrfs volume. + + :param str devspec: the device specification + :param str mountpoint: the mountpoint + + :returns: the subvolume specification, 5 for a top-level volume + :rtype: str or NoneType + """ + for line in open("/proc/self/mountinfo").readlines(): + fields = line.split() + if fields[4] == mountpoint and fields[9] == devspec: + # empty _subvol[1:] means it is a top-level volume + subvolspec = fields[3] + return subvolspec[1:] or str(btrfs.MAIN_VOLUME_ID) + return None + def _getActiveMounts(self): """ Get information about mounted devices from /proc/mounts and /proc/self/mountinfo @@ -70,24 +91,17 @@ def _getActiveMounts(self): self.mountpoints = defaultdict(list) for line in open("/proc/mounts").readlines(): try: - (devspec, mountpoint, fstype, _options, _rest) = line.split(None, 4) + (devspec, mountpoint, fstype, _rest) = line.split(None, 3) except ValueError: log.error("failed to parse /proc/mounts line: %s", line) continue
if fstype == "btrfs": - # get the subvol name from /proc/self/mountinfo - for line in open("/proc/self/mountinfo").readlines(): - fields = line.split() - _subvol = fields[3] - _mountpoint = fields[4] - _devspec = fields[9] - if _mountpoint == mountpoint and _devspec == devspec: - # empty _subvol[1:] means it is a top-level volume - subvolspec = _subvol[1:] or 5 - - self.mountpoints[(devspec, subvolspec)].append(mountpoint) - + subvolspec = self._getSubvolSpec(devspec, mountpoint) + if subvolspec is not None: + self.mountpoints[(devspec, subvolspec)].append(mountpoint) + else: + log.error("failed to obtain subvolspec for btrfs device %s", devspec) else: self.mountpoints[(devspec, None)].append(mountpoint)
In reply to line 44 of blivet/mounts.py:
Why not ``str or NoneType`` as that's what we expect?
In reply to line 77 of blivet/mounts.py:
Two things here: * the file is not closed anywhere (can be addressed by using ``with open() as f:`` * the ``readlines()`` call is not needed (the file object is an iterable producing lines)
In reply to line 79 of blivet/mounts.py:
A comment with an example line would make the split and comparisons way clearer (I know the original version of this code didn't have it either).
In reply to line 44 of blivet/mounts.py:
Can not always rely on it being a str AFAICT from examining related BTRFS code.
In reply to line 44 of blivet/mounts.py:
OK, thanks for the clarification.
From: mulhern amulhern@redhat.com
Signed-off-by: mulhern amulhern@redhat.com --- 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 dd282eb..f9ec7ac 100644 --- a/blivet/formats/fs.py +++ b/blivet/formats/fs.py @@ -40,6 +40,7 @@ from ..i18n import _, N_ from .. import udev from ..mounts import mountsCache +from ..devicelibs import btrfs
import logging log = logging.getLogger("blivet") @@ -1157,7 +1158,7 @@ class BTRFS(FS): def __init__(self, **kwargs): super(BTRFS, self).__init__(**kwargs) self.volUUID = kwargs.pop("volUUID", None) - self.subvolspec = kwargs.pop("subvolspec", None) + self.subvolspec = kwargs.pop("subvolspec", btrfs.MAIN_VOLUME_ID)
def create(self, **kwargs): # filesystem creation is done in blockdev.btrfs.create_volume
Other than the minor comments above this looks good to me.
Added label: ACK.
Closed.
anaconda-patches@lists.fedorahosted.org