[node-patches] Change in ovirt-node[master]: [RFC] process: Make use of shell explicit
fabiand at fedoraproject.org
fabiand at fedoraproject.org
Wed Jul 10 11:51:40 UTC 2013
Fabian Deutsch has uploaded a new change for review.
Change subject: [RFC] process: Make use of shell explicit
......................................................................
[RFC] process: Make use of shell explicit
Previously some methods in the process module implicitly used the shell
keyword-argument, some others didn't.
If the shell=True arg is used, then Popen will allow and interprete a
string as the cmd, e.g. "ls /tmp" and will do the right thing. If
shell=False the Popen will look for a command named "ls /tmp". Because
in the case of shell=False, Popen expects a list, the first item in the
list beeing the command itself, and the subsequent items beeing
arguments to that command.
This patch removes the implicit use if shell=True to have the same usage
pattern accross all functions in the process module.
This prepares to solve problems e.g. related to kdump.
Summarized:
Args need to be a list in the default (shell=False) case:
process.call(["ls", "/tmp"])
This is a save way of calling a function with arguments, because quoting
the args is done by python.
Args need to be a string in the shell=True case:
process.call("ls /tmp | grep something", shell=True)
The string will be interpreted by a shell, thus all shell features can
be used in that string, but also quotation is up to the user and
therefor considered as unsafe.
Change-Id: I3cbc920e07fd4c3890028fe2f2d006e1be7c2ae6
Signed-off-by: Fabian Deutsch <fabiand at fedoraproject.org>
---
M src/ovirt/node/config/defaults.py
M src/ovirt/node/config/network.py
M src/ovirt/node/setup/cim/cim_model.py
M src/ovirt/node/setup/core/kdump_page.py
M src/ovirt/node/setup/core/ping.py
M src/ovirt/node/setup/core/support_page.py
M src/ovirt/node/setup/puppet/puppet_page.py
M src/ovirt/node/setup/snmp/snmp_model.py
M src/ovirt/node/utils/console.py
M src/ovirt/node/utils/network.py
M src/ovirt/node/utils/process.py
M src/ovirt/node/utils/security.py
M src/ovirt/node/utils/system.py
M src/ovirt/node/utils/tuned.py
14 files changed, 59 insertions(+), 45 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-node refs/changes/65/16665/1
diff --git a/src/ovirt/node/config/defaults.py b/src/ovirt/node/config/defaults.py
index 56ae68f..a4d6762 100644
--- a/src/ovirt/node/config/defaults.py
+++ b/src/ovirt/node/config/defaults.py
@@ -426,7 +426,7 @@
rulesfile = "/etc/udev/rules.d/70-persistent-net.rules"
newrulesfile = "/etc/udev/rules.d/71-persistent-node-net.rules"
if File(rulesfile).exists():
- process.check_call("cp %s %s" % (rulesfile, newrulesfile))
+ process.check_call(["cp", rulesfile, newrulesfile])
fs.Config().persist(newrulesfile)
class StartNetworkServices(utils.Transaction.Element):
@@ -1305,7 +1305,7 @@
fs.Config().persist(nfsv4.configfilename)
system.service("rpcidmapd", "restart")
- process.check_call("nfsidmap -c")
+ process.check_call(["nfsidmap", "-c"])
tx = utils.Transaction("Configuring NFSv4")
tx.append(ConfigureNfsv4())
diff --git a/src/ovirt/node/config/network.py b/src/ovirt/node/config/network.py
index 319e37c..15a5bf3 100644
--- a/src/ovirt/node/config/network.py
+++ b/src/ovirt/node/config/network.py
@@ -196,7 +196,8 @@
"""Get or set DNS servers
>>> import ovirt.node.utils.process as p
- >>> stdout = p.pipe("egrep '^nameserver' /etc/resolv.conf | wc -l")
+ >>> stdout = p.pipe("egrep '^nameserver' /etc/resolv.conf | wc -l",
+ ... shell=True)
>>> len(nameservers()) == int(stdout)
True
"""
@@ -222,8 +223,8 @@
if new_hostname:
# hostnamectl set's runtime and config file
- utils.process.check_call("hostnamectl --static set-hostname %s" %
- new_hostname)
+ utils.process.check_call(["hostnamectl", "--static", "set-hostname",
+ new_hostname])
current_hostname = utils.fs.get_contents(hostnamefile)
if new_hostname and current_hostname != new_hostname:
diff --git a/src/ovirt/node/setup/cim/cim_model.py b/src/ovirt/node/setup/cim/cim_model.py
index fa4f09b..4f590d6 100644
--- a/src/ovirt/node/setup/cim/cim_model.py
+++ b/src/ovirt/node/setup/cim/cim_model.py
@@ -98,12 +98,13 @@
userinfo = pwd.getpwnam(username)
if not userinfo.pw_gid == grp.getgrnam(main_group).gr_gid:
process.check_call("usermod -g %s %s" %
- (main_group, username))
+ (main_group, username), shell=True)
if not userinfo.pw_shell == shell:
process.check_call("usermod -s %s %s" %
- (shell, username))
+ (shell, username), shell=True)
for group in group_list:
if username not in grp.getgrnam(group).gr_mem:
process.check_call("usermod -G %s %s" %
- (",".join(group_list), username))
+ (",".join(group_list), username),
+ shell=True)
break
diff --git a/src/ovirt/node/setup/core/kdump_page.py b/src/ovirt/node/setup/core/kdump_page.py
index 5552320..577fd79 100644
--- a/src/ovirt/node/setup/core/kdump_page.py
+++ b/src/ovirt/node/setup/core/kdump_page.py
@@ -159,7 +159,7 @@
txs += model.transaction()
with self.application.ui.suspended():
- utils.process.call("reset")
+ console.reset()
is_dry = self.application.args.dry
progress_dialog = console.TransactionProgress(txs, is_dry)
progress_dialog.run()
diff --git a/src/ovirt/node/setup/core/ping.py b/src/ovirt/node/setup/core/ping.py
index b76b3c4..90cb297 100644
--- a/src/ovirt/node/setup/core/ping.py
+++ b/src/ovirt/node/setup/core/ping.py
@@ -125,7 +125,7 @@
self.p.widgets["ping.do_ping"].enabled(False)
ui_thread.call(lambda: stdoutdump.text("Pinging ..."))
- out = process.pipe(self.cmd)
+ out = process.pipe(self.cmd, shell=True)
ui_thread.call(lambda: stdoutdump.text(out))
except:
self.p.logger.exception("Exception while pinging")
diff --git a/src/ovirt/node/setup/core/support_page.py b/src/ovirt/node/setup/core/support_page.py
index 718bdac..6bf5071 100644
--- a/src/ovirt/node/setup/core/support_page.py
+++ b/src/ovirt/node/setup/core/support_page.py
@@ -79,7 +79,8 @@
cmd = cmds[logfile] if logfile in cmds else None
if cmd:
- contents = process.check_output(cmd, stderr=process.STDOUT)
+ contents = process.check_output(cmd, shell=True,
+ stderr=process.STDOUT)
return ui.TextViewDialog("output.dialog", "Logfile",
contents)
diff --git a/src/ovirt/node/setup/puppet/puppet_page.py b/src/ovirt/node/setup/puppet/puppet_page.py
index 9029dd9..a398b4a 100644
--- a/src/ovirt/node/setup/puppet/puppet_page.py
+++ b/src/ovirt/node/setup/puppet/puppet_page.py
@@ -151,5 +151,5 @@
conf.write(line)
system.service("puppet", "stop")
- utils.process.check_call("puppet agent --test")
+ utils.process.check_call("puppet agent --test", shell=True)
system.service("puppet", "start")
diff --git a/src/ovirt/node/setup/snmp/snmp_model.py b/src/ovirt/node/setup/snmp/snmp_model.py
index 9aba2de..032997c 100644
--- a/src/ovirt/node/setup/snmp/snmp_model.py
+++ b/src/ovirt/node/setup/snmp/snmp_model.py
@@ -38,9 +38,9 @@
else:
conf = snmp_conf
cmd = "cat %s|grep createUser|awk '{print $4}'" % conf
- oldpwd, stderr = process.pipe(cmd)
+ oldpwd, stderr = process.pipe(cmd, shell=True)
oldpwd = oldpwd.stdout.read().strip()
- process.call("sed -c -ie '/^createUser root/d' %s" % snmp_conf)
+ process.call("sed -c -ie '/^createUser root/d' %s" % snmp_conf, shell=True)
f = open(snmp_conf, "a")
# create user account
f.write("createUser root SHA %s AES\n" % password)
@@ -51,9 +51,9 @@
pwd_change_cmd = (("snmpusm -v 3 -u root -n \"\" -l authNoPriv -a " +
"SHA -A %s localhost passwd %s %s -x AES") %
(oldpwd, oldpwd, password))
- process.check_call(pwd_change_cmd)
+ process.check_call(pwd_change_cmd, shell=True)
# Only reached when no excepion occurs
- process.call("rm -rf /tmp/snmpd.conf")
+ process.call(["rm", "-rf", "/tmp/snmpd.conf"])
ovirt_store_config(snmp_conf)
@@ -62,8 +62,9 @@
system.service("snmpd", "stop")
# copy to /tmp for enable/disable toggles w/o reboot
- process.check_call("cp /etc/snmp/snmpd.conf /tmp")
- process.check_call("sed -c -ie '/^createUser root/d' %s" % snmp_conf)
+ process.check_call(["cp", "/etc/snmp/snmpd.conf", "/tmp"])
+ process.check_call("sed -c -ie '/^createUser root/d' %s" % snmp_conf,
+ shell=True)
remove_config(snmp_conf)
diff --git a/src/ovirt/node/utils/console.py b/src/ovirt/node/utils/console.py
index e9c7a12..8c462dd 100644
--- a/src/ovirt/node/utils/console.py
+++ b/src/ovirt/node/utils/console.py
@@ -19,13 +19,17 @@
# MA 02110-1301, USA. A copy of the GNU General Public License is
# also available at http://www.gnu.org/copyleft/gpl.html.
from ovirt.node import base
-from ovirt.node.utils import Transaction
+from ovirt.node.utils import Transaction, process
import StringIO
import sys
import termios
import tty
+def reset():
+ process.call(["reset"])
+
+
def writeln(txts):
"""Write something to stdout
A wrapper if we want to do this differently in future
diff --git a/src/ovirt/node/utils/network.py b/src/ovirt/node/utils/network.py
index 7343654..f4e1f29 100644
--- a/src/ovirt/node/utils/network.py
+++ b/src/ovirt/node/utils/network.py
@@ -295,7 +295,7 @@
# Fallback
cmd = "ip -o addr show {ifname}".format(ifname=self.ifname)
- for line in process.pipe(cmd).split("\n"):
+ for line in process.pipe(cmd, shell=True).split("\n"):
token = re.split("\s+", line)
if re.search("\sinet[6]?\s", line):
addr, mask = token[3].split("/")
@@ -326,7 +326,7 @@
def identify(self):
"""Flash the lights of this NIC to identify it
"""
- utils.process.call("ethtool --identify %s 10" % self.ifname)
+ utils.process.call(["ethtool", "--identify", self.ifname, "10"])
def __str__(self):
return self.build_str(["ifname"])
@@ -609,7 +609,7 @@
def _default_fallback(self):
# Fallback
gw = None
- cmd = "ip route list"
+ cmd = ["ip", "route", "list"]
for line in process.pipe(cmd).split("\n"):
token = re.split("\s+", line)
if line.startswith("default via"):
@@ -706,8 +706,8 @@
The current hostname
"""
if new_hostname:
- utils.process.call("hostname %s" % new_hostname)
- return utils.process.pipe("hostname").strip()
+ utils.process.call(["hostname", new_hostname])
+ return utils.process.pipe(["hostname"]).strip()
# http://git.fedorahosted.org/cgit/smolt.git/diff/?
@@ -800,8 +800,8 @@
def delete(self, ifname):
if not self.is_bridge(ifname):
raise RuntimeError("Can no delete '%s', is no bridge" % ifname)
- process.call("ifconfig %s down" % ifname)
- process.call("ip link delete %s type bridge" % ifname)
+ process.call(["ip", "link", "set", "dev", ifname, "down"])
+ process.call(["ip", "link", "delete", ifname, "type", "bridge"])
class Bonds(base.Base):
@@ -828,4 +828,4 @@
def delete_all(self):
"""Deletes all bond devices
"""
- process.call(["rmmod", "bonding"], shell=False)
+ process.call(["rmmod", "bonding"])
diff --git a/src/ovirt/node/utils/process.py b/src/ovirt/node/utils/process.py
index 1ddcd5f..5de36ab 100644
--- a/src/ovirt/node/utils/process.py
+++ b/src/ovirt/node/utils/process.py
@@ -31,8 +31,7 @@
LOGGER = logging.getLogger(__name__)
COMMON_POPEN_ARGS = {
- "close_fds": True,
- "shell": True
+ "close_fds": True
}
CalledProcessError = subprocess.CalledProcessError
@@ -83,7 +82,7 @@
return pipe(*args)
-def pipe(cmd, stdin=None):
+def pipe(cmd, stdin=None, **kwargs):
"""Run a non-interactive command and return it's output
Args:
@@ -96,5 +95,6 @@
return unicode(popen(cmd,
stdin=PIPE,
stdout=PIPE,
- stderr=STDOUT
+ stderr=STDOUT,
+ **kwargs
).communicate(stdin)[0])
diff --git a/src/ovirt/node/utils/security.py b/src/ovirt/node/utils/security.py
index 3dc737c..f4e590e 100644
--- a/src/ovirt/node/utils/security.py
+++ b/src/ovirt/node/utils/security.py
@@ -19,6 +19,7 @@
# MA 02110-1301, USA. A copy of the GNU General Public License is
# also available at http://www.gnu.org/copyleft/gpl.html.
from ovirt.node import base, valid, utils
+from ovirt.node.utils import system
from ovirt.node.utils.fs import File
import PAM as _PAM # @UnresolvedImport
import cracklib
@@ -94,11 +95,13 @@
additional_lines = []
ofunc.unmount_config("/etc/profile")
- process.check_call("sed -i '/OPENSSL_DISABLE_AES_NI/d' /etc/profile")
+ process.check_call("sed -i '/OPENSSL_DISABLE_AES_NI/d' /etc/profile",
+ shell=True)
if disable_aes:
additional_lines += ["export OPENSSL_DISABLE_AES_NI=1"]
- process.check_call("sed -i '/SSH_USE_STRONG_RNG/d' /etc/profile")
+ process.check_call("sed -i '/SSH_USE_STRONG_RNG/d' /etc/profile",
+ shell=True)
if rng_num_bytes:
additional_lines += ["export SSH_USE_STRONG_RNG=%s" %
rng_num_bytes]
@@ -142,7 +145,7 @@
def restart(self):
self.logger.debug("Restarting SSH")
- process.call("service sshd restart &>/dev/null")
+ system.service("sshd", "restart")
def password_authentication(self, enable=None):
"""Get or set the ssh password authentication
@@ -176,7 +179,8 @@
hostkey = File(fn_hostkey).read()
hostkey_fp_cmd = "ssh-keygen -l -f '%s'" % fn_hostkey
- fingerprint = process.pipe(hostkey_fp_cmd).strip().split(" ")[1]
+ out = process.pipe(hostkey_fp_cmd, shell=True)
+ fingerprint = out.strip().split(" ")[1]
return (fingerprint, hostkey)
diff --git a/src/ovirt/node/utils/system.py b/src/ovirt/node/utils/system.py
index a841dde..3bd969d 100644
--- a/src/ovirt/node/utils/system.py
+++ b/src/ovirt/node/utils/system.py
@@ -40,7 +40,7 @@
def reboot():
"""Reboot the system
"""
- process.call("reboot")
+ process.call(["reboot"])
def async_reboot(delay=3):
@@ -51,7 +51,7 @@
def poweroff():
"""Poweroff the system
"""
- process.call("poweroff")
+ process.call(["poweroff"])
def is_efi():
@@ -131,7 +131,8 @@
if os.path.isdir("%s/%s" % (orig, f)):
if not os.path.exists("%s/%s" % (target, f)):
process.call("cp -av %s/%s %s &>/dev/null" % (orig, f,
- target))
+ target),
+ shell=True)
else:
copy_dir_if_not_exist("%s/%s" % (orig, f), "%s/%s" % (target,
f))
@@ -298,7 +299,7 @@
def set_layout(self, layout):
assert layout
if has_systemd():
- utils.process.call("localectl set-keymap %s" % layout)
+ utils.process.call(["localectl", "set-keymap", layout])
else:
self.kbd.set(layout)
self.kbd.write()
@@ -372,7 +373,7 @@
# the following lines are all executed in a background daemon
time.sleep(delay)
cmd = which("reboot")
- subprocess.call(cmd, close_fds=True)
+ subprocess.call(cmd, shell=True)
except:
self.logger.info("Scheduling Reboot")
diff --git a/src/ovirt/node/utils/tuned.py b/src/ovirt/node/utils/tuned.py
index 4be10fa..2770e48 100644
--- a/src/ovirt/node/utils/tuned.py
+++ b/src/ovirt/node/utils/tuned.py
@@ -34,7 +34,8 @@
A list of profiles
"""
prof_list = [u'None']
- for i in process.check_output("/usr/sbin/tuned-adm list").split("\n"):
+ lines = process.check_output(["/usr/sbin/tuned-adm", "list"]).split("\n")
+ for i in lines:
if "- " in i:
prof_list.append(i.replace("- ", ""))
return prof_list
@@ -47,7 +48,7 @@
A string with the active tuned profile
"""
try:
- profile = process.check_output("/usr/sbin/tuned-adm active")
+ profile = process.check_output(["/usr/sbin/tuned-adm", "active"])
except:
return "None"
return re.match(r'.*?: (.*)', profile).group(1)
@@ -61,15 +62,15 @@
"""
try:
if (profile == "None" or profile == "off"):
- ret = process.check_call("/usr/sbin/tuned-adm off")
+ ret = process.check_call(["/usr/sbin/tuned-adm", "off"])
if not ret == 0:
raise Exception("DISABLE")
raise Exception("Failed to disable tuned")
elif profile not in get_available_profiles():
raise Exception("%s is not a known profile" % profile)
else:
- ret = process.check_call("/usr/sbin/tuned-adm profile %s"
- % profile)
+ ret = process.check_call(["/usr/sbin/tuned-adm", "profile",
+ profile])
if not ret == 0:
raise Exception("Failed to set profile to %s" % profile)
except Exception as e:
--
To view, visit http://gerrit.ovirt.org/16665
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3cbc920e07fd4c3890028fe2f2d006e1be7c2ae6
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Fabian Deutsch <fabiand at fedoraproject.org>
More information about the node-patches
mailing list