Change in vdsm[master]: [WIP] Added blockSD.getImageMap(), [block|file]SD.getAllVolu...
abaron at redhat.com
abaron at redhat.com
Mon Apr 23 09:39:18 UTC 2012
Ayal Baron has posted comments on this change.
Change subject: [WIP] Added blockSD.getImageMap(), [block|file]SD.getAllVolumes
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(16 inline comments)
....................................................
File vdsm/storage/image.py
Line 460: tSelfImage = tImgs[0]
pyflakes image.py
image.py:460: undefined name 'tImgs'
image.py:461: undefined name 'tImgs'
....................................................
File vdsm/storage/sp.py
Line 1717: # 4) if regular image template based remove image
s/0).*4).*/
In addition to removing image, this function also does the following:
If removing a template which has dependent images - creates a fake template
If removing the last image which depends on a fake template - removes the fake template as well
This should be in the docstring and not here.
Line 1718: # May be worth block file differentiation?
please remove this comment
Line 1719: allVols = sdDom.getAllVolumes()
why not pass image to filter by ?
Line 1721: volsImgs = dict((k, v) for k, v in allVols.iteritems() if imgUUID in v[0])
s/volsImgs/imagesByVol/ ?
Line 1722: if all(len(v[0]) == 1 for k, v in volsImgs.iteritems()):
Add comment:
Image has no dependent images and does not depend on other images so safe to delete
s/k/vol/
s/v[0]/v.images/
why does volImgs include the parent volume and not just the images?
Line 1723: # This is a self contained regular image, e.g. no template based
Entire comment is redundant
Line 1728: # TODO: image.delete should be a sdDom function
remove comment
Line 1730: # Deleting a template with dependencies or a template based image.
s/#.*/
This is either a template with derived images or a derived image so needs further scrutiny
Line 1733: # Template for an image should be only one!
Remove comment, log message should be self explanatory.
Line 1735: raise se.ImageValidationError("mismatched qty of templates"
Image points to multiple templates, cannot safely delete it.
Line 1738: # TODO: Lock the template, reload allVols.
just add the lock
Line 1742: tImgs = template[1][0]
s/template...
vol,((images),parent) = ts[0]
templateImage = images[0]
Line 1744: tSelfImage = tImgs[0]
remove
Line 1746: if numOfDependentImgs < 1:
remove this "if", this will always be false
Line 1764: # Fake templates are unknown by the manager, then it's real
remove last 2 lines of comment
--
To view, visit http://gerrit.ovirt.org/3462
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7eccf5ca100bd354aa09208ca60bb112fb697063
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
More information about the vdsm-patches
mailing list