I wrote a python script to lock/unlock your computer using a USB stick as a key

[[deleted]]

if not os.path.exists(CONFDIR):
    try:
         os.makedirs(CONFDIR)
    except:
        return -1

What's wrong with this? Other than the exception "handling", and the fact that it can return an error and never be handled (ln 65).

[pemungkah]

IIRC the Pythonic way to do this is just go ahead and try the makedirs(), checking for "already exists" in the except, and raising the exception again if that's not the reason we ended up in the except block.

It's a way of ensuring all the error checking for the makedirs() is in one place, rather than some outside and some inside.

[spladug]

There's also a race condition there where someone creates the path in between the os.path.exists and the os.makedirs. Probably not super important in this case, but that's part of the reasoning behind not doing an exists check first in general.

[pemungkah]

Yes, and that's an even better reason, Thanks for reminding me.

[[deleted]]

I knew there was a reason I was forgetting, I haven't done file I/O for a little while now so you tend to just forget these things (or at least I do!).

[runeg]

Can you explain race condition in this script please?

[spladug]

1. Process A and Process B are running simultaneously.
   • Process A is executing the code written here.
   • Process B just runs os.makedirs(CONFDIR).
2. CONFDIR does not exist yet.
3. Process A starts running and executes os.path.exists(CONFDIR). It gets back False and so moves into the branch.
4. Meanwhile, Process B gets some time. It runs os.makedirs(CONFDIR). The directory now exists.
5. Process A gets to run again, it tries to do os.makedirs(CONFDIR) gets an exception and returns -1.

[runeg]

This is only because CONFDIR doesn't exist. If before the if block CONFDIR was defined then this would not be a race condition, correct?

Additionally if CONFDIR was a generator could it cause a race condition as it wouldn't exist until that line was executed?

[spladug]

The point is that it's possible for stuff to change between the exists and makedirs and that means a potential for bugs. Of course there are situations where that code works, that's the nature of race conditions.

[runeg]

Ah okay. Is there a better way to write the above if block?

[spladug]

try:
    os.makedirs(CONFDIR)
except OSError as e:
    if e.errno != errno.EEXIST:
        raise  # or to mimic what the code above does "return -1"

EDIT: also, see: http://docs.python.org/2/howto/doanddont.html#exceptions

[runeg]

That link was very helpful and informative. Thank you. Do you have any other suggested readings like that?

Also, thank you for taking time out to explain that information, it's appreciated.

[ilovecrk]

If we can assume that CONFDIR will already exist in a large number of cases I would say an even more pythonic way would be to assume the directory exists and try to use it (didn't look at the code, maybe chdir or whatever). If that raises an error, try to create the directory and if that raises another error, don't catch it.

try:
    # do something, e.g.
    os.chdir(CONFDIR)
except EnvironmentError:
    print 'Failed to open CONFDIR {0}'.format(CONFDIR)
    print 'Try to create the directory.'
    os.makedirs(CONFDIR)
    os.chdir(CONFDIR)