Repository: ansible/ansible
Pull request: aix network facts: Separate out the uname call to reduce total calls
##### ISSUE TYPE

 - Bugfix Pull Request


##### COMPONENT NAME
facts.py module util

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
2.3
```

##### SUMMARY

Fixes #11289



Problem statement:
AIX WPAR handling is inefficient and possibly buggy
The `lib/ansible/module_utils/facts.py:AIXNetwork.get_interfaces_info` method is structured like:

```
rc, out, err = module.run_command([ifconfig_path, '-a'])
for line in out.split('\n'):
    ...
    uname_path = module.get_bin_path('uname')
    if uname_path:
        rc, out, err = module.run_command([uname_path, '-W'])
```

This has two big problems:
1. It uses `subprocess.Popen` to run `uname -W` _for each line in ifconfig_. This is a performance issue because shelling out is expensive and this data isn't going to change during run on a single host. It should be collected once and then reused.
2. Note that `out` is overwritten inside the loop. It won't break the loop, which is iterating over `out.split('\n')` and not `out` directly. However, that may cause breakage in further edits that expect `out` to keep the same information throughout the loop. Consider using unique variable names like `ifconfig_out`,`entstat_out`, etc. to avoid weird, hard-to-debug errors later.

/cc @bmanojlovic who contributed this code.

Hints from issue discussion:
The code in devel (2.3) still has this inefficient behavior. I'm guessing it was just a mistaken indentation? https://github.com/ansible/ansible/pull/11290 was never merged and I suspect will never get rebased.

I don't have access to a machine to test this against anymore.

Commit message: aix network facts: Separate out the uname call to reduce total calls

Patch to review:
diff --git a/lib/ansible/module_utils/facts.py b/lib/ansible/module_utils/facts.py
index 9c98f796f96ff5..3a98ca23d6f22c 100644
--- a/lib/ansible/module_utils/facts.py
+++ b/lib/ansible/module_utils/facts.py
@@ -2837,6 +2837,14 @@ def get_interfaces_info(self, ifconfig_path, ifconfig_options='-a'):
             all_ipv4_addresses = [],
             all_ipv6_addresses = [],
         )
+
+        uname_rc = None
+        uname_out = None
+        uname_err = None
+        uname_path = self.module.get_bin_path('uname')
+        if uname_path:
+            uname_rc, uname_out, uname_err = self.module.run_command([uname_path, '-W'])
+
         rc, out, err = self.module.run_command([ifconfig_path, ifconfig_options])
 
         for line in out.splitlines():
@@ -2866,12 +2874,12 @@ def get_interfaces_info(self, ifconfig_path, ifconfig_options='-a'):
                     self.parse_inet6_line(words, current_if, ips)
                 else:
                     self.parse_unknown_line(words, current_if, ips)
-            uname_path = self.module.get_bin_path('uname')
+
             if uname_path:
-                rc, out, err = self.module.run_command([uname_path, '-W'])
                 # don't bother with wpars it does not work
                 # zero means not in wpar
-                if not rc and out.split()[0] == '0':
+                if not uname_rc and uname_out.split()[0] == '0':
+
                     if current_if['macaddress'] == 'unknown' and re.match('^en', current_if['device']):
                         entstat_path = self.module.get_bin_path('entstat')
                         if entstat_path:
@@ -2888,6 +2896,7 @@ def get_interfaces_info(self, ifconfig_path, ifconfig_options='-a'):
                                 buff = re.match('^Device Type:', line)
                                 if buff and re.match('.*Ethernet', line):
                                     current_if['type'] = 'ether'
+
                     # device must have mtu attribute in ODM
                     if 'mtu' not in current_if:
                         lsattr_path = self.module.get_bin_path('lsattr')


Code review comments:
diff_hunk: @@ -2866,12 +2874,12 @@ def get_interfaces_info(self, ifconfig_path, ifconfig_options='-a'):
                     self.parse_inet6_line(words, current_if, ips)
                 else:
                     self.parse_unknown_line(words, current_if, ips)
-            uname_path = self.module.get_bin_path('uname')
+
             if uname_path:
path: lib/ansible/module_utils/facts.py
text: @user1:
why not have the execution here and avoid the repeat of `if uname_path`


@author:
@user1 i'm not sure what you mean