Python - Using variables in subprocess.Popen command

Question

I'm new to coding and need some assistance. I am writing a python script that will go through the contents of a directory and as its walks through the directory it will send each file to a Bluetooth device.

It works fine if I specify the filename but I can’t get it to work by using the file name as a variable. Here is the code below

import os
import time
import subprocess

indir = '\\\\10.12.12.218\\myshare'
for  root, dirs, filenames in os.walk(indir):
   for file in filenames:
      print (file)
      subprocess.Popen('ussp-push /dev/rfcomm0 image1.jpg file.jpg', shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print ('end') 

I am trying to replace 'image1.jpg' in the command with the variable 'file' like below but have not been successful.

subprocess.Popen('ussp-push /dev/rfcomm0', file, 'file.jpg', shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

Will really appreciate any help.

Answer 1

There are several issues:

• shell=True is unnecessary. Drop it and use a list argument:

  import shlex
  
  args = shlex.split('ussp-push /dev/rfcomm0 image1.jpg file.jpg')
  

• you are trying to pass command-line arguments as separate arguments for Popen. Use Popen(['echo', 'a']) instead of Popen('echo', 'a'). The later is wrong completely. See Popen() function signature in the docs

• do not use stdout=PIPE and/or stderr=PIPE unless you read from p.stdout/p.stderr pipes otherwise your child process may block forever if it fills any of the OS pipe buffers

• save a reference to Popen() to wait for its status later. It is optional but it helps to avoid creating too many zombies

You could extract the file generating part into a separate function:

import os

def get_files(indir, extensions=('.jpg', '.png')):
    """Yield all files in `indir` with given `extensions` (case-insensitive)."""
    for root, dirs, files in os.walk(indir):
        for filename in files:
            if filename.casefold().endswith(extensions):
               yield os.path.join(root, filename)

Then to execute commands for each file in parallel:

from subprocess import CalledProcessError, Popen

indir = r'\\10.12.12.218\myshare'
commands = [['ussp-push', '/dev/rfcomm0', path] for path in get_files(indir)]

# start all child processes
children = [Popen(cmd) for cmd in commands]

# wait for them to complete, raise an exception if any of subprocesses fail
for process, cmd in zip(children, commands):
    if process.wait() != 0:
       raise CalledProcessError(process.returncode, cmd)        

If you don't want to run the commands in parallel then just use subprocess.call instead of subprocess.Popen:

import subprocess

indir = r'\\10.12.12.218\myshare'
statuses = [subprocess.call(['ussp-push', '/dev/rfcomm0', path])
            for path in get_files(indir)]
if any(statuses):
   print('some commands have failed')

It runs one command at a time.

Answer 2

Try this:

subprocess.Popen(
    ['ussp-push', '/dev/rfcomm0', file, 'file.jpg'],
     stdout=subprocess.PIPE, 
     stderr=subprocess.PIPE)

You want to pass to Popen() a list of strings. An alternative would be to build a space-separate command such as:

subprocess.Popen(
    'ussp-push /dev/rfcomm0 "{0}" file.jpg'.format(file) # replace {0} with file
     stdout=subprocess.PIPE, 
     stderr=subprocess.PIPE)

Is shell=True Unsafe?

I would like to make a couple of points regarding the use of shell=True.

1. In this case, as m.wasowski points out in the comment, it is not necessary.
2. shell=True is unsafe if you don't have any control on the command. For example, if you take the command from user's input, then the user can pass you something like sudo rm -fr /.
3. It is unsafe because once invoke the shell, the PATH might be different. When you issue a command such as ls, it might not come from the usual place (/bin/ls) but some malicious places such as /home/evil/bin

That being said, shell=True is safe if you have control over the command, in this case, /dev/rfcomm0--you define what the command is, instead of receiving it from elsewhere. Thank you m.wasowski for bringing up this point.

Update

Remove shell=True. See comments.

Answer 3

So, there are a bunch of files and you want to run a command once with each file as an argument. I think the following should work well.

import os
import time
import subprocess

indir = '\\\\10.12.12.218\\myshare'
for  root, dirs, filenames in os.walk(indir):
    for file in filenames:
        print 'Sending', os.path.join(root, file)
        subprocess.check_call(['ussp-push', '/dev/rfcomm0', os.path.join(root, file), 'file.jpg'])
print ('end')

Here's the changes I made:

1. I'm using the check_call function instead of Popen as you wanted to run the commands sequentially and not in parallel. The check_call function takes the same arguments as Popen, but waits for the process to finish and raises an exception if the process fails.
2. I am passing an array (written in square brackets) as the first argument, the command to check_call. This also means there is no need for a shell to interpret a command string and so I removed the shell=True. The first item in this array is the command the rest are commands passed to it.
3. The last item in the array is the full path to the file. The file variable only holds the file's name. But we need its path as it might lie somewhere deep inside a folder (as you are walking recursively). The os.path.join joins two strings with a \ or / as appropriate in the platform.
4. I also removed the stdout and stderr arguments. This means the output and errors from the command are going to turn up in the command line, which is probably what you want. The stdout and stderr arguments make sense when you want to read the output of the command and process it yourself instead of showing it in the terminal.