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 <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/vdsm-patches
