Frank Kobzik has posted comments on this change. Change subject: backend: Control virtio rng device ......................................................................
Patch Set 12: (5 comments) http://gerrit.ovirt.org/#/c/18176/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractRngDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractRngDeviceCommand.java: Line 24: Line 25: protected AbstractRngDeviceCommand(T parameters) { Line 26: super(parameters); Line 27: if (parameters.getRngDevice() == null) { Line 28: throw new IllegalArgumentException("RNG Device must be specified."); > there is no need to throw exceptions in the constructor, we have 2 infrastr Done Line 29: } Line 30: Line 31: Guid vmId = parameters.getRngDevice().getVmId(); Line 32: setVmId(vmId); http://gerrit.ovirt.org/#/c/18176/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetRngDeviceQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetRngDeviceQuery.java: Line 14: } Line 15: Line 16: @Override Line 17: protected void executeQueryCommand() { Line 18: final List<VmDevice> vmDevices = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdAndType(getParameters().getId(), > since this is a user query i think you need to use filtering and user id, n Ok, it's probably not a user query. It is used from commands that regular user is not permitted to use. Line 19: VmDeviceGeneralType.RNG); Line 20: Line 21: if (vmDevices != null && !vmDevices.isEmpty()) { Line 22: VmDevice dev = vmDevices.get(0); http://gerrit.ovirt.org/#/c/18176/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 133: } Line 134: } Line 135: Line 136: if (rngCommandResult != null && !rngCommandResult.getSucceeded()) { Line 137: getReturnValue().setSucceeded(false); > its good that you set the succeeded to false, but it will be overridden out Ok, I tried to solve this in new patchset - I set the succeeded attribute of UpdateVmCommand according to result of updateRngDevice. Just a question: in nonhappy scenario (exception thrown in executeVmCommand or succeeded == false) the changes done by UpdateVmCommand should be compensated, am I correct? Line 138: } Line 139: } Line 140: } Line 141: http://gerrit.ovirt.org/#/c/18176/12/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateRngDeviceTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateRngDeviceTest.java: Line 1: //package org.ovirt.engine.core.bll; > if this file belong to the next patch, just add it there.. I forgot to handle this test. I'm moving it to the follow up. Line 2: // Line 3: //import java.util.Collections; Line 4: //import org.junit.Assert; Line 5: //import org.junit.Test; http://gerrit.ovirt.org/#/c/18176/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RngDeviceParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RngDeviceParameters.java: Line 11: isVm = true; Line 12: } Line 13: Line 14: public RngDeviceParameters(VmRngDevice rngDevice, boolean vm) { Line 15: if (rngDevice == null) { > please no exceptions in the constructor Done Line 16: throw new IllegalArgumentException("RNG Device must be specified."); Line 17: } Line 18: this.rngDevice = rngDevice; Line 19: isVm = vm; -- To view, visit http://gerrit.ovirt.org/18176 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc26a484c84244074426f4b0c790e2cc4ebb8da6 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@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
