Ayal Baron has posted comments on this change.
Change subject: core: Cleanup in RemoveImageCommand and anscentors
......................................................................
Patch Set 10: (17 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java
Line 45: if (parameters instanceof ImagesContainterParametersBase) {
not relevant for this patch but, ContainTer?
Line 48: initStoragePoolId();
The name doesn't reflect the fact that this only happens for image disks.
Line 54:
setStoragePoolId(getDiskImage().getstorage_pool_id().getValue());
how could we be in BaseImagesCommand and hasDiskImage returns False?
If storage_pool_id is null, what's the issue with setting null in
setStoragePoolId? (I'm trying to see if 'if' clause is at all necessary.
If you're already testing, could getDiskImage().getstorage_pool_id() return
Guid.Empty ? if so, why would it be ok to set that but not null?
Line 179: DiskImage image = (DiskImage) runVdsCommand(
this cannot return null?
Line 364: protected void SetImageStatus(DiskImage diskImage, ImageStatus
imageStatus) {
simply removing the word 'static' did not make this a non static method.
As long as you're passing diskImage there is no sense in this method to be a
bound method
Line 399: if (hasDiskImage()) {
how can we reach this point without an image and why is that ok?
Line 401: UnLockImage();
why does BaseImageCommand UnLockImage but never LockImage?
Line 419: if (hasDiskImage()) {
UnlockImage should probably be in EndWithFailure directly and not here (like it
is in EndSuccessfully)
Line 435: // Default action is no action
wasn't it self evident?
Line 448: public void GetImageChildren(Guid snapshot, List<Guid> children) {
again, you did not make this method any less static.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java
Line 44: setDiskImage(((getParameters().getDiskImage()) != null) ?
getParameters().getDiskImage() : getDiskImage());
and again, why is RemoveImageCommand even called with a null image?
and even if it is, how would getDiskImage() have an image at this point?
In addition, if mDiskImage is already set then why would it be ok to override
it?
This whole thing doesn't make any sense to me.
why is setDiskImage(getDiskImage().getstorage_pool_id()) not sufficient?
Line 48: if (GuidUtils.isEmpty(getStoragePoolId())) {
why did you change this from Guid.Empty.equals(getStoragePoolId()) ?
Line 55: if (GuidUtils.isEmpty(getParameters().getStorageDomainId()) &&
hasDiskImage()) {
same here wrt Guid
btw, here you placed hasDiskImage in the if and above in the set.
I prefer this way (in the if)
Line 97: if (diskImage != null) {
we already checked hasDiskImage in executeCommand, why again?
Line 148: UnLockImage();
Lock is done only if isShouldBeLocked is true, so Unlock should be done under
same condition?
Also, not related to this patch but I think Lock should be taken in
executeCommand and not hidden in performImageVdsmOperation
Finally, UnLockImage is called in BaseImagesCommand, so why again here?
bonus nit pick - Unlock doesn't require a capital L
....................................................
File
backend/manager/modules/compat/src/main/java/org/ovirt/engine/core/compat/NGuid.java
Line 96: public boolean equalsGuid(NGuid otherGuid) {
why?
....................................................
Commit Message
Line 7: core: Cleanup in RemoveImageCommand and anscentors
anscentors?
motivation for cleanup?
--
To view, visit http://gerrit.ovirt.org/6024
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie66b450571d680e556dc812d9926161d9f190221
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@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
