Nir Soffer has posted comments on this change. Change subject: VolumeMetadata: Add from_lines factory method ......................................................................
Patch Set 2: Code-Review-1 (4 comments) Partial review https://gerrit.ovirt.org/#/c/52672/2/vdsm/storage/volume.py File vdsm/storage/volume.py: Line 169: TYPE_NETWORK = "network" Line 170: Line 171: Line 172: class VolumeMetadata(object): Line 173: # Metadata_key: (class_attribute, filter_fn) class_attribute? filter_fn? Maybe: attribute, validate_fn Line 174: MD_FIELDS = { Line 175: DOMAIN: ('sd_id', str), Line 176: IMAGE: ('img_id', str), Line 177: PUUID: ('parent_vol_id', str), Line 171: Line 172: class VolumeMetadata(object): Line 173: # Metadata_key: (class_attribute, filter_fn) Line 174: MD_FIELDS = { Line 175: DOMAIN: ('sd_id', str), str is not needed, everything is str when you read from storage. Maybe use None for strings? Line 176: IMAGE: ('img_id', str), Line 177: PUUID: ('parent_vol_id', str), Line 178: SIZE: ('size', int), Line 179: FORMAT: ('vol_format', str), Line 216: if key == POOL: # POOL is deprecated Line 217: continue Line 218: Line 219: try: Line 220: param = cls.MD_FIELDS[key.strip()][0] We must convert the value from storage to the expected type here, so an object will always have the correct type (.e.g not int as string). Line 221: except KeyError: Line 222: raise se.VolumeMetadataReadError("Invalid key: %s" % Line 223: key.strip()) Line 224: kwargs[param] = value.strip() Line 251: Return metadata in dictionary format Line 252: """ Line 253: ret = {} Line 254: for field, (attr, filter_fn) in self.MD_FIELDS.items(): Line 255: ret[field] = filter_fn(getattr(self, attr)) Do we really need to convert the variables here? format() is handling this. Line 256: ret[sd.DMDK_POOLS] = "" # Deprecated Line 257: return ret Line 258: Line 259: -- To view, visit https://gerrit.ovirt.org/52672 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7f63dce8e42e99d3240f4e916a9bd6ee5ee4b61 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
