ACK with comments below...
On Mon, 2011-09-26 at 13:09 +0200, Ales Kozumplik wrote:
> Unit test included.
>
> Resolves: rhbz#740222
> ---
> booty/bootloaderInfo.py | 32 +++++++++++++++++++++++++++++++
> tests/booty_test/bootloaderInfo_test.py | 28 +++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+), 0 deletions(-)
> create mode 100644 tests/booty_test/bootloaderInfo_test.py
>
> diff --git a/booty/bootloaderInfo.py b/booty/bootloaderInfo.py
> index e3b5d34..4e1e7d3 100644
> --- a/booty/bootloaderInfo.py
> +++ b/booty/bootloaderInfo.py
> @@ -86,6 +86,36 @@ def rootIsDevice(dev):
>
> class KernelArguments:
>
> + def _merge_ip(self, args):
> + """
> + Find ip= arguments targetting the same interface and merge them.
> + """
> + # partition the input
> + def partition_p(arg):
> + # we are only interested in ip= parameters that use some kind of
> + # automatic network setup:
> + return arg.startswith("ip=") and arg.count(":") == 1
> + ip_params = filter(partition_p, args)
> + rest = set(filter(lambda p: not partition_p(p), args))
> + # strip away 'ip=' and split at the colon:
> + ip_params = map(lambda p: p[3:], ip_params)
> + ip_params = map(lambda p: p.split(":"), ip_params)
> + # create mapping from nics to their configurations
> + config = {}
> + for (nic, cfg) in ip_params:
> + if nic in config:
2 spaces before "config"
> + config[nic].append(cfg)
> + else:
> + config[nic] = [cfg]
> +
> + # output the new parameters:
> + ip_params = set()
> + for nic in config:
> + ip_params.add("ip=%s:%s" % (nic, ",".join(sorted(config[nic]))))
> + rest.update(ip_params)
> +
> + return rest
> +
I don't see any errors in here, but it took me a while to wrap my brain
around it. I think in this particular case you would be better of with
list comprehensions instead of map.
This would be my take on this functionality:
def _merge_ip(self, args):
# partition the input
pred = lambda arg: arg.startswith("ip=") and arg.count(":") == 1
ip_params = set([arg.split(":") for arg in args if pred(arg)])
rest = set([arg for arg in args if not pred(arg)])
# create mapping from nics to their configurations
config = collections.defaultdict(list)
for nic, cfg in ip_params:
config[nic].append(cfg)
# output the new parameters
ip_params = set(["%s:%s" % (nic, ",".join(sorted(cfg))) for nic, cfg
in config.items()])
rest.update(ip_params)
return rest
I use set() on ip_params in the beginning to make sure we don't have
more of the same arguments, if this can happen here...
Also, as I see it, you don't need to strip the "ip=" because all the
parameters in that list have it, and later you are prepending it again
to all members.
This way in the config dictionary, we will not have "eth0" but "ip=eth0"
but it does not matter, because at the end, we want to have "ip=eth0".
But this is just my point of view and others can see it even more hard
to read :)
So you can take this as an ACK to your solution, and this is just my
suggestion to make the function a little bit simpler.
> def getDracutStorageArgs(self, devices):
> args = set()
> types = {}
> @@ -135,6 +165,8 @@ class KernelArguments:
> all_args.update(self.args)
> all_args.update(self.appendArgs)
>
> + all_args = self._merge_ip(all_args)
> +
> return " ".join(all_args)
>
> def set(self, args):
> diff --git a/tests/booty_test/bootloaderInfo_test.py b/tests/booty_test/bootloaderInfo_test.py
> new file mode 100644
> index 0000000..fe057a8
> --- /dev/null
> +++ b/tests/booty_test/bootloaderInfo_test.py
> @@ -0,0 +1,28 @@
> +import mock
> +
> +class KernelArgumentsTestCase(mock.TestCase):
> + def setUp(self):
> + self.setupModules(
> + ['_isys', 'logging', 'anaconda_log', 'block'])
> +
> + def tearDown(self):
> + self.tearDownModules()
> +
> + def test_merge_ip(self):
> + import booty
> + ka = booty.KernelArguments(mock.Mock())
> +
> + # test that _merge_ip() doesnt break the simple case:
> + args = set(["one", "two", "ip=eth0:dhcp"])
> + self.assertEqual(ka._merge_ip(args),
> + set(["one", "two", "ip=eth0:dhcp"]))
> +
> + # test that it does what it's supposed to:
> + args = set(["one", "two", "ip=eth0:dhcp", "ip=eth0:auto6",
> + "ip=wlan0:dhcp",
> + "ip=10.34.102.102::10.34.39.255:24:aklab:eth2:none"])
> + self.assertEqual(ka._merge_ip(args), set([
> + "one", "two",
> + "ip=wlan0:dhcp",
> + "ip=10.34.102.102::10.34.39.255:24:aklab:eth2:none",
> + "ip=eth0:auto6,dhcp"]))
--
Martin Gracik <mgracik@xxxxxxxxxx>
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
