Roy Golan has posted comments on this change. Change subject: backend: Control virtio rng device ......................................................................
Patch Set 14: (6 comments) http://gerrit.ovirt.org/#/c/18176/14/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 25: protected AbstractRngDeviceCommand(T parameters) { Line 26: super(parameters); Line 27: Line 28: if (parameters.getRngDevice() == null) { Line 29: cachedEntity = null; > this is because i wanted the attributes to be final. if you want to, i can but if rng device is null then no point in continuing no? Line 30: cachedRngDevices = null; Line 31: return; Line 32: } Line 33: Line 57: return permissionList; Line 58: } Line 59: Line 60: @Override Line 61: protected boolean canDoAction() { perfect candidate for bean validation on the parameter class Line 62: if (getParameters().getRngDevice() == null) { Line 63: return failCanDoAction(VdcBllMessages.ACTION_TYPE_RNG_MUST_BE_SPECIFIED); Line 64: } Line 65: http://gerrit.ovirt.org/#/c/18176/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 746: throw > i believe adding the message is done in AddRngDeviceCommand class in canDoA 2 issues: 1. minor but for correctness - the exception used isn't correct for this case. we don't have a "state" problem rather an argument - the rng device is problematic in some way 2. the message is there but its kept result variable. no one knows what happen. just log it in some way http://gerrit.ovirt.org/#/c/18176/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmRngDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmRngDevice.java: Line 7: import java.util.Map; Line 8: import java.util.Set; Line 9: import org.ovirt.engine.core.common.utils.VmDeviceType; Line 10: import org.ovirt.engine.core.compat.Guid; Line 11: can u add javadoc with short description of what source is? Line 12: /** Line 13: * This class represents paravirtualized rng device. Line 14: */ Line 15: public class VmRngDevice extends VmDevice implements Serializable { Line 15: public class VmRngDevice extends VmDevice implements Serializable { Line 16: Line 17: public enum Source { Line 18: RANDOM("random"), Line 19: HWRNG("hwrng"); > well, we could use these. i just don't like relying on textual representati yes please. its way simpler and less code actually. if we should create a value only if its really different than the constant name. after all enums are exactly for that why not use it? Line 20: Line 21: private final String val; Line 22: Line 23: private Source(String val) { http://gerrit.ovirt.org/#/c/18176/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceType.java: Line 22: CHANNEL("channel"), Line 23: SMARTCARD("smartcard"), Line 24: BALLOON("balloon"), Line 25: CONSOLE("console"), Line 26: VIRTIO("virtio"), > it's reported as model=virtio, device=rng. it corresponds with the way virt I see. Line 27: WATCHDOG("watchdog"), Line 28: VIRTIOSCSI("virtio-scsi"), Line 29: OTHER("other", "0"), Line 30: UNKNOWN("unknown", "-1"); -- 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: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
