Allon Mureinik has posted comments on this change.

Change subject: engine: Ties more consitently between vm disk device and disk 
entities.
......................................................................


Patch Set 2: Code-Review+1

(2 comments)

The change looks right, but the old behavior has been there since forever.
I'm curious how this bug never showed up.
Did something change, or did we always have it and just not notice it ever?

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 86:      * A list of the new disk images which were saved for the VM.
Line 87:      */
Line 88:     protected List<DiskImage> newDiskImages = new 
ArrayList<DiskImage>();
Line 89: 
Line 90:     protected Map<Guid, Guid> srcDiskToTargetDiskMapping = new 
HashMap<>();
I'm not a fan of protected members.
Consider having this private, and have methods to manipulate it.
Line 91: 
Line 92:     public AddVmCommand(T parameters) {
Line 93:         super(parameters);
Line 94:         // if we came from EndAction the VmId is not null


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
Line 166:      *
Line 167:      * @param oldVm
Line 168:      * @param newVmBase
Line 169:      * @param compatibilityVersion compatibility version
Line 170:      * @param isSoundDeviceEnabled device enabled - if null, keep old 
state
please refrain from making these fixes in unrelated patches.
Line 171:      */
Line 172:     public static void updateAudioDevice(VmBase oldVm, VmBase 
newVmBase, Version compatibilityVersion, Boolean isSoundDeviceEnabled) {
Line 173:         boolean removeDevice = false;
Line 174:         boolean createDevice = false;


-- 
To view, visit http://gerrit.ovirt.org/17854
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8bf284ea4e05f03fc0a7e94a1ec901aacbdc1ab1
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Haim Ateya <hat...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
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

Reply via email to