Liron Aravot has posted comments on this change.

Change subject: core: Add Video managed device for VM's/Template's OVF.
......................................................................


Patch Set 3:

(5 comments)

Comments inside.
I think that the problem is broader, the OVF files on the domains never 
contained devices info. (not only video devices)- which will cause the import 
to always lack data.
I think that this is what should be fixed, not only the video devices.

http://gerrit.ovirt.org/#/c/34734/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java:

Line 319:         List<VmTemplate> templates = 
getVmTemplateDao().getVmTemplatesByIds(idsToProcess);
Line 320: 
Line 321:         for (VmTemplate template : templates) {
Line 322:             if (VmTemplateStatus.Locked != template.getStatus()) {
Line 323:                 setManagedVideoDevices(template);
this should move to loadTemplateData() method
Line 324:                 updateTemplateDisksFromDb(template);
Line 325:                 boolean verifyDisksNotLocked = 
verifyImagesStatus(template.getDiskList());
Line 326:                 if (verifyDisksNotLocked) {
Line 327:                     loadTemplateData(template);


Line 499:             }
Line 500:         }
Line 501:     }
Line 502: 
Line 503:     private void setManagedVideoDevices(VmBase vm) {
please rename vm to vmBase
Line 504:         Map<Guid, VmDevice> managedDeviceMap = 
vm.getManagedDeviceMap();
Line 505:         if (managedDeviceMap == null) {
Line 506:             managedDeviceMap = new HashMap<Guid, VmDevice>();
Line 507:         }


Line 501:     }
Line 502: 
Line 503:     private void setManagedVideoDevices(VmBase vm) {
Line 504:         Map<Guid, VmDevice> managedDeviceMap = 
vm.getManagedDeviceMap();
Line 505:         if (managedDeviceMap == null) {
please check here vm.getManagedDeviceMap()
Line 506:             managedDeviceMap = new HashMap<Guid, VmDevice>();
Line 507:         }
Line 508:         List<VmDevice> devices = 
DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(vm.getId());
Line 509:         for (VmDevice device : devices) {


Line 504:         Map<Guid, VmDevice> managedDeviceMap = 
vm.getManagedDeviceMap();
Line 505:         if (managedDeviceMap == null) {
Line 506:             managedDeviceMap = new HashMap<Guid, VmDevice>();
Line 507:         }
Line 508:         List<VmDevice> devices = 
DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(vm.getId());
use getVmDeviceByVmIdAndType()
Line 509:         for (VmDevice device : devices) {
Line 510:             if (device.getType() == VmDeviceGeneralType.VIDEO) {
Line 511:                 managedDeviceMap.put(device.getDeviceId(), device);
Line 512:             }


Line 510:             if (device.getType() == VmDeviceGeneralType.VIDEO) {
Line 511:                 managedDeviceMap.put(device.getDeviceId(), device);
Line 512:             }
Line 513:         }
Line 514:         vm.setManagedDeviceMap(managedDeviceMap);
unneeded..initiate directly in line 506
Line 515:     }
Line 516: 
Line 517:     protected List<DiskImage> getAllImageSnapshots(DiskImage 
diskImage) {
Line 518:         return 
ImagesHandler.getAllImageSnapshots(diskImage.getImageId());


-- 
To view, visit http://gerrit.ovirt.org/34734
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5583edadd75bce7dfb3f5ba04cfcbe38f1dc7091
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipc...@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: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to