Ayal Baron has posted comments on this change.
Change subject: core: Fascilitate RemoveImageCommand testing
......................................................................
Patch Set 14: I would prefer that you didn't submit this
(18 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java
Line 159: DiskImage diskImage = getDiskImage();
see comment below
Line 160: Guid storagePoolId = diskImage.getstorage_pool_id() !=
null ? diskImage.getstorage_pool_id().getValue()
to me it seems that diskImage might be null (hence a possible npe here)
Line 171: DiskImage image = (DiskImage) runVdsCommand(
this is imageInfo or imageFromStorage (yuck) or realImage (what can you do, the
actual state is on storage, not in db).
having image and diskImage is quite confusing.
diskImage is currImage or imageFromDb (brrr), or just image if previous name
changed.
Line 174:
getImage().getImageId())).getReturnValue();
so how do you decide when to use getImage and when to use getDiskImage?
(btw, the purpose of geDisktImage is beyond me)
Line 177: diskImage.setimageStatus(image.getimageStatus());
I wanted to comment that there should be a method doing these 2 things, but
apparently you already wrote it, so why not use it? setImageStatus
Line 355: DiskImage diskImage = getParameters().isImportEntity() ?
getDestinationDiskImage() : getDiskImage();
this logic has no business here! (pun not intended).
isImportEntity is only relevant to import commands and should remain
encapsulated there!
Line 365: Guid storagePoolId =
getDestinationDiskImage().getstorage_pool_id() != null ?
getDestinationDiskImage()
maybe assign ddisk = getDestinationDiskImage() and then use it throughout here
and make the lines much shorter (yet lo and behold no info is lost)?
Line 366: .getstorage_pool_id().getValue() : Guid.Empty;
I can't take it anymore!
please, change getstorage_pool_id to return Guid.Empty if it's null and in one
shot make it so that it cannot be null anywhere in the code. All this null
messing is nullifying my brain.
please change setstorage_pool_id to set it to Guid.Empty if null is passed and
set initial value to Guid.Empty and then remove the check if null I asked for
above in the getter because it can never be null!!!!!!!!!!!!!
ahhhhhhhhhhhhhhhhhh!
repeat everywhere for everything and stop all this null nonsense.
it's 01:43, I shouldn't be reviewing code at such hours, it's starting to get
to me.
Line 374: DiskImage newImageIRS = (DiskImage) runVdsCommand(
I love the naming consistency whenever we get the imageInfo from storage:
image
newImageIRS
fromIRS
etc.
Line 405: if (hasDiskImage()) {
can it be that getDestinationDiskImage is not null but hasDiskImage is?
if not then you should change this to:
if (hasDiskImage()) {
if (getDestinationDiskImage() != null) {
RemoveBlahBlah
}
UnlockImage();
}
And if it is possible then something is wrong?
In addition, how is it that you're calling UnlockImage here? EndWithFailure in
commands which LockImage should UnlockImage
But this entire method doesn't belong here as BaseImagesCommand should have no
notion of source and target, only commands dealing with multiple domains should
have such a notion.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateImageTemplateCommand.java
Line 59: .RunVdsCommand(
why not just runVdsCommand ?
Line 69: getParameters()
why break here?
Line 72: newImage
and here?
Line 144: UnlockImage();
how is it that you can unlock first and then perform all these operations?
seems like a race between the command failing and concurrently being run again?
(same command)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateSnapshotCommand.java
Line 86: vdsReturnValue =
again, why not runVdsCommand while you're at it?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyImageGroupCommand.java
Line 175: UnlockImage();
this is asymmetric!
LockImage locks regardless of operation.
no check if hasDiskImage?
....................................................
Commit Message
Line 7: core: Fascilitate RemoveImageCommand testing
s/Fascilitate/Facilitate/
Line 10: order to fascilitate writing unit tests for RemoveImageCommand in later
same
--
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: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@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
