> On June 13, 2016, 1:03 p.m., Andrew Onischuk wrote:
> > ambari-common/src/main/python/ambari_commons/firewall.py, line 123
> > <https://reviews.apache.org/r/48309/diff/1/?file=1408121#file1408121line123>
> >
> >     Can you please explain why did we remove the check for firewalld?
> 
> Masahiro Tanaka wrote:
>     Thank you for reviewing.
>     Acutually I didn't remove the check for firewalld. `systemctl is-active` 
> can take multiple arguments.
>     This is a sample output on CentOS7.2
>     
>     ```
>     # systemctl is-active iptables firewalld
>     inactive
>     active
>     # systemctl is-active iptables
>     inactive
>     # systemctl is-active firewalld
>     active
>     ```
> 
> Andrew Onischuk wrote:
>     Oh, I see.
>     
>     So:
>     if first  is active and second is inactive what is the return code of 
> command?
>     if second is active and first is  inactive what is the return code of 
> command?
> 
> Masahiro Tanaka wrote:
>     Accoding to the `man systemctl`:
>     ```
>     is-active PATTERN...
>         Check whether any of the specified units are active (i.e. running). 
> Returns an exit
>         code 0 if at least one is active, or non-zero otherwise. Unless 
> --quiet is specified,
>         this will also print the current unit state to standard output.
>     ```
>     
>     We can expect that if at least one unit is active, then return code (exit 
> code) equals 0.
>     
>     BUT there is a bug in *systemd*. See [this PR on 
> GitHub](https://github.com/systemd/systemd/pull/2430)
>     
>     So the actual behavior on CentOS 7.2 is below:
>     ```
>     # systemctl is-active iptables firewalld
>     inactive
>     active
>     # systemctl is-active iptables
>     inactive
>     # systemctl is-active firewalld
>     active
>     # systemctl is-active iptables firewalld
>     inactive
>     active
>     # echo $?
>     3
>     # systemctl is-active firewalld iptables
>     active
>     inactive
>     # echo $?
>     3
>     ```
>     
>     That's why I checked if stdout includes the string `active` in my code.
> 
> Andrew Onischuk wrote:
>     What will happen is firewalld or iptables is absent as a services, could 
> that be the case for centos7?
> 
> Andrew Onischuk wrote:
>     Basically try doing something like this:
>     
>     # systemctl is-active firewalld somethingunknown
>     # echo $?
> 
> Masahiro Tanaka wrote:
>     Here
>     ```
>     # systemctl is-active firewalld somethingunknown
>     active
>     unknown
>     # echo $?
>     3
>     # systemctl is-active somethingunknown firewalld
>     unknown
>     active
>     # echo $?
>     3
>     ```

Thanks!


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48309/#review137283
-----------------------------------------------------------


On June 7, 2016, 11:11 a.m., Masahiro Tanaka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48309/
> -----------------------------------------------------------
> 
> (Updated June 7, 2016, 11:11 a.m.)
> 
> 
> Review request for Ambari, Andrew Onischuk, Dmytro Sen, Florian Barca, and 
> Yusaku Sako.
> 
> 
> Bugs: AMBARI-17047
>     https://issues.apache.org/jira/browse/AMBARI-17047
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> In firewall.py, `systemctl is-active iptables || systemctl is-active 
> firewalld` is passed to `run_in_shell` function, which splits cmd string by 
> using `shlex.split`.
> 
> run_in_shell function finally calls `subprocess.Popen` with `shell=True`, so 
> the cmd string is evaluated like `Popen(['/bin/sh', '-c', 'systemctl', 
> 'is-active', 'iptables', '||', 'systemctl', 'is-active', 'firewalld'])`. This 
> doesn't returns values as expected, because after args[1] (in this case, 
> after the first `is-active`) are evaluated as sh arguements.
> 
> `systemctl is-active` can take multiple arugments, so we can use it.
> 
> 
> Diffs
> -----
> 
>   ambari-common/src/main/python/ambari_commons/firewall.py 72e6d26 
> 
> Diff: https://reviews.apache.org/r/48309/diff/
> 
> 
> Testing
> -------
> 
> mvn clean test & manual test
> 
> 
> Thanks,
> 
> Masahiro Tanaka
> 
>

Reply via email to