Ayal Baron has posted comments on this change.
Change subject: BZ#788640 - Check move image conditions from SD data.
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(12 inline comments)
....................................................
Commit Message
Line 7: BZ#788640 - Check move image conditions from SD data.
what does that mean? why?
....................................................
File vdsm/storage/hsm.py
Line 1300: destination SD.
s/*/
Moving an image based on a template to a data domain is only allowed if the
template exists on the target domain.
Moving a template from a data domain is only allowed if there are no images
based on it in the source data domain.
Line 1307: if imgUUID in v.imgs)
isn't this filter repeated in several places? seeing as it is not trivially
clear what it does, maybe have a function with a clear name and proper comment
that does this?
Line 1308: if any(len(imgs) > 1 for imgs in srcVolsImgs.itervalues()):
imageFromVolume = any(len...)
if imageFromVolume:
get rid of comment
Line 1312: if len(imgs) > 1:
instead of comment above and if len(imgs):
isTemplate = len(imgs) > 1
if isTemplate:
Line 1313: # This is the template. Should be only one.
comment is redundant
Line 1316: # Template self image is the 1st entry
To make it clear you can change the following to:
movingTemplate = (imgUUID == tImgs[0])
templateInDestination = tname in dstAllVols.keys()
if not movingTemplate and not templateInDestination:
Line 1317: if imgUUID != tImgs[0] and tName not in
dstAllVols.keys():
looking at the above:
if any...:
for...
the if any is redundant, just do the for list and instead of "break" continue
if len(imgs) <= 1
Line 1321: elif imgUUID == tImgs[0] and not srcDom.isBackup():
assuming above change:
elif movingTemplate and not ...
Line 1341: self.isImageMoveLegal(srcDom, dstDom, imgUUID)
you're not checking the return value hence the name of the function should be:
validateImageMove ('is' indicates a boolean which needs to be checked)
Line 1356: # Therefore there is no special semantics for other SDs types as
backup.
comment is unclear and redundant.
Line 1374: # Validate the conditions for each move
comment is redundant
--
To view, visit http://gerrit.ovirt.org/3466
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2422217717fe0f3cb668b4dc5f775e4a971c8d9b
Gerrit-PatchSet: 7
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]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/vdsm-patches
