Antoni Segura Puimedon has posted comments on this change.
Change subject: iproute2 binary wrapper
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(4 inline comments)
Beginning of the review. Will continue later.
....................................................
File vdsm/netconf/iproute2.py
Line 23:
Line 24: ipBinary = str(utils.CommandPath("ip", "/sbin/ip"))
Line 25:
Line 26:
Line 27: class Route(object):
vdsm/netconf/iproute2.py should be the network configurator (just like
ifcfg.py). In fact, anything inside vdsm/netconf was meant to be a
configurator. I would probably put this module in vdsm/
Line 28: def __init__(self, device=None, network=None, ipaddr=None,
table=None,
Line 29: gateway=None, text=None):
Line 30: '''
Line 31: * Two allowed possibilities:
Line 26:
Line 27: class Route(object):
Line 28: def __init__(self, device=None, network=None, ipaddr=None,
table=None,
Line 29: gateway=None, text=None):
Line 30: '''
PEP257 indicates that for consistency docstrings should always be lead and
closed by three double quotes, i.e., """
I know we are not very consistent with this in the codebase and I'm a guilty
party, but we should not introduce any more '''.
Line 31: * Two allowed possibilities:
Line 32: 1) default via <gateway> ...
Line 33: 2) <network> via <ip> ...
Line 34:
Line 35: * device is required
Line 36: * table is optional
Line 37: '''
Line 38: if network and gateway:
Line 39: raise ValueError("A text cannot be a default text and a "
I don't find the name "text" nor the whole ValueError message to constitute a
very clear communication of the problem at hand.
I'm not perfectly convinced that this is the correct modeling though. In my
mind, gateway and ipaddr are no different from one another.
To sum up. I would drop gateway both as parameter and as attribute. If network
is None or 0.0.0.0/0 ipaddr is a gateway, otherwise it is just a route to the
indicated network. I don't see the need to have a special variable and logic
for it.
Line 40: "non-default text simultaneously")
Line 41: if not text and device is None:
Line 42: raise ValueError("device is a required parameter")
Line 43:
Line 57: self.table = table
Line 58: self.gateway = gateway
Line 59: else:
Line 60: (self.device, self.network, self.ipaddr, self.table,
Line 61: self.gateway) = self._parse(text)
you are missing a space of indentation to lead this line.
Line 62:
Line 63: def _parse(self, route):
Line 64: route = route.split()
Line 65:
--
To view, visit http://gerrit.ovirt.org/15335
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d315c3294fd7f058cdc840dea329d91a658a304
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Assaf Muller <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Assaf Muller <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Livnat Peer <[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
