Ayal Baron has posted comments on this change.
Change subject: core: export/import of diskless VM/VM with no snappable
disks(#838937)
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java
Line 183: getDisksBasedOnImage();
in this case I still think this method should be wrapped with either a
getSnappableDisks method (which does not return a list of DiskImages) or just
with a hasSnappableDisks method which internally right now would perform the
above call + the isEmpty check.
It should be clear that what we care about here is snappable disks and not
diskimages
Line 409: VM vm = getVm();
I gather vm cannot possibly be null here?
Line 444: this.endDefaultOps(vm);
sorry for saying this only now but endDefaultOps is a pretty bad name (yes, I
know it was there before).
Should be something like 'updateSnapshotOvf' or 'persistSnapshotConfig' or
something (can be a separate patch as far as I'm concerned)
Line 455: EndActionOnAllImageGroups();
Can't we fail when we have no snappable images?
If so shouldn't we avoid calling EndActionOnAllImageGroups?
If we can fail and shouldn't avoid then why aren't we calling it in
EndSuccessfullySync...
Line 456: VM vm = getVm();
Again, I understand that vm cannot be null, correct? how do we know this?
canDoAction fails or something?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 475: if (vm.getImages().isEmpty()) {
again, should be wrapped.
Line 894: private void endVmRelatedOps() {
this name is not very indicative of what the method actually does
(persistVmConf or something)
Line 904: log.warn("ImportVmCommand::EndImportCommand: Vm is null -
not performing full EndAction");
Although in previous review I asked, it wasn't so that you'd return the old
behaviour, just to consider the implications of changing it.
it looks wrong to setSucceeded to true here
....................................................
Commit Message
Line 7: core: export/import of diskless VM/VM with no snappable disks(#838937)
I disagree, but it could be rephrased as:
*enable* export/import of Vm with no snappable disks (or no disks at all)
Note that without 'enable' you're not stating what you actually did at all.
--
To view, visit http://gerrit.ovirt.org/6379
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I77565ffd66134b44d15f66cfdfa97422e3c11fb2
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
