Change in vdsm[master]: Refactor metadata operations

nsoffer at redhat.com nsoffer at redhat.com
Fri Apr 15 21:29:38 UTC 2016


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 <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list