I have lots of external shell commands to run.

So I gave every command a name and then run it.

tar_report_cmd = 'zip -r  {0} {1} '%(report_file, _export_dir)
mv_report_cmd = "mv {0} {1}".format(report_file,report_dir)

But it looks sort of redundant and not beautiful.

Is there any better practice to replace my way?

def exec_proc(cmd_str):
    results = os.popen(cmd_str).read()
    return results

closed as off-topic by Stephen Rauch, Daniel, Zeta, Billal Begueradj, Graipher Jul 6 '18 at 9:05

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Stephen Rauch, Daniel, Zeta, Billal Begueradj, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ For the examples given, I would suggest using the library support for zip files and moving files. It's more cross-platform and gives you better precision and error recovery options. Tend towards calling other commands only when library versions are unavailable and do exactly what you need. \$\endgroup\$ – Michael Urman Feb 18 '14 at 13:20
  • 5
    \$\begingroup\$ This question appears to be off-topic because it is primarily about how to write code, rather than really reviewing code that has been written. \$\endgroup\$ – Michael Urman Feb 18 '14 at 13:21
  • \$\begingroup\$ @MichaelUrman The code may be written at a novice level, but it's still reviewable code. We're all here to learn how to write better code, and this question is no different. \$\endgroup\$ – 200_success Feb 18 '14 at 23:38
  • 2
    \$\begingroup\$ @MichaelUrman Also, your comment that this should be done using a Python library rather than an external command should be posted as an answer, since it is a suggestion that would result in an improvement. \$\endgroup\$ – 200_success Feb 18 '14 at 23:38
  • 1
    \$\begingroup\$ @200_success I agree that if it were an on-topic question, my comment should be posted as an answer. But I see this as asking if it's a good approach, rather than asking for a code review. Note that the code given doesn't even work (what's report_file, _export_dir, ...?). \$\endgroup\$ – Michael Urman Feb 19 '14 at 2:50

No, it is a security risk, and furthermore, os.popen() has been deprecated since Python 2.6. The replacement is to use the subprocess module.

The security risk is that os.popen() expects its command as a string. That means that the command will be interpreted by a shell to split it up into the command and its arguments. If report_file, report_dir, or _export_dir contain any spaces or characters with a special meaning to the shell, horrible unexpected things can happen, including arbitrary command execution.


Not the answer you're looking for? Browse other questions tagged or ask your own question.