Arik Hadas has posted comments on this change.

Change subject: core: fixed video device setting on import vm/template
......................................................................


Patch Set 3: (2 inline comments)

....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
Line 365:         // Note: the fetching of 'default display type' should happen 
before reading
Line 366:         // the hardware section
Line 367:         node = 
content.SelectSingleNode(getDefaultDisplayTypeStringRepresentation());
Line 368:         if (node != null) {
Line 369:             if (!StringHelper.isNullOrEmpty(node.InnerText)) {
I tried to make minimum changes in this patch because it fix a bug that its 
target is 3.1.z.. 
I want to make further refactoring in this class, besides the cleanup patch I 
already pushed (see http://gerrit.ovirt.org/#/c/10526/) - eliminate code 
duplication of getting the node, check that it's not null, check that its inner 
text is not null or empty.. replacing the usage of StringHelper with 
StringUtils can be made in that patch
Line 370:                 defaultDisplayType = 
DisplayType.forValue(Integer.parseInt(node.InnerText));
Line 371:                 vmBase.setdefault_display_type(defaultDisplayType);
Line 372:             }
Line 373:         }


Line 500:             // we need special handling for Monitor to define it as 
vnc or spice
Line 501:             if (Integer.valueOf(OvfHardware.Monitor) == resourceType) 
{
Line 502:                 // if default display type is defined in the ovf, set 
the video device that is suitable for it
Line 503:                 if (defaultDisplayType != null) {
Line 504:                     
vmDevice.setDevice(defaultDisplayType.getVmDeviceType().getName());
but then the control flow of the method becomes more complicated.. I think that 
we can stick with the 'single exit point' principle here, as the readability is 
not too bad (not like in methods which have flags all over just to follow the 
single exit point principle)
Line 505:                 }
Line 506:                 else {
Line 507:                     // get number of monitors from VirtualQuantity in 
OVF
Line 508:                     if 
(node.SelectSingleNode(OvfProperties.VMD_VIRTUAL_QUANTITY, _xmlNS) != null


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1bfaa3414507a982b9f22aa21fb3d05fe87e84d
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to