Dan Kenigsberg has posted comments on this change.
Change subject: Fix checking bonding options for missing bonds.
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File vdsm/configNetwork.py
Line 733: try:
Line 734: for option in bondingOptions.split():
Line 735: key, value = option.split('=')
Line 736: if not os.path.exists(
Line 737: '/sys/class/net/%s/bonding/%s' % (bond, key)):
this pyflakes is starting to infuriate me...
Line 738: raise ConfigNetworkError(ne.ERR_BAD_BONDING, '%r
is not a '
Line 739: 'valid bonding option' %
key)
Line 740: except ValueError:
Line 741: raise ConfigNetworkError(ne.ERR_BAD_BONDING, 'Error
parsing '
Line 739: 'valid bonding option' %
key)
Line 740: except ValueError:
Line 741: raise ConfigNetworkError(ne.ERR_BAD_BONDING, 'Error
parsing '
Line 742: 'bonding options: %r' %
bondingOptions)
Line 743:
I'd swear that I had a comment here, about my primordial fear of duplicating
data and about reading from sysfs not being THAT expensive.
Line 744:
Line 745: @contextmanager
Line 746: def validationBond(bonding):
Line 747: bond_created = False
Line 751: open(BONDING_MASTERS, 'w').write('+%s\n' % bonding)
Line 752: bond_created = True
Line 753: try:
Line 754: yield bonding
Line 755: except Exception:
to me this defies the beauty of context management - and it does not save much.
after all `ifup bondX` would attempt to re-add the device anyway.
Line 756: if bond_created:
Line 757: try:
Line 758: open(BONDING_MASTERS, 'w').write('-%s\n' % bonding)
Line 759: except IOError:
Line 756: if bond_created:
Line 757: try:
Line 758: open(BONDING_MASTERS, 'w').write('-%s\n' % bonding)
Line 759: except IOError:
Line 760: pass
There should not be two concurrent setupNetwork commands running. We'd break
down in gazilion other places if we allow that.
I'd rather have this context manager as simple as possible, and in the case of
an unforeseen interaction - die a brave death.
Line 761: raise
Line 762:
Line 763:
Line 764: def validateVlanId(vlan):
--
To view, visit http://gerrit.ovirt.org/12701
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I023a5bb8a52719559bb9d4716f25e0cba8b3530b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
