Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
Sorry for the long time it took me to do a second, in depth review of this patch. I like a lot the fact that this reduces the size of setupNetworks and splits the bonding option builgin responsability away from it.
With a couple of tweaks I think we could take it in soon. Thanks for the refactoring ;-)
.................................................... File vdsm/configNetwork.py Line 1392: # If the new added network was created on top of Line 1393: # existing bond, we need to keep the bond on rollback Line 1394: # flow, else we will break the new created bond. Line 1395: isNew = networkAttrs['bonding'] in bondings Line 1396: delNetwork(network, force=True, implicitBonding=isNew) isNew is used only once. I think we could just replace the previous two lines with:
delNetwork( network, force=True, implicitBonding=networkAttrs['bonding'] in bondings ) or delNetwork(network, force=True, implicitBonding=\ networkAttrs['bonding'] in bondings)
better yet: delNetwork(network, force=True, implicitBonding= networkAttrs['bonding'] in bondings)
Apart from that, this 'for' does not runs all the time with the networkAttrs or the last added network in the previour 'for'. Shouldn't this get the networkAttrs corresponding to the current network from networksAdded? Line 1397: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1398: 'connectivity check failed') Line 1399: except: Line 1400: configWriter.restoreBackups()
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server