Omer Frenkel has posted comments on this change. Change subject: core: VmDeviceUtils refactoring ......................................................................
Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/41620/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java: Line 60: /* Line 61: * CD-ROM device Line 62: */ Line 63: Line 64: private static String getCdPath(String srcCdPath, String dstCdPath) { please add a doc what this method do, the logic is not clear from the name Line 65: if (!StringUtils.isEmpty(dstCdPath)) { Line 66: return dstCdPath; Line 67: } else if (!StringUtils.isEmpty(srcCdPath)) { Line 68: return srcCdPath; Line 178: if (!hasSmartcardDevice(vmId)) { Line 179: addSmartcardDevice(vmId); Line 180: } Line 181: } else { Line 182: removeSmartcardDevices(vmId); although this code is nicer, it will cause going to the db twice to get the devices: first to check if exists, and again to remove, where before it goes only once, i think we can change this a little that the method will do only one call (this is relevant for other device types below Line 183: } Line 184: } Line 185: Line 186: /** Line 200: * Remove all smartcard devices from the VM. Line 201: * Line 202: * @param vmId Line 203: */ Line 204: public static void removeSmartcardDevices(Guid vmId) { not sure if this should be public or not although its a "service" method that can be used from outside, when its private its easier to track that this is used only internally Line 205: removeVmDevices(getSmartcardDevices(vmId)); Line 206: } Line 207: Line 208: /** -- To view, visit https://gerrit.ovirt.org/41620 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iffa1ae3176b4082ef3b96277f6ea351e82f3e613 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud <smela...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shmuel Melamud <smela...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
