Nir Soffer has posted comments on this change. Change subject: Refactor metadata operations ......................................................................
Patch Set 2: (8 comments) https://gerrit.ovirt.org/#/c/52671/2/vdsm/storage/sdm/volume_artifacts.py File vdsm/storage/sdm/volume_artifacts.py: Line 257: volume.type2name(volume.LEAF_VOL), Line 258: disk_type, Line 259: desc, Line 260: volume.LEGAL_VOL) Line 261: self._oop.writeLines(self.meta_volatile_path, meta.format()) You are passing string, and this function seems to expect lines. Hopefully we have another function writing data - if not, we can add it. Line 262: Line 263: def _create_lease_file(self): Line 264: if self.sd_manifest.hasVolumeLeases(): Line 265: meta_id = (self.volume_path,) https://gerrit.ovirt.org/#/c/52671/2/vdsm/storage/volume.py File vdsm/storage/volume.py: Line 174: Line 175: def __init__(self, sd_id, img_id, parent_vol_id, size, vol_format, Line 176: prealloc, vol_type, disk_type, description="", Line 177: legality=ILLEGAL_VOL): Line 178: self.sd_id = str(sd_id) Why convert to string? it should be a string. Line 179: self.img_id = str(img_id) Line 180: self.parent_vol_id = str(parent_vol_id) Line 181: self.size = int(size) Line 182: self.vol_format = str(vol_format) Line 177: legality=ILLEGAL_VOL): Line 178: self.sd_id = str(sd_id) Line 179: self.img_id = str(img_id) Line 180: self.parent_vol_id = str(parent_vol_id) Line 181: self.size = int(size) Why not require integer value? We should require the correct type in __init__. We will convert to string when formating, and when reading from storage, back to the correct type. Line 182: self.vol_format = str(vol_format) Line 183: self.prealloc = str(prealloc) Line 184: self.vol_type = str(vol_type) Line 185: self.disk_type = str(disk_type) Line 185: self.disk_type = str(disk_type) Line 186: self.description = description Line 187: self.legality = str(legality) Line 188: self.ctime = int(time.time()) Line 189: self.mtime = 0 There is no point in keeping constant value as instance variable. We can use a read only property instead: @property def mtime(self): return 0 And if we don't access this in any code, just hardcode the value in the info(). Line 190: Line 191: @property Line 192: def description(self): Line 193: return self._description Line 193: return self._description Line 194: Line 195: @description.setter Line 196: def description(self, desc): Line 197: self._description = VolumeManifest.validateDescription(desc) Can we move the validation code to this class and use this class in VolumeManifest? Line 198: Line 199: def format(self): Line 200: """ Line 201: Format metadata string in storage format. Line 201: Format metadata string in storage format. Line 202: Line 203: Raises MetadataOverflowError if formatted metadata is too long. Line 204: """ Line 205: lines = ["%s=%s\n" % (key.strip(), str(value).strip()) Why strip? we are using info(), which should not return extra whitespace. We also do not need to convert to string, as "%s" will convert any type to string. Line 206: for key, value in self.info().iteritems()] Line 207: lines.append("EOF\n") Line 208: data = "".join(lines) Line 209: if len(data) > METADATA_SIZE: Line 202: Line 203: Raises MetadataOverflowError if formatted metadata is too long. Line 204: """ Line 205: lines = ["%s=%s\n" % (key.strip(), str(value).strip()) Line 206: for key, value in self.info().iteritems()] iteritems will not work in python3, the portable solution is: for key, value in six.iteritems(self.info()) Line 207: lines.append("EOF\n") Line 208: data = "".join(lines) Line 209: if len(data) > METADATA_SIZE: Line 210: raise se.MetadataOverflowError(data) Line 209: if len(data) > METADATA_SIZE: Line 210: raise se.MetadataOverflowError(data) Line 211: return data Line 212: Line 213: def info(self): Do we use this dict in old code? If not, we don't have to convert value to string if they are not strings. Line 214: """ Line 215: Return metadata in dictionary format Line 216: """ Line 217: return { -- To view, visit https://gerrit.ovirt.org/52671 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4fee56b30c13a3c1cede8489338ed60f4e1d5eab Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
