Liron Aravot has posted comments on this change. Change subject: core: change return value of AddDisk ......................................................................
Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/36425/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java: Line 452: Line 453: if (tmpRetValue.getActionReturnValue() != null) { Line 454: DiskImage diskImage = (DiskImage) tmpRetValue.getActionReturnValue(); Line 455: addDiskPermissions(diskImage); Line 456: getReturnValue().setActionReturnValue(isExecutedAsChildCommand() ? diskImage : diskImage.getId()); > I agree that it is confusing, but the motivation is that I want to use this I'm not against changing the return value, but if we do change it i think it should be changed for all cases and not only for internal execution. Currently, at least imo we introduce vulnerable code -for example ,If one changes the execution in the caller to be not as child command (which seems trivial) - the code will break as the expected return value is different than what is being returned. Line 457: } Line 458: getReturnValue().setFault(tmpRetValue.getFault()); Line 459: setSucceeded(tmpRetValue.getSucceeded()); Line 460: } -- To view, visit http://gerrit.ovirt.org/36425 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a41d57bfb1bf48f0a2c6a3703e612b27da50db8 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
