[blivet:rhel7/master 2/2] Check if device has enough members when setting RAID level (#1019685)

Anne Mulhern amulhern at redhat.com
Fri Sep 12 12:14:05 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Friday, September 12, 2014 2:30:26 AM
> Subject: Re: [blivet:rhel7/master 2/2] Check if device has enough members	when setting RAID level (#1019685)
> 
> On Thu, 2014-09-11 at 13:34 -0400, mulhern wrote:
> > Related: rhbz#1019685
> > 
> > RAID levels have minimum member requirements. Level setters now raise a
> > BTRFSValueError if device does not have enough members to meet the minimum
> > requirement. However, if the device exists its members may not be known
> > yet, so the check is omitted.
> > 
> > Also, RaidError is rethrown as BTRFSValueError in level setter if the level
> > is not valid for its use. Previously the RaidError was just propagated.
> > 
> > Signed-off-by: mulhern <amulhern at redhat.com>
> > ---
> >  blivet/devices.py | 62
> >  ++++++++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 52 insertions(+), 10 deletions(-)
> > 
> > diff --git a/blivet/devices.py b/blivet/devices.py
> > index fe87226..0e02396 100644
> > --- a/blivet/devices.py
> > +++ b/blivet/devices.py
> > @@ -5127,6 +5127,50 @@ class BTRFSVolumeDevice(BTRFSDevice,
> > ContainerDevice):
> >  
> >          self._defaultSubVolumeID = None
> >  
> > +    def _validateRaidLevel(self, level):
> > +        """ Returns an error message if the RAID level is invalid for this
> > +            device, otherwise None.
> > +
> > +            :param level: a RAID level
> > +            :type level: :class:`~.devicelibs.raid.RAIDLevel`
> > +            :returns: an error message if the RAID level is invalid, else
> > None
> > +            :rtype: str or NoneType
> > +        """
> > +        if not self.exists and len(self.parents) < level.min_members:
> > +            return "RAID level %s requires that device have at least %d
> > members, but device has only %d members." % (level, level.min_members,
> > len(self.parents))
> > +        return None
> > +
> > +    def _setLevel(self, value, data):
> > +        """ Sets a valid level for this device and level type.
> > +
> > +            :param object value: value for this RAID level
> > +            :param bool data: True if for data, False if for metadata
> > +
> > +            :returns: a valid level for value, if any, else None
> > +            :rtype: :class:`~.devicelibs.raid.RAIDLevel` or NoneType
> > +
> > +            :raises: :class:`~.errors.BTRFSValueError` if value represents
> > +            an invalid level.
> > +        """
> > +        level = None
> > +        if value:
> > +            try:
> > +                levels = btrfs.RAID_levels if data else
> > btrfs.metadata_levels
> > +                level = levels.raidLevel(value)
> > +            except errors.RaidError:
> > +                data_type_str = "data" if data else "metadata"
> > +                raise errors.BTRFSValueError("%s is an invalid value for
> > %s RAID level." % (value, data_type_str))
> > +
> > +            error_msg = self._validateRaidLevel(level)
> > +            if error_msg:
> > +                raise errors.BTRFSValueError(error_msg)
> > +
> > +        # pylint: disable=attribute-defined-outside-init
> Why not to define these in __init__?
 
We actually do define these in __init__, not directly, but
transitively, since we always assign to self.dataLevel and
self.metatDataLevel in __init__.

As far as I knew at the time, pylint analysis would, however,
never be strong enough to detect this fact, so I put this
annotation here, instead of screening it out in false-positives.

Turns out, though, that they actually believe that they've fixed
the problem in recent version of pylint
(https://bitbucket.org/logilab/pylint/issue/192/w0201-message-may-be-a-little-too-general),
so soon we may be able to get rid of this annotation altogether.
I'ld still like to keep it here, however, because I haven't yet seen whether
their analysis is successful.

Meanwhile, we're confident that either these attributes are set in the object or the
object is not created, which is just what we want so we don't ever get an AttributeError
when accessing them.


> > +        if data:
> > +            self._dataLevel = level
> > +        else:
> > +            self._metaDataLevel = level
> > +
> >      @property
> >      def dataLevel(self):
> >          """ Return the RAID level for data.
> > @@ -5140,12 +5184,11 @@ class BTRFSVolumeDevice(BTRFSDevice,
> > ContainerDevice):
> >      def dataLevel(self, value):
> >          """ Set the RAID level for data.
> >  
> > -            :param value: new raid level
> > -            :param type:  a valid raid level descriptor
> > -            :returns:     None
> > +            :param object value: new raid level
> > +            :returns: None
> > +            :raises: :class:`~.errors.BTRFSValueError`
> >          """
> > -        # pylint: disable=attribute-defined-outside-init
> > -        self._dataLevel = btrfs.RAID_levels.raidLevel(value) if value else
> > None
> > +        self._setLevel(value, True)
> >  
> >      @property
> >      def metaDataLevel(self):
> > @@ -5160,12 +5203,11 @@ class BTRFSVolumeDevice(BTRFSDevice,
> > ContainerDevice):
> >      def metaDataLevel(self, value):
> >          """ Set the RAID level for metadata.
> >  
> > -            :param value: new raid level
> > -            :param type:  a valid raid level descriptor
> > -            :returns:     None
> > +            :param object value: new raid level
> > +            :returns: None
> > +            :raises: :class:`~.errors.BTRFSValueError`
> >          """
> > -        # pylint: disable=attribute-defined-outside-init
> > -        self._metaDataLevel = btrfs.metadata_levels.raidLevel(value) if
> > value else None
> > +        self._setLevel(value, False)
> >  
> >      @property
> >      def formatImmutable(self):
> 
> --
> Vratislav Podzimek
> 
> Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> 
> _______________________________________________
> 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