Re: PATCH: pyblock: Make the dmraid table compare function a global function

[Joel Granados <jgranado redhat com>]

Patch looks good.  Only one comment: I would leave map_dev as an inside
function to compare_tables.  This would stress the point that it is
only used by this function.  While we could leave it outside for it to
be uesed, I would prefer to avoid using it as it is not general enough.
I mean, map_dev might imply mapping major,minor to a dev path, or a dev
path to a mount point or something different of what is currently
implemented.

regards.
On Tue, Feb 24, 2009 at 10:42:43PM +0100, Hans de Goede wrote:
> We are comparing tables in other places too, so make the dmraid table
> equal function a global function named compare_tables. This is a preparation
> patch for modifying the python code to match the C-code table handling changes.
> ---
>  device.py |  158 ++++++++++++++++++++++++++-----------------------------------
>  1 files changed, 67 insertions(+), 91 deletions(-)
>
> diff --git a/device.py b/device.py
> index 34a361a..aae1204 100644
> --- a/device.py
> +++ b/device.py
> @@ -48,6 +48,69 @@ def removeDeviceMap(map):
>          pass
>      map.remove()
>
> +def map_dev(path):
> +    if path[0] != '/':
> +        return path
> +
> +    try:
> +        statinfo = _os.stat(path)
> +        if not _stat.S_ISBLK(statinfo.st_mode):
> +            return path
> +
> +        return "%d:%d" % (statinfo.st_rdev/256, statinfo.st_rdev%256, )
> +    except:
> +        return path
> +
> +# Helper function for get_map
> +# The tables will be considered the same if, everything else being the
> +# same, they contain the same sets of devices.
> +def compare_tables(table1,  table2):
> +        table1 = str(table1).strip().split(' ')
> +        table2 = str(table2).strip().split(' ')
> +        table1sets = []
> +        table2sets = []
> +
> +        # We do this to avoid the index out of range exception
> +        if len(table1) != len(table2):
> +            return False
> +
> +        for i in range(len(table1)):
> +            d1 = map_dev(table1[i])
> +            d2 = map_dev(table2[i])
> +
> +            # when at least one changes its a device.
> +            if (table1[i] != d1) or (table2[i] != d2):
> +                # The d{1,2} will always have the major:minor string
> +                # We also need what comes after the dev, the offset.
> +                try:
> +                    table1sets.append("%s %s" % (d1, table1[i+1]))
> +                    table2sets.append("%s %s" % (d2, table2[i+1]))
> +                    i += 1
> +                except IndexError, msg:
> +                    # The device must have an offset, if not its nonesense
> +                    return False
> +                continue
> +
> +            # these are not devices
> +            if d1 == d2:
> +                continue
> +
> +            if d1 != d2:
> +                return False
> +
> +        # For mirror sets the devices can be in disorder.
> +        if table1[2] == "mirror":
> +            if set(table1sets) != set(table2sets):
> +                return False
> +
> +        # For none mirror the devs have to be in order.
> +        else:
> +            for i in range(len(table1sets)):
> +                if table1sets[i] != table2sets[i]:
> +                    return False
> +
> +        return True
> +
>  class BlockDev:
>      def get_major(self):
>          return self._BlockDev__device.major
> @@ -370,34 +433,11 @@ class MultiPath:
>
>          # we get "/dev/hda" from one and "3:0" from the other, so we have to
>          # fix up the device name
> -        def map_dev(path):
> +        def munge_dev(path):
>              if path[0] != '/':
>                  return path.strip()
>
> -            bd = path.strip()
> -            try:
> -                newpath=path.split('/')[-1]
> -                pos = 0
> -                dev = None
> -                num = None
> -                while pos < len(newpath):
> -                    if newpath[pos].isdigit():
> -                        dev = newpath[:pos]
> -                        num = newpath[pos:]
> -                        break
> -                    pos += 1
> -                if dev is None:
> -                    dev = newpath
> -                sysnewpath = None
> -                if num is None:
> -                    sysnewpath = '/sys/block/%s/dev' % (dev,)
> -                else:
> -                    sysnewpath = '/sys/block/%s/%s%s/dev' % (dev, dev, num)
> -                f = open(sysnewpath, 'r')
> -                l = f.readline()
> -                bd = l.strip()
> -            except:
> -                pass
> +            bd = map_dev(path)
>              # starting with 2.6.17-1.2510.fc6 or so, there's an implicit
>              # minimum IOs of 1000, which gets _added_ to the line "dmsetup ls"
>              # shows.  This sucks.
> @@ -406,7 +446,7 @@ class MultiPath:
>          tableParts = [0, self.size, 'multipath']
>
>          params = '0 0 1 1 round-robin 0 %s 1 %s' % (len(self.bdevs), \
> -                        _string.join(map(map_dev, self.bdevs)))
> +                        _string.join(map(munge_dev, self.bdevs)))
>          tableParts.append(params)
>
>          import dm as _dm
> @@ -658,69 +698,6 @@ class RaidSet:
>              pass
>      bdev = property(get_bdev, None, None, "block.BlockDev")
>
> -    # Helper function for get_map
> -    # The tables will be considered the same if, everything else being the
> -    # same, they contain the same sets of devices.
> -    def equal(self, table1,  table2):
> -        def map_dev(path):
> -            if path[0] != '/':
> -                return path
> -
> -            try:
> -                statinfo = _os.stat(path)
> -                if not _stat.S_ISBLK(statinfo.st_mode):
> -                    return path
> -
> -                return "%d:%d" % (statinfo.st_rdev/256, statinfo.st_rdev%256, )
> -            except:
> -                return path
> -
> -        table1 = table1.strip().split(' ')
> -        table2 = table2.strip().split(' ')
> -        table1sets = []
> -        table2sets = []
> -
> -        # We do this to avoid the index out of range exception
> -        if len(table1) != len(table2):
> -            return False
> -
> -        for i in range(len(table1)):
> -            d1 = map_dev(table1[i])
> -            d2 = map_dev(table2[i])
> -
> -            # when at least one changes its a device.
> -            if (table1[i] != d1) or (table2[i] != d2):
> -                # The d{1,2} will always have the major:minor string
> -                # We also need what comes after the dev, the offset.
> -                try:
> -                    table1sets.append("%s %s" % (d1, table1[i+1]))
> -                    table2sets.append("%s %s" % (d2, table2[i+1]))
> -                    i += 1
> -                except IndexError, msg:
> -                    # The device must have an offset, if not its nonesense
> -                    return False
> -                continue
> -
> -            # these are not devices
> -            if d1 == d2:
> -                continue
> -
> -            if d1 != d2:
> -                return False
> -
> -        # For mirror sets the devices can be in disorder.
> -        if self.rs.type == "mirror":
> -            if set(table1sets) != set(table2sets):
> -                return False
> -
> -        # For none mirror the devs have to be in order.
> -        else:
> -            for i in range(len(table1sets)):
> -                if table1sets[i] != table2sets[i]:
> -                    return False
> -
> -        return True
> -
>      def get_map(self):
>          if not self._RaidSet__map is None:
>              return self._RaidSet__map
> @@ -728,8 +705,7 @@ class RaidSet:
>          import dm as _dm
>
>          for map in _dm.maps():
> -            # XXX wtf?  why's it have a space at the end sometimes?
> -            if self.equal(str(map.table), str(self.rs.dmTable)):
> +            if compare_tables(map.table, self.rs.dmTable):
>                  self._RaidSet__map = map
>                  self.active = True
>                  del _dm
> -- 
> 1.6.1.3
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list redhat com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.