Dominik Holler has posted comments on this change.
Change subject: storage: prepare iscsi for IPv6 targets
......................................................................
Patch Set 11:
(3 comments)
https://gerrit.ovirt.org/#/c/65696/11/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
PS11, Line 59: getTargetString
How about 'portal_join'?
How about 'portal_join'?
- I try to respect the coding conventions currently used.
This file does not use the underscore_case convention, but a getCamleCase-like
convention.
- The 'portal' is just the hostname:port part of the String, without tpgt and
iqn.
On another thought, are you sure this func is needed?
Your are right. Since the function is very small, it's functionality could be easily
inlined in the callers. But from my point of view it is more maintainable to have the
creation of the portal and target string in centralized in just one place. For example if
the namedtuple IscsiPortal is changed in future, the creation of the portal string, which
may be part of the target string, has to be updated only here in this function.
Looks like the func is trying to find a common usage for
creating a portal, but it is not really common when you need
to use so many branches inside.
You are right, it is possible to separate a dedicated getPortalString(portal) function,
but this would not make the getTargetString() function significant simpler.
I think you can drop it and use hosttail_join at the callers.
If you confirm, that this is more easy to maintain from your point of view, I will do
this.
PS11, Line 60: if tpgt is None:
: tpgtStr = ""
: else:
: tpgtStr = ",%d" % tpgt
Python using the Hungarian notation is a bit too much for me :)
Good idea!
PS11, Line 71: str(portal.port)),
This one does not fit the previous line?
Why not? hosttail_join
wants the port number as a string, for this reason the caller has to convert the port
number to a string.
--
To view, visit
https://gerrit.ovirt.org/65696
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e1e1ffe1c5d09b7bc3767d84a99711986eaef97
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dominik Holler <dholler(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes