Change in vdsm[master]: Add hotPlug/hotUnplug NIC feature

ewarszaw at redhat.com ewarszaw at redhat.com
Wed Feb 1 15:24:24 UTC 2012


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 <ilvovsky at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list