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 <dhol...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Dominik Holler <dhol...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <mmire...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Kaul <yk...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
