On Wed, Feb 18, 2009 at 07:32:17PM +0100, Hans de Goede wrote:
>
>
> Joel Granados Moreno wrote:
>> From: Joel Granados <joel.granados@xxxxxxxxx>
>>
>> modified: device.py: We need to compare table from dmraid and from
>> device-mapper in a more general way. It is possible to have an optional
>> argument in the table. So its better not to hardcode the possition of
>> the devices in the table string.
>> ---
>> device.py | 81 ++++++++++++++++++++++++++++++++++++++++++++----------------
>> 1 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/device.py b/device.py
>> index cbf196c..16c6b49 100644
>> --- a/device.py
>> +++ b/device.py
>> @@ -658,46 +658,83 @@ class RaidSet:
>> pass
>> bdev = property(get_bdev, None, None, "block.BlockDev")
>> - def get_map(self):
>> - if not self._RaidSet__map is None:
>> - return self._RaidSet__map
>> + # 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.
>> - # we get "/dev/hda" from one and "3:0" from the other, so we
>> have to
>> - # fix up the device name
>> + def equal(self, table1, table2):
>> def map_dev(path):
>> if path[0] != '/':
>> - return path.strip()
>> + return path
>> try:
>> statinfo = _os.stat(path)
>> if not _stat.S_ISBLK(statinfo.st_mode):
>> - return path.strip()
>> + return path
>> return "%d:%d" % (statinfo.st_rdev/256,
>> statinfo.st_rdev%256, )
>> except:
>> - return path.strip()
>> + return path
>> - parts = str(self.rs.dmTable).split(' ')
>> + table1 = table1.strip().split(' ')
>> + table2 = table2.strip().split(' ')
>> + table1sets = []
>> + table2sets = []
>> - table = []
>> - for part in parts:
>> - table += [map_dev(part),]
>> - tablestr = _string.join(table, ' ')
>> - # sometimes dmraid and dm have the disks of a mirror swapped
>> - if self.rs.dmTable.type == "mirror":
>> - tmp = table[8]
>> - table[8] = table[10]
>> - table[10] = tmp
>> - alt_tablestr = _string.join(table, ' ')
>> + # We do this to avoid the index out of range exception
>> + if len(table1) != len(table2):
>> + return False
>> +
>> + # We parse the first section before the list of devs.
>> + 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((d1, table1[i+1]))
>> + table2sets.append((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 (table1[i] == d1) and (table2[i] == d2):
>
> This test is the logical complement of the one above, so since the if
> block above ends in a continue, there is no need for this test.
This I already notice but left it there for readability. Guess if we
leave the comment it will maintain its readability. Will change.
>
>> + if d1 == d2:
>> + continue
>> +
>> + if d1 != d2:
>> + return False
>> +
>> + # For mirror sets the devices might be in disorder.
>> + if self.rs.type == "mirror":
>> + for elem in table1sets:
>> + if elem not in table2sets:
>> + return False
>> +
>
> This test is not 100% proof, what if table1 has 8:0 0, 8:0 0 and table 2
> has 8:0 0, 8:16 0 ?
>
> I know this sounds very unlikely and should never happen, but still. Howabout:
> ###
> if self.rs.type == "mirror" and len(table1sets) == 2:
> if table1sets[0] == table2sets[1] and table1sets[1] == table2sets[0]:
> return True
This wont hold when there is a mirror with 3 devs. Which I think is
equally unlikelly, but still....
will resend.
Thx for the review.
>
> for i in range(len(table1sets)):
> if (table1sets[i][1] != table2sets[i][1]) or\
> table1sets[i][2] != table2sets[i][2]:
> return False
> ###
>
> Note that the mirror with non swapped disks is handled by the regular case here
>
>
>> + # For none mirror the devs have to be in order.
>> else:
>> - alt_tablestr = None
>> + for i in range(len(table1sets)):
>> + if (table1sets[i][1] != table2sets[i][1]) or\
>> + table1sets[i][2] != table2sets[i][2]:
>> + return False
>> +
>> + return True
>> +
>> + def get_map(self):
>> + if not self._RaidSet__map is None:
>> + return self._RaidSet__map
>> import dm as _dm
>> for map in _dm.maps():
>> # XXX wtf? why's it have a space at the end sometimes?
>> - if str(map.table).strip() == tablestr or alt_tablestr and \
>> - str(map.table).strip() == alt_tablestr:
>> + if self.equal(str(map.table), str(self.rs.dmTable)):
>> self._RaidSet__map = map
>> self.active = True
>> del _dm
>
> Regards,
>
> Hans
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
Joel Andres Granados
Brno, Czech Republic, Red Hat.
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
