3
\$\begingroup\$

The code takes srcPaths and actionType as parameters. The former is a string with tab-delimited file paths, while the latter is a string that may have values of "Lock", "Unlock", and "Toggle", which is chosen by the user.

#!/usr/bin/python

import os
import sys
import ntpath
import subprocess

srcPaths = sys.argv[1]
actionType = sys.argv[2]

srcPaths = srcPaths.split("\t")
srcPathsCount = len(srcPaths)

lockCount = unlockCount = 0

def fileIsLocked(srcPath):

    cmd1 = subprocess.Popen(["ls", "-ldO", srcPath], stdout=subprocess.PIPE)
    cmd2 = subprocess.Popen(["awk", "{ print $5 }"], stdin=cmd1.stdout, stdout=subprocess.PIPE)
    cmd1.stdout.close()

    if "uchg" in cmd2.communicate()[0]:
        return True
    else:
        return False

def setLock(flagStr, srcPath):

    global lockCount, unlockCount

    subprocess.call(["chflags", flagStr, srcPath])

    if flagStr == "uchg":
        lockCount += 1
    else:
        unlockCount += 1

for srcPath in srcPaths:

    if actionType == "Lock":

        if not fileIsLocked(srcPath):
            setLock("uchg", srcPath)

    elif actionType == "Unlock":

        if fileIsLocked(srcPath):
            setLock("nouchg", srcPath)

    else:

        if fileIsLocked(srcPath):
            setLock("nouchg", srcPath)
        else:
            setLock("uchg", srcPath)

sys.stdout.write("{}/{}/{}".format(lockCount, unlockCount, srcPathsCount))

The function fileIsLocked(srcPath) checks if the file is locked. It does this by using ls -ldO and awk to get the file flags. If it finds "uchg" there, then the function returns True, and vice versa. The function setLock(flagStr, srcPath) executes the shell script command to lock/unlock the file depending on the conditions, and keeps count of files that have been locked and unlocked.

Naturally, if the user wants to Lock the files, the file paths will be checked for unlockedness, and will proceed to lock those that are unlocked. If files are to be Unlocked, vice versa. If the user wants to Toggle, then if the files are locked, they are unlocked, and vice versa.

I'm not certain that the code is as concise as it needs to be. It seems to me like the code might be shortened or functions might be joined together. But I haven't been able to modify the code and still keep it working as intended.

What do you think could be done to improve the logic of the code?

\$\endgroup\$
  • 1
    \$\begingroup\$ I'd start by using the python naming convention on variable and method names i.e def file_is_locked(src_path): \$\endgroup\$ – AK47 May 27 '16 at 13:02
3
\$\begingroup\$
  1. I have absolutely no idea what the purpose of this code is. Something to do with locking and unlocking files, but why? Your code doesn't say, and neither does your post. It's hard to review code when I have no idea what it is supposed to do.

  2. There are no docstrings. What do these functions do? What arguments should I pass?

  3. Running shell commands and parsing their output is complex and slow. Instead, get the flags for a file by calling os.stat and set the flags for a file by calling os.chflags. Use the constants in the stat module, for example stat.UF_IMMUTABLE for the uchg flag.

    To read the flags for a file:

    import os, stat
    flags = os.stat(filename).st_flags
    

    To determine if a file is immutable, evaluate:

    bool(flags & stat.UF_IMMUTABLE)
    

    To set the immutable flag, call:

    os.chflags(filename, flags | stat.UF_IMMUTABLE)
    

    To clear the immutable flag, call:

    os.chflags(filename, flags & ~stat.UF_IMMUTABLE)
    
  4. Since you don't say what the purpose of this code is, I can't tell if it is a good implementation or not. If you are planning to use this mechanism for mutual exclusion (e.g. to prevent two instances of a program running at the same time) then it won't work, because it has race conditions (the flags might change between getting them and setting them).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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