Change in vdsm[master]: add/del network - add bridgesless network

abaron at redhat.com abaron at redhat.com
Sun Jan 29 14:46:34 UTC 2012


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 at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>


More information about the vdsm-patches mailing list