Moti Asayag has posted comments on this change.
Change subject: core : VmDeviceUtils fixes
......................................................................
Patch Set 1: (13 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
Line 35: VmBase newVmBase = getBaseObject(entity, oldVmBase.getId());
the getBaseObject might return null, yet you refer to its return value as not
null.
Line 69: VmBase VmBase =
DbFacade.getInstance().getVmStaticDAO().get(dstId);
DbFacade.getInstance().getVmDeviceDAO() is called many times in this class.
please consider either extracting it to a method or have it as a class member.
Line 187: // CD was changed
Question: the else statement refers to a case in which both old and new entity
had a CDROM or hadn't.
Is it possible that no CDROM was assigned for the VM/VM Template ?
if such possibility might occur, you'll face NPE at cd.setSpecParam()
Line 412: * @param <T>
if already added javadoc, perhaps complete parameter description
Line 417: private static <T> VmBase getBaseObject(T entity, Guid newId) {
consider define strongly types T as <T extends VmBase> here and in the methods
which uses this one.
This change requires change in ImportVmTemplateCommand.processImages to pass
getVm().getStaticData() instead of getVM() as an argument for this method
Line 420: newVmBase = (VmBase)
DbFacade.getInstance().getVmDAO().get(newId).getStaticData();
downcast isn't required
Line 422: newVmBase = (VmBase)
(DbFacade.getInstance().getVmTemplateDAO().get(newId));
same here
Line 437: disks = ((VM)entity).getDiskList();
This looks like the disksList should be moved to VmBase as well as it common
for both VM and VM Template. however not sure if it should be done in the scope
of this patch.
By doing so, you could use also <T extends VmBase>, skip the casting and the
'instance of'.
In addition, you're doing here some wrong casting: on the one hand you verify
the entity is VmStatic and a minute later you cast it to VM. This is a good
candidate for ClassCastException, So even if not planning moving the diskList
to VmBase, perhaps it is better moving it to VM static or modifying this
condition.
Line 445: String specParams = "";
those two lines could be merged as:
String specParams = appendDeviceIdToSpecParams(deviceId, "");
Line 460: id = ((VM)entity).getId();
same casting issue here.
Line 461: ifaces = ((VM)entity).getInterfaces();
and seems that interfaces could also be shifted to VmBase.
Line 469: String specParams = "";
those two lines could be merged as:
String specParams = appendDeviceIdToSpecParams(deviceId, "");
Line 481: private static String appendDeviceIdToSpecParams(Guid deviceId,
String specParams) {
i didn't see any proper use of specParams in this class and since this method
is private, perhaps it could be removed (unless future intention for it) ?
--
To view, visit http://gerrit.ovirt.org/2330
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5a5356b291d851e83eccb98fceb6cccd759e2e3
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
