Arik Hadas has posted comments on this change. Change subject: backend: Add HostDev passthrough support #1 ......................................................................
Patch Set 12: (6 comments) http://gerrit.ovirt.org/#/c/35892/12//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-12-03 17:09:16 +0100 Line 4: Commit: Martin Betak <mbe...@redhat.com> Line 5: CommitDate: 2015-01-15 16:25:53 +0100 Line 6: Line 7: backend: Add HostDev passthrough support #1 > just that this patch is the first in the PCI series, I don't have any follo I see, so please drop it Line 8: Line 9: Preliminary steps for providing the ability to pass through Line 10: selected devices from Hosts to VMs. Line 11: http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/RefreshHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/RefreshHostDevicesCommand.java: Line 18: import java.util.HashMap; Line 19: import java.util.List; Line 20: import java.util.Map; Line 21: Line 22: @NonTransactiveCommandAttribute(forceCompensation = true) > If I understand correctly the transaction at line #65 needs compensation si I see that you didn't take any snapshot for any entity so there's no point in compensation (what will the roll-back do? which state does it need to restore?) I agree this command should not be transactive since it issues VDS command, but I see no point in compensation here Line 23: public class RefreshHostDevicesCommand<T extends VdsActionParameters> extends RefreshHostInfoCommandBase<T> { Line 24: Line 25: @Inject Line 26: private ResourceManager resourceManager; Line 46: Line 47: for (Map.Entry<String, HostDevice> entry : fetchedMap.entrySet()) { Line 48: HostDevice device = entry.getValue(); Line 49: if (oldMap.containsKey(entry.getKey())) { Line 50: if (!oldMap.get(entry.getKey()).equals(device)) { > `entry` iterates over new map and device = entry.getValue() by definition : I was probably not focused enough when writing that comment.. ack Line 51: changedDevices.add(device); Line 52: } Line 53: } else { Line 54: newDevices.add(device); Line 52: } Line 53: } else { Line 54: newDevices.add(device); Line 55: } Line 56: } > Not sure how do you want this to look like, but consider that this "device" alright, I can live with it that way, just a matter of code-style I guess Line 57: Line 58: final List<HostDevice> removedDevices = new ArrayList<>(); Line 59: for (Map.Entry<String, HostDevice> entry : oldMap.entrySet()) { Line 60: if (!fetchedMap.containsKey(entry.getKey())) { http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/HostDeviceParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/HostDeviceParameters.java: Line 1: package org.ovirt.engine.core.common.queries; Line 2: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class HostDeviceParameters extends VdcQueryParametersBase { > The tuple (host_id, device_name) represents primary key of the HostDevice ( ack Line 6: Line 7: private Guid hostId; Line 8: private String deviceName; Line 9: http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 1855: List<HostDevice> devices = new ArrayList<>(); Line 1856: Line 1857: for (Entry<String, Map<String, Object>> entry : deviceList.entrySet()) { Line 1858: Line 1859: Map<String, Object> params = (Map<String, Object>) entry.getValue().get(VdsProperties.PARAMS); > Done what about replacing also the Object with String? is it possible? I see that all the values are Strings, right? Line 1860: String deviceName = entry.getKey(); Line 1861: Line 1862: HostDevice device = new HostDevice(); Line 1863: device.setDeviceName(entry.getKey()); -- To view, visit http://gerrit.ovirt.org/35892 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5575c0db797d7d04339c4b309bb4325e853ffed Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
