Alissa Bonas has posted comments on this change. Change subject: core: add check for iqn in addStorageServerConn ......................................................................
Patch Set 5: Liron - I checked the code, thanks for the reference. Regarding the port - you are probably right, I will add it in another patch, it doesn't have to be in this one. Regarding code similarity - the method in AddDisk checks validity of connections of a specific lun. Indeed it checks for iqn/connection/port of a connection, however the method loops on lun's connections and the error message also refers lun. In the current patch there are no luns, just a single standalone connection. There is similarity just in an if statement (if connection.getiqn is empty, etc) - I don't see much point to extract the code because of that single if, especially given the fact that the error message there mentions luns, and here the connection is standalone and is yet not related to any lun, so it will be incorrect to reuse same error message here and there. -- To view, visit http://gerrit.ovirt.org/15516 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide54ab2ec4a864dadd01cbe746df6592e32eab2f Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alissa Bonas <[email protected]> Gerrit-Reviewer: Alissa Bonas <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
