Ayal Baron has posted comments on this change.
Change subject: add/del network - add bridgesless network
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(11 inline comments)
I got tired of reviewing in the middle (not patch author's fault, the code was
convoluted to begin with).
....................................................
File vdsm/configNetwork.py
Line 260: open(conffile,
'w').write("""DEVICE=%s.%s\nONBOOT=yes\nVLAN=yes\nBOOTPROTO=none\n"""
% (pipes.quote(iface), vlanId))
I would split the content from the writing to make this section clearer and remove
dupication.
something like:
net =
"""DEVICE=%s.%s\nONBOOT=yes\nVLAN=yes\nBOOTPROTO=none\n%s"""
% (pipes.quote(iface), vlanId, 'BRIDGE=%s\n' % pipes.quote(network) if bridged
else "")
open(conffile, 'w').write(net)
(or split net line into 2 if you don't like using x if whatever else something else)
Line 375: if bridged:
You should turn bridged into a boolean before using it, otherwise 'false' would be
analysed as True.
something like:
bridged = misc.parseBool(bridged)
Line 443: def addNetwork(network, vlan=None, bonding=None, nics=None, ipaddr=None,
netmask=None, gateway=None,
This function is a disaster and the patch is just making it that much worse.
Should split internal functionalities into separate functions, so that flow is clear.
It is waaaaaaay too long.
Line 447: bridged = utils.tobool(options.get('bridged', 'True'))
addNetwork is the old API, why are we adding support for bridgeless networks to it and not
only to setupNetworks?
And in any event, I hate the usage of 'options', it is wrong. why is
'bridged' an option but 'force' is not nor any other parameter (incl.
'optional' ones)?
Line 450: if not _isTrue(force) and bridged:
is there no validation for bridgeless?
Shouldn't block this patch, but instead of not _isTrue which has weird semantics, why
not use misc.parseBool for boolean params in API and then just say "if not
force"?
e.g.
force = misc.parseBool(force)
if not force:
(same applies to bridge)
Line 468: configWriter.addVlan(vlan, bonding or nics[0], network, bridged)
'bonding or nics[0]' repeats in this function, why not define
ifbond = bonding or nics[0] at the beginning and then use it?
Line 501: and bridged:
why is dhcp dependent on bridge?
and what's the point in doing this if a second later you rub ifup(network) if it is
bridged? (I'm assuming the logic here is first ifup would do the work in a separate
process and second would immediately return, but this is convoluted, need to make the code
more straightforward)
Line 510: ifaceNetwork = bonding or nics[0]
I see it's already defined, so need to move this up and use it all over
Line 513: if not utils.tobool(options.get('skipLibvirt', False)):
Again, not related to this patch but,
Again usage of options for a hidden option...
In the least there should be a comment in the docstring about the options...
I see we have 3 implementations of the same things:
misc.parseBool
util.tobool
configNetwork._isTrue
Argh.
Line 516: def createLibvirtNetwork(network, bridged, iface):
Looks (to me) like the logical order of the params should be changed to (network, iface,
bridged) - i.e. devices first then logic modifier
Also, I'm not certain about this, but since you have if bridged many times in the
calling function, why not just accept the 'forward mode' as a param and avoid the
different paths in the function?
Line 589: if configWriter is None:
Why not move if configWriter up and then unite the if bridged clauses into a single
clause?
i.e.
if configWriter...
if bridged:
assert...
if network:
....
--
To view, visit
http://gerrit.ovirt.org/848
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>