Eduardo has posted comments on this change. Change subject: Add hotPlug/hotUnplug NIC feature ......................................................................
Patch Set 1: Looks good to me, but someone else must approve (8 inline comments) Passed with objections. Dan, up to you. .................................................... File vdsm/API.py Line 364: IMHO this two functions are unnecessary. There are only validates (oh my...) Better if will be done in the level using the parameters. .................................................... File vdsm/BindingXMLRPC.py Line 245: def vmHotplugNic(self, params): All this functions but vm*plug* (4) + vmMigrate (1) receive vmId as 1st param and pass all the others. May be better to make input uniform and wrap all this in invariant code. .................................................... File vdsm/libvirtvm.py Line 1295: 'message': e.message}} IMHO: is better to unify all the return values. In addition: get an errCode['hotplugNic'] and change the message. Line 1303: self.saveState() I'm not sure here. Line 1314: if dev.macAddr.lower() == nicParams['macAddr'].lower(): Why do you suspect that are UPPER mac's in devices? Enforce it when adding. Line 1328: self._devices[vm.NIC_DEVICES].remove(nic) If you worry about thread safety, this remove can raise. Line 1331: for dev in self.conf['devices'][:]: Do list comprehension. Line 1353: 'message': e.message}} Same as before. -- To view, visit http://gerrit.ovirt.org/1388 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iecd56650308f5836913f513fffc179f39fc8f6cc Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Igor Lvovsky <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
