| Message ID | 1423042236-25252-18-git-send-email-ilan.peer@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Feb 04, 2015 at 04:30:29AM -0500, Ilan Peer wrote: > 1. Add get_group_ifname() to wpasupplicant.py > 2. Use the function to get the interface name for the bridge. That approach in general looks fine.. > diff --git a/tests/hwsim/test_p2p_autogo.py b/tests/hwsim/test_p2p_autogo.py > @@ -451,17 +451,17 @@ def test_autogo_bridge(dev): > - dev[0].remove_group() > finally: > dev[0].request("AUTOSCAN ") > - subprocess.Popen(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].ifname], > + subprocess.Popen(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].get_group_ifname()], > stderr=open('/dev/null', 'w')) > subprocess.Popen(['sudo', 'ip', 'link', 'set', 'dev', 'p2p-br0', 'down'], > stderr=open('/dev/null', 'w')) > subprocess.Popen(['sudo', 'brctl', 'delbr', 'p2p-br0'], > stderr=open('/dev/null', 'w')) > + dev[0].remove_group() ... but I'm not that sure about move dev[0].remove_group() in this way. That function can fail and raise an exception. I'd rather leave it where it was and store the dev[0].get_group_ifname() result locally so that it can be used in finally-processing.
> -----Original Message----- > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > bounces@lists.shmoo.com] On Behalf Of Jouni Malinen > Sent: Thursday, February 05, 2015 13:52 > To: hostap@lists.shmoo.com > Subject: Re: [PATCH 17/24] tests: Modify test_autogo_bridge() test to use > group interface > > On Wed, Feb 04, 2015 at 04:30:29AM -0500, Ilan Peer wrote: > > 1. Add get_group_ifname() to wpasupplicant.py 2. Use the function to > > get the interface name for the bridge. > > That approach in general looks fine.. > > > diff --git a/tests/hwsim/test_p2p_autogo.py > > b/tests/hwsim/test_p2p_autogo.py @@ -451,17 +451,17 @@ def > test_autogo_bridge(dev): > > - dev[0].remove_group() > > finally: > > dev[0].request("AUTOSCAN ") > > - subprocess.Popen(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].ifname], > > + subprocess.Popen(['sudo', 'brctl', 'delif', 'p2p-br0', > > + dev[0].get_group_ifname()], > > stderr=open('/dev/null', 'w')) > > subprocess.Popen(['sudo', 'ip', 'link', 'set', 'dev', 'p2p-br0', 'down'], > > stderr=open('/dev/null', 'w')) > > subprocess.Popen(['sudo', 'brctl', 'delbr', 'p2p-br0'], > > stderr=open('/dev/null', 'w')) > > + dev[0].remove_group() > > ... but I'm not that sure about move dev[0].remove_group() in this way. > That function can fail and raise an exception. I'd rather leave it where it was > and store the dev[0].get_group_ifname() result locally so that it can be used > in finally-processing. > This was the reason for this change. remove_group() has the logic to deduce what is the group interface name. I might be missing something here .. Ilan.
On Fri, Feb 06, 2015 at 07:21:57AM +0000, Peer, Ilan wrote: > > ... but I'm not that sure about move dev[0].remove_group() in this way. > > That function can fail and raise an exception. I'd rather leave it where it was > > and store the dev[0].get_group_ifname() result locally so that it can be used > > in finally-processing. > > This was the reason for this change. remove_group() has the logic to deduce what is the group interface name. I might be missing something here .. The reason for that change was to use the group interface in the bridge to make the test case work. The way this was done is not ideal for error cases. remove_group() can raise an exception if it is called before the group is formed and that can indeed be the case on the finally-path. I'll fix the patch to do this in the way described above.
diff --git a/tests/hwsim/test_p2p_autogo.py b/tests/hwsim/test_p2p_autogo.py index 460fa55..77e789f 100644 --- a/tests/hwsim/test_p2p_autogo.py +++ b/tests/hwsim/test_p2p_autogo.py @@ -440,10 +440,10 @@ def test_autogo_bridge(dev): autogo(dev[0]) subprocess.call(['sudo', 'brctl', 'addbr', 'p2p-br0']) subprocess.call(['sudo', 'brctl', 'setfd', 'p2p-br0', '0']) - subprocess.call(['sudo', 'brctl', 'addif', 'p2p-br0', dev[0].ifname]) + subprocess.call(['sudo', 'brctl', 'addif', 'p2p-br0', dev[0].get_group_ifname()]) subprocess.call(['sudo', 'ip', 'link', 'set', 'dev', 'p2p-br0', 'up']) time.sleep(0.1) - subprocess.call(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].ifname]) + subprocess.call(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].get_group_ifname()]) time.sleep(0.1) subprocess.call(['sudo', 'ip', 'link', 'set', 'dev', 'p2p-br0', 'down']) time.sleep(0.1) @@ -451,17 +451,17 @@ def test_autogo_bridge(dev): ev = dev[0].wait_global_event(["P2P-GROUP-REMOVED"], timeout=1) if ev is not None: raise Exception("P2P group removed unexpectedly") - if dev[0].get_status_field('wpa_state') != "COMPLETED": + if dev[0].get_group_status_field('wpa_state') != "COMPLETED": raise Exception("Unexpected wpa_state") - dev[0].remove_group() finally: dev[0].request("AUTOSCAN ") - subprocess.Popen(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].ifname], + subprocess.Popen(['sudo', 'brctl', 'delif', 'p2p-br0', dev[0].get_group_ifname()], stderr=open('/dev/null', 'w')) subprocess.Popen(['sudo', 'ip', 'link', 'set', 'dev', 'p2p-br0', 'down'], stderr=open('/dev/null', 'w')) subprocess.Popen(['sudo', 'brctl', 'delbr', 'p2p-br0'], stderr=open('/dev/null', 'w')) + dev[0].remove_group() def test_presence_req_on_group_interface(dev): """P2P_PRESENCE_REQ on group interface""" diff --git a/tests/hwsim/wpasupplicant.py b/tests/hwsim/wpasupplicant.py index 0ee653b..ad6f4e7 100644 --- a/tests/hwsim/wpasupplicant.py +++ b/tests/hwsim/wpasupplicant.py @@ -1041,3 +1041,6 @@ class WpaSupplicant: if ev is None: raise Exception(error) return ev + + def get_group_ifname(self): + return self.group_ifname if self.group_ifname else self.ifname