Ayal Baron has posted comments on this change.
Change subject: core: added snapshot id to attached disk (#834004)
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java
Line 124: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
why is this if statement inside the function but checking whether snapshots are
allowed is not? basically if it's not of type IMAGE then snapshots are not
allowed so I'm not sure why we'd even get here (looks like the 'if' is
redundant unless AllowSnapshot doesn't report correctly).
In addition, personally I dislike a function with all code inside an 'if'
statement. Also the name does not reflect what it does this way (it only
updates if ...)
--
To view, visit http://gerrit.ovirt.org/5718
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0945cecc63dc041fdb56e2fd7b733b287a9904c
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
