Allon Mureinik has posted comments on this change.
Change subject: core: Add the QEMU guest agent support
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(10 inline comments)
the patch seems incomplete - who uses this property? where is it set?
....................................................
File backend/manager/dbscripts/create_tables.sql
Line 475:
Line 476: --
----------------------------------------------------------------------
Line 477: -- Add table "vm_dynamic"
Line 478: --
----------------------------------------------------------------------
Line 479:
why did you add a whitespace?
Line 480: CREATE TABLE vm_dynamic
Line 481: (
Line 482: vm_guid UUID NOT NULL,
Line 483: status INTEGER NOT NULL,
Line 495: migrating_to_vds UUID,
Line 496: app_list VARCHAR(4000),
Line 497: display INTEGER,
Line 498: acpi_enable BOOLEAN,
Line 499: qga_enable BOOLEAN,
You should not touch the create_tables.sql script at all.
You should have an upgrade script that contains:
select fn_db_add_column('vm_synamic', 'qga_enable', 'boolean')
In addition:
1. consider making the column not null and adding a default (presumably false?)
2. note that since you're upgrading, the column will be at the end of the table.
3. shouldn't this /also/ be in vm_static?
Line 500: session INTEGER,
Line 501: display_ip VARCHAR(255),
Line 502: display_type INTEGER,
Line 503: kvm_enable BOOLEAN,
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java
Line 86: @Column(name = "acpi_enable")
Line 87: private Boolean acpi_enable;
Line 88:
Line 89: @Column(name = "qga_enable")
Line 90: private Boolean qga_enable;
I'd use a primitive boolean
Line 91:
Line 92: @Column(name = "session")
Line 93: private SessionState session = SessionState.Unknown;
Line 94:
Line 450: public VmDynamic() {
Line 451: mExitStatus = VmExitStatus.Normal;
Line 452: mWin2kHackEnable = false;
Line 453: acpi_enable = true;
Line 454: qga_enable = Config.<Boolean>
GetValue(ConfigValues.QEMUGuestAgentEnabled);
I'd remove this, and just set it as false.
Line 455: kvm_enable = true;
Line 456: session = SessionState.Unknown;
Line 457: boot_sequence = BootSequence.C;
Line 458: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java
Line 632: public void setacpi_enable(Boolean value) {
Line 633: this.mVmDynamic.setacpi_enable(value);
Line 634: }
Line 635:
Line 636: public Boolean getqga_enable() {
why allow null?
why not simply use a primitive boolean?
Line 637: return this.mVmDynamic.getqga_enable();
Line 638: }
Line 639:
Line 640: public void setqga_enable(Boolean value) {
Line 636: public Boolean getqga_enable() {
Line 637: return this.mVmDynamic.getqga_enable();
Line 638: }
Line 639:
Line 640: public void setqga_enable(Boolean value) {
same
Line 641: this.mVmDynamic.setqga_enable(value);
Line 642: }
Line 643:
Line 644: public String getdisplay_ip() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1407:
Line 1408: @Reloadable
Line 1409: @TypeConverterAttribute(Boolean.class)
Line 1410: @DefaultValueAttribute("true")
Line 1411: QEMUGuestAgentEnabled(373),
why is this config value needed?
isn't this property decided per VM?
Line 1412:
Line 1413: Invalid(65535);
Line 1414:
Line 1415: private int intValue;
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDAODbFacadeImpl.java
Line 293: entity.setnum_of_sockets(rs.getInt("num_of_sockets"));
Line 294: entity.setcpu_per_socket(rs.getInt("cpu_per_socket"));
Line 295:
entity.setusb_policy(UsbPolicy.forValue(rs.getInt("usb_policy")));
Line 296: entity.setacpi_enable((Boolean)
rs.getObject("acpi_enable"));
Line 297: entity.setqga_enable((Boolean)
rs.getObject("qga_enable"));
if you change it to a primitive, you can just use rs.getBoolean
Line 298:
entity.setsession(SessionState.forValue(rs.getInt("session")));
Line 299: entity.setdisplay_ip(rs.getString("display_ip"));
Line 300:
entity.setdisplay_type(DisplayType.forValue(rs.getInt("display_type")));
Line 301: entity.setkvm_enable((Boolean)
rs.getObject("kvm_enable"));
....................................................
File backend/manager/modules/dal/src/test/resources/fixtures.xml
Line 1370: <column>migrating_to_vds</column>
Line 1371: <column>app_list</column>
Line 1372: <column>display</column>
Line 1373: <column>acpi_enable</column>
Line 1374: <column>qga_enable</column>
you should also add the values in the corresponding rows.
Have you ran the DAO tests?
Line 1375: <column>session</column>
Line 1376: <column>display_ip</column>
Line 1377: <column>display_type</column>
Line 1378: <column>kvm_enable</column>
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java
Line 61: createInfo.add(VdsProperties.kvmEnable,
vm.getkvm_enable().toString()
Line 62: .toLowerCase());
Line 63: createInfo.add(VdsProperties.acpiEnable,
vm.getacpi_enable().toString()
Line 64: .toLowerCase());
Line 65: createInfo.add(VdsProperties.qgaEnable,
vm.getqga_enable().toString()
I think that will be better.
what will happen with a VDSM that /does/ support it but won't get the key word?
will it just assume it's false?
Line 66: .toLowerCase());
Line 67:
Line 68: createInfo.add(VdsProperties.Custom,
Line 69:
VmPropertiesUtils.getInstance().getVMProperties(vm.getvds_group_compatibility_version(),
--
To view, visit http://gerrit.ovirt.org/7179
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f6f2d372fd94ae235b1803bcde6ec0f188d488
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
