Hi Stratos Sorry, many style issues. See inline comments.
2013/1/30 Stratos Psomadakis <[email protected]>: > --- a/lib/bdev.py > +++ b/lib/bdev.py > @@ -39,6 +39,7 @@ from ganeti import compat > from ganeti import netutils > from ganeti import pathutils > > +import simplejson Why not ganeti.serializer? > + def _VolumeToBlockdev(pool, volume_name): > + """Do the 'volume name'-to-'rbd block device' resolving. > + > + @type pool: string > + @param pool: RADOS pool to use > + @type volume_name: string > + @param volume_name: the name of the volume whose device we search for > + > + """ > + # Newer versions of the rbd tool support json output formatting. Use it > if > + # available. > + showmap_cmd = [ > + constants.RBD_CMD, > + "showmapped", "-p", pool, "--format=json" > + ] Formatting: http://code.google.com/p/ganeti/wiki/StyleGuide#Formatting_sequences > + result = utils.RunCmd(showmap_cmd) > + if result.failed: > + # For older versions of rbd, we have to parse the output manually. > + showmap_cmd = [constants.RBD_CMD, "showmapped", "-p", pool] > + result = utils.RunCmd(showmap_cmd) > + if result.failed: > + _ThrowError("rbd showmapped failed (%s): %s", > + result.fail_reason, result.output) > + > + rbd_dev = RADOSBlockDevice._ParseRbdShowmappedOutput(result.output, > + volume_name) Please make “_VolumeToBlockdev” a classmethod, then you can write: rbd_dev = \ cls._ParseRbdShowmappedOutput(result.output, volume_name) > + return rbd_dev > + > + > + try: Only one empty line. > + devices = simplejson.loads(result.output) > + except simplejson.JSONDecodeError: > + _ThrowError("Invalid json input %s!\n", result.output) No newlines in error messages please (also no punctuation or full sentences). > + rbd_dev = None > + # pylint: disable=E1103 Don't you want to limit this to a specific line or block? Right now it applies to the whole method. > + for d in devices.itervalues(): Please use “values()”, not “itervalues()” (the latter does not longer exist in Python 3). > + if "name" not in d: > + _ThrowError("'name' key missing from json object %s!\n", > + devices) > + > + if d["name"] == volume_name: I assume “name” missing is a rare case: try: name = d["name"] except KeyError: _ThrowError("…") else: if name == volume_name: … > + if rbd_dev is not None: > + _ThrowError("The rbd volume %s is mapped more than once." > + " This shouldn't happen, try to unmap the extra" > + " devices manually.", volume_name) See above for error message formatting. > + > + rbd_dev = d["device"] > + > + return rbd_dev > + > + > + @staticmethod > def _ParseRbdShowmappedOutput(output, volume_name): > """Parse the output of `rbd showmapped'. > > @@ -2781,21 +2823,25 @@ class RADOSBlockDevice(BlockDev): > volumefield = 2 > devicefield = 4 > > - field_sep = "\t" > - > lines = output.splitlines() > - splitted_lines = map(lambda l: l.split(field_sep), lines) > > - # Check empty output. > + # Try parsing the new output format (ceph >= 0.55). > + splitted_lines = map(lambda l: l.split(), lines) > + > + # Check for empty output. > if not splitted_lines: > - _ThrowError("rbd showmapped returned empty output") > + return None > > # Check showmapped header line, to determine number of fields. > field_cnt = len(splitted_lines[0]) > if field_cnt != allfields: > - _ThrowError("Cannot parse rbd showmapped output because its format" > - " seems to have changed; expected %s fields, found %s", > - allfields, field_cnt) > + # Parsing the new format failed. Fallback to parsing the old output > + # format (< 0.55). > + splitted_lines = map(lambda l: l.split('\t'), lines) Use "" for strings, not ''. > + if field_cnt != allfields: > + _ThrowError("Cannot parse rbd showmapped output because its format" > + " seems to have changed; expected %s fields, found %s", > + allfields, field_cnt) Michael
