Added logging of additional debugging information to nsenter function #17
Conversation
Previosuly we haven't logged the output of the STDERR, only the STDOUT. Additionally we haven't logged the return code. This patch fixes that. Signed-off-by: Lev Veyde <lveyde@redhat.com>
lveyde
force-pushed
the
log-stderr-and-returncodes
branch
from
a58883f to
6595889
Compare
| stdout, stderr = proc.communicate() | ||
| log.debug("STDOUT: %s", repr(stdout)) | ||
| log.debug("STDERR: %s", repr(stderr)) | ||
| log.debug("ReturnCode: %s", repr(proc.returncode)) |
There was a problem hiding this comment.
This works but having single log like we have in vdsm can be more clear:
log.debug("Command %s terminated with return code %s, out %r, err %r",
arg, proc.returncode, out, err)
We don't need to use repr(obj), %r does it for us. There is no need to do repr of the return code, it is an integer.
There was a problem hiding this comment.
The output normally is already long enough, and frequently is multi-line.
So pushing it into one line will make it even less visible.
There was a problem hiding this comment.
Same, marked as resolved but I don't see any change in the code or reply.
There was a problem hiding this comment.
This works but having single log like we have in vdsm can be more clear:
log.debug("Command %s terminated with return code %s, out %r, err %r", arg, proc.returncode, out, err)We don't need to use repr(obj), %r does it for us. There is no need to do repr of the return code, it is an integer.
Potentially the returncode can be None if for some weird reason the process hasn't been ended by the time we return from communicate(), so I preferred to use repr() on it as well.
There was a problem hiding this comment.
The return code cannot be None when communicate() returns. It returns after the process terminated and and the child was reaped.
https://docs.python.org/3.6/library/subprocess.html#subprocess.Popen.communicate
Regardless, logging None with %s is the same as logging with %r:
>>> "%s %s %r %r" % (None, None, 0, 0)
'None None 0 0'
You did not answer about using repr() instead of %r, I guess you missed this. This should be fixed.
There was a problem hiding this comment.
Since this is already merged, I'll open a new PR for that.
There was a problem hiding this comment.
Sent PR#18 ( #18 ) to address this.
Previosuly we haven't logged the output of the STDERR, only the STDOUT, while calling the nsenter command.
Additionally we haven't logged the return code.
This patch fixes that.
Signed-off-by: Lev Veyde lveyde@redhat.com
Changes introduced with this PR
Are you the owner of the code you are sending in, or do you have permission of the owner?
[y]