Change in vdsm[master]: netwiring: [1/3] Add API definitions.
danken at redhat.com
danken at redhat.com
Thu Nov 29 19:09:04 UTC 2012
Dan Kenigsberg has posted comments on this change.
Change subject: netwiring: [1/3] Add API definitions.
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File vdsm_api/vdsmapi-schema.json
Line 4852: # @updateVmDeviceParams:
Line 4853: #
Line 4854: # Parameters for @VM.updateVmDevice
Line 4855: #
Line 4856: # @type: The type of device to update. For example 'interface' for network
I think this can be made more similar to VmInterfaceDevice - I think this is @device.
Line 4857: # devices or 'disk' for disk storage devices.
Line 4858: #
Line 4859: # @alias: Alias used to identify this device in commands.
Line 4860: #
Line 4866: #
Line 4867: # @promisc: If present, indicates the traffic of the network which should be
Line 4868: # mirrored to the interface identified by alias.
Line 4869: #
Line 4870: # Since 4.11.0
this is going to be available in 4.10.3 (I hope)
Line 4871: ##
Line 4872: {'type': 'updateVmDeviceParams',
Line 4873: 'data': {'type': 'str', 'network': 'str',
Line 4874: 'linkState': 'NetworkInterfaceState', 'alias': 'str', 'promisc': ''}}
Line 4870: # Since 4.11.0
Line 4871: ##
Line 4872: {'type': 'updateVmDeviceParams',
Line 4873: 'data': {'type': 'str', 'network': 'str',
Line 4874: 'linkState': 'NetworkInterfaceState', 'alias': 'str', 'promisc': ''}}
I'd follow libvirt's naming: it's the state of the link, not of the network interface.
I think linkState should be optional here (default is up) , as well as in VmInterfaceDevice.
promisc should not be an empty string. Actually, it is called "portMirroring", it's optional, and is a list of network that should be mirrored into this iface. This should be put in VmInterfaceDevice as well - preferably in a preceding patch.
Line 4875:
Line 4876: ##
Line 4877: # @VM.updateVmDevice:
Line 4878: #
....................................................
File vdsm/BindingXMLRPC.py
Line 263: return vm.hotunplugNic(params)
Line 264:
Line 265: def updateVmDevice(self, vmId, params):
Line 266: vm = API.VM(vmId)
Line 267: return vm.updateDevice(params)
I'm not the biggest fan of introducing partial commits, that ends up in AttributeError.
Maybe it simplifies the review process - I should probably first see how complex is the implementation.
Line 268:
Line 269: def vmSnapshot(self, vmId, snapDrives):
Line 270: vm = API.VM(vmId)
Line 271: return vm.snapshot(snapDrives)
Line 793: (self.vmHotplugDisk, 'hotplugDisk'),
Line 794: (self.vmHotunplugDisk, 'hotunplugDisk'),
Line 795: (self.vmHotplugNic, 'hotplugNic'),
Line 796: (self.vmHotunplugNic, 'hotunplugNic'),
Line 797: (self.updateVmDevice, 'updateVmDevice'))
Please keep the name convention for vm-related commands: vmUpdateDevice seems better.
Line 798:
Line 799: def getIrsMethods(self):
Line 800: return ((self.domainActivate, 'activateStorageDomain'),
Line 801: (self.domainAttach, 'attachStorageDomain'),
--
To view, visit http://gerrit.ovirt.org/9560
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d8352f237dbe0229dff368f7c1dfa4f5f8fc766
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list