From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has uploaded a new change for review.
Change subject: move exec_sync_bytes to common.cmdutils ......................................................................
move exec_sync_bytes to common.cmdutils
exec_sync_bytes is useful not only in the network subpackage, so it needs to reside under vdsm.common.
Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be Signed-off-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/common/cmdutils.py M lib/vdsm/network/cmd.py 2 files changed, 16 insertions(+), 15 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/79693/1
diff --git a/lib/vdsm/common/cmdutils.py b/lib/vdsm/common/cmdutils.py index 85a4ae0..26e08a7 100644 --- a/lib/vdsm/common/cmdutils.py +++ b/lib/vdsm/common/cmdutils.py @@ -22,6 +22,8 @@ import os import re
+from vdsm.common.compat import CPopen as Popen +
SYSTEMD_RUN = "/usr/bin/systemd-run"
@@ -106,3 +108,16 @@ CPU = 'CPU' Memory = 'Memory' BlockIO = 'BlockIO' + + +def exec_sync_bytes(cmds): + logging.debug(cmdutils.command_log_line(cmds)) + + p = Popen( + cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + + out, err = p.communicate() + + logging.debug(cmdutils.retcode_log_line(p.returncode, err=err)) + + return p.returncode, out, err diff --git a/lib/vdsm/network/cmd.py b/lib/vdsm/network/cmd.py index f6d9b73..bf5d537 100644 --- a/lib/vdsm/network/cmd.py +++ b/lib/vdsm/network/cmd.py @@ -23,8 +23,7 @@ import uuid
from vdsm.common import cmdutils -from vdsm.common.cmdutils import systemd_run -from vdsm.common.compat import CPopen as Popen +from vdsm.common.cmdutils import systemd_run, exec_sync_bytes from vdsm.network import py2to3
@@ -50,16 +49,3 @@ cmds = systemd_run(cmds, scope=True, unit=unit, slice=slice_name)
return exec_sync(cmds) - - -def exec_sync_bytes(cmds): - logging.debug(cmdutils.command_log_line(cmds)) - - p = Popen( - cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - - out, err = p.communicate() - - logging.debug(cmdutils.retcode_log_line(p.returncode, err=err)) - - return p.returncode, out, err
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: move exec_sync_bytes to common.cmdutils ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/79693/4/lib/vdsm/common/cmdutils.py File lib/vdsm/common/cmdutils.py:
Line 111: Memory = 'Memory' Line 112: BlockIO = 'BlockIO' Line 113: Line 114: Line 115: def exec_sync_bytes(cmds):
Please rename it to run().
Are you asking me for a (limited) backport of python3's subprocess.run? If so, why?
I am looking for a bare-bone version of execCmd, to be used in network/* and hooks.py; Using run(check=True) to achieve cleaner code all over Vdsm should not be part of this task.
It is important for me to express that my out/err is kept in bytes. Each user should know that it is responsible to decoding, if needed. Line 116: logging.debug(command_log_line(cmds)) Line 117: Line 118: p = Popen( Line 119: cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: move exec_sync_bytes to common.cmdutils ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/79693/4/lib/vdsm/common/cmdutils.py File lib/vdsm/common/cmdutils.py:
Line 111: Memory = 'Memory' Line 112: BlockIO = 'BlockIO' Line 113: Line 114: Line 115: def exec_sync_bytes(cmds):
I just suggested a nicer name. I don't want to have name like exec_sync_byt
I don't know what you mean by default behavior.
It is important to keep encoding mess local to function that communicate with the external world. However, some external commands may return non-ut8 bytestring, so we cannot decode at this low level.
I feel very bad about this, and want to warn the users of this function about this fact.
The problem with a nicer name like `run` is that may lead someone (e.g. me) to understand it has a signature similar to that of subprocess.run.
How about a plain exec_cmd()? Line 116: logging.debug(command_log_line(cmds)) Line 117: Line 118: p = Popen( Line 119: cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd ......................................................................
Patch Set 6: Verified+1
OST http://jenkins.ovirt.org/view/oVirt%20system%20tests/job/ovirt-system-tests_... is happy
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd ......................................................................
Patch Set 8: Verified+1
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/79693/8/lib/vdsm/network/cmd.py File lib/vdsm/network/cmd.py:
Line 51 Line 52 Line 53 Line 54 Line 55
Dan, did you miss my comment about the docstring and the warnings needed he
yep
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd ......................................................................
Patch Set 10: Verified+1
OST passes http://jenkins.ovirt.org/view/oVirt%20system%20tests/job/ovirt-system-tests_...
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has submitted this change and it was merged. ( https://gerrit.ovirt.org/79693 )
Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd ......................................................................
move exec_sync_bytes to common.cmdutils.exec_cmd
exec_cmd(), a skinny version of our ancient over-grown execCmd, is useful not only in the network subpackage, so it needs to reside under vdsm.common.
Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be Signed-off-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/common/cmdutils.py M lib/vdsm/network/cmd.py M tests/common/cmdutils_test.py 3 files changed, 53 insertions(+), 17 deletions(-)
Approvals: Piotr Kliczewski: Looks good to me, approved Nir Soffer: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Dan Kenigsberg: Verified Francesco Romani: Looks good to me, but someone else must approve Edward Haas: Looks good to me, but someone else must approve
vdsm-patches@lists.fedorahosted.org