Ayal Baron has posted comments on this change.
Change subject: ISCSI Subsystem refactoring
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(21 inline comments)
mostly nitpicking which I don't mind fixed in a different patch.
-1 is due to the questions in iscsi.py (functionality and bc)
Note that due to the shear size I did not compare with old code to make sure
everything is supported properly
....................................................
Commit Message
Line 12: This refactoring ties to:
s/ties/tries
Line 18: actually property of the interface and not it's identity
actually "a"
s/it's/its/
Line 20: now
is there a newline here?
Line 27: work to support authentication methods other then CHAP.
s/then/than/
Line 33: code path to preserve backward compatibility.
what backward compatibility? it was never used.
Line 36: - tpgt is ignores. This is in line with old VDSM behaviour.
s/ignores/ignored/ ?
....................................................
File vdsm/storage/hsm.py
Line 2304: # I unparse the response. Why you ask? Backward
compatibility! At
unparse? what does that even mean?
In fact what you're doing is parsing.
....................................................
File vdsm/storage/iscsiadm.py
Line 49: def iface_exists(interfaceName):
underscore? same for all the rest
....................................................
File vdsm/storage/iscsi.py
Line 68: IscsiTarget(IscsiPortal("", 0), 0, ""), None)
why do the ugliness here and not where this function is called?
Line 72: sessionDir = glob.glob("/sys/devices/platform/host*/%s/" %
sessionName)
what's the difference between this 'sessionDir' and the ones in getDevIscsiInfo
and iterateIscsiSessions?
If non, why is it not passed as a param into this function?
If there is a difference then the naming is confusing
Line 89: conn_pattern = os.path.join(sessionDir, "connection*")
underscore?
Line 91: connectiondir = glob.glob(conn_pattern)[0]
capital D?
Line 169: # this is just a to be polite we don't really care
s/a //
Line 176: key = "discovery.sendtargets." + key
'.'.join("discovery.sendtargets", key) ?
if so then also above
Line 190: # discoveries at once will cause unpredicrable results
s/unpredictable/unpredictable/
Line 195: addIscsiPortal(iface, portal, credentials)
what happens if portal is already in iscsiadm db?
Line 199: deleteIscsiPortal(iface, portal)
what happens if portal was already in iscsiadm db before adding it above? is
this the proper behaviour?
Line 204: # returns here has it's IP resolved!
s/it's/its/
how do we correlate the resolved IP with the one passed? are returned targets
limited to this ip only? and if so, how do we support backward compatibility?
Line 210: for sessionDir in
glob.iglob("/sys/devices/platform/host*/session*/iscsi_session/session?*"):
no constant for sessionDirPattern? iscsiSessionPattern?
Line 316: # If VDSM crashes while creating an interface a grabages
interface will
s/garbages/garbage/
Line 329: # Suggestions are welcome.
maybe libiscsi supports setting this atomically
--
To view, visit http://gerrit.ovirt.org/1607
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b5f99598a559731951c271e97f04f03a2d2173a
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: shu ming <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/vdsm-patches
