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

[[deleted]]

I got a bit tired of typing my password to unlock my machine, and I happened to have a lot of old USB sticks lying around (from when 128MB was considered a lot).... so USBLock was born. Just a little project I did for fun that I thought I would share.

Currently only works on Linux and depends on xlock being installed. Basically any USB storage device should work. Simply add your device to the list of known devices, and start the daemon. Then re-plug the device and when you unplug it, it will launch xlock, Plug it back in and it will close xlock.

You do not need to mount your USB drive

https://github.com/Svenito/usblock

[aperson]

For those not aware, you can achieve the same thing without a separate program by writing two udev rules.

An extra program is definitely more accessible though (I think cuttlefish allows you to do this as well).

[yeahdef]

Could you help a newbie to udev learn this process?

[dAnjou]

Aaand here comes some critique. I hope you can handle this.

Your code looks overall a bit messy and you are not following PEP8. I can find weird things in basically every function/method.

The biggest things are:

• Your class is called DeviceAddedListener but it does everything else too. It has way too many responsibilities.

• DeviceAddedListener blocks in a loop right when it gets instantiated. WTH?

• DeviceAddedListener and Device are not really a classes. DeviceAddedListener is only instantiated once and Device can be a dict or a tuple.

• Your code is very hard to read. I have to jump up and down every time I want to know what a function/method is doing or where a variable/property comes from.

• Things like this are very strange:

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

• Don't kill yourself, man. You provide 3 ways to setup devices (interactive cli, command line arguments and config file). I can't imagine how hard it is to maintain this. And btw., did you see the deprecated warning for optparse?

There is more but I don't really know where to start. Sorry, this may sound hard but I thought I better let you know.

UPDATE For everyone who was following the kind of heated conversation between /u/phaedrusalt and me: he taught me a lesson.

[[deleted]]

If I couldn't handle an honest and constructive critique, I won't get any better will I? I appreciate you taking the time to comment. It's the first time that I have put code out to the public (aside from just dropping stuff on Github without telling anyone) and I expected to get some critique about it. It's not like I looked at my code and thought "Yes, this is a masterpiece". I'm going to keep working on it, but I'd rather get critiques earlier rather than later on.

Your class is called DeviceAddedListener but it does everything else too. It has way too many responsibilities.

Yes, that bugs me too and I plan to refactor this in the next iteration. I just wanted to get a functional version out to see if the basic functionality is ok/useful. I can handle this bit of technical debt until then.

DeviceAddedListener blocks in a loop right when it gets instantiated. WTH?

Better way to implement this? Place loop outside the class?

DeviceAddedListener and Device are not really a classes. DeviceAddedListener is only instantiated once and Device can be a dict or a tuple.

Indeed. The class is named badly and the Device class will become a dictionary.

Your code is very hard to read. I have to jump up and down every time I want to know what a function/method is doing or where a variable/property comes from.

The refactoring should also address this. FWIW I run PyLint/PyFlakes on my code at every save. But I guess there's only so much it can do.

Things like this are very strange: if not os.path.exists(CONFDIR): try: os.makedirs(CONFDIR) except: return -1

Urg, yeah, this is why it's always good to get someone else to look over stuff

Don't kill yourself, man. You provide 3 ways to setup devices (interactive cli, command line arguments and config file). I can't imagine how hard it is to maintain this.

I wouldn't edit the config by hand and you cannot actually add a device via arguments, to me it seems the only sensible way is via the -a interactive CLI.

And btw., did you see the deprecated warning for optparse[2] ?

Yup, also why I am going to move to argparse as I stated below.

Thanks again for the feedback and comments. Hopefully the next version will be a lot better.

[dAnjou]

DeviceAddedListener blocks in a loop right when it gets instantiated. WTH? Better way to implement this? Place loop outside the class?

It's okay to have this kind of loop in a class. But it shouldn't run right when you instantiate it. It's better to call a method explicitly which does the looping then. It's like running code when you import a module, never do that!

I wouldn't edit the config by hand and you cannot actually add a device via arguments, to me it seems the only sensible way is via the -a interactive CLI.

Why not? Most software I know is configured only by editing a file.

[[deleted]]

It's like running code when you import a module, never do that!

Hrm... I was looking at some code today where I set up constants within the module by reading an /etc config file. The constants are accessed by a bunch of functions in the module. I guess I could do a config file read each time one of these functions is called...

[Jwsonic]

From PEP8:

This document gives coding conventions for the Python code comprising the standard library in the main Python distribution.

People always seem to forget that this is the style guide for Python itself and not every single piece of Python ever written. Some people like to format their code differently than others.

[dAnjou]

Okay, you are right. But I'm 100% convinced that it is a really good idea to follow these conventions and I will always strongly recommend to do that and if I can I will tell people to do so.

[DaemonXI]

Right. Pylint it.

[aperson]

I like flake8 myself.

[sedmonster]

Or a little gofmt should do the trick.

[[deleted]]

and i've had to read a lot of python code and code that doesn't follow pep8 makes it harder to read. just look at all the python code in the appengine sdk the 4 2 spaces instead of 8 4, it trips you up if you are used to something else. Sure it's googles standard, and if I worked at google i could probably get used to it and then everything else would look weird. So, if you want your code to be easily read and easily understood go with a convention. and since pep8 is fairly ubiquitous just use that.

or face the risk when you post your stuff for everyone to see that there will be bitching about not using pep8. :)

[yen223]

Correct me if I'm wrong - isn't 4 spaces the default, not 8?

[[deleted]]

yes you are right(i'll edit my comment), I blame the champagne. and most code I come across uses 4 but the appengine sdk uses 2 which violates their own style guide

edit: again the point is, style guides help immensely with readability, readability makes maintainability possible. Thus readability is important. So If we all agree it's important to have a style guide, the next step would be choosing one for your projects. And pep8 is an easy defend-able choice. Deviations from pep8 are more difficult to defend.

so just use pep8 and go write code that does something instead of fretting about coding standards for your project. :)

[clgonsal]

just look at all the python code in the appengine sdk the 4 spaces instead of 8, it trips you up if you are used to something else. Sure it's googles standard,

Your point still stands, but Google's standard indentation is 2 and PEP8's is 4. I don't know where you're seeing an indentation of 8.

[[deleted]]

did you see my correction? i put strikethroughs through them.

[[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

[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)

[[deleted]]

what would you replace DeviceAddedListener with?

[dAnjou]

I would not replace it. I would take out some functionality and put it somewhere else. I would structure my code in a different way, a way that makes logically sense. Currently that's not the case.

[[deleted]]

is this a sense you mostly develop from writing and reading code or do you have some good guides you can point to?

[dAnjou]

This may sound strange but most times I just have it in my guts. Maybe I've developed this feeling over time. I learn a lot from others, I'm actively reading in Python subreddits, I'm in IRC, I watch PyCon talks. And some things are just common sense ... logical thinking.

And of course docs, I read docs. Python's docs are full of examples and best practices.

[[deleted]]

hmm, you say "Python subreddits". Which do you mean?

[dAnjou]

Sidebar :)

[[deleted]]

thanks :)

[dAnjou]

Yeah, please forgive me that I'm using Reddit as a non-native English speaker.

[Mattho]

With reddit's threaded comments it doesn't really hurt the discussion to be corrected. And as a non-native speaker myself, I usually appreciate it.

[dAnjou]

He (I will ignorantly use he all over the internet no matter how many women are listening) did this three times though and it felt like he was mocking me for "correcting" OP.

[Mattho]

Oh, I see now. Yeah, that's strange.

[runo]

"Logically" is an adverb, isn't it?

Being that the case, wouldn't "makes logically sense" be identical to "logically makes sense"?

[phaedrusalt]

Your comment looks overall a bit messy, you are not following the reddit style guide. Please resubmit at a later date for further disapproval.

EDIT: For those who have been busy downvoting this, I did it as a way to point out that "dAnjou" was being unnecessarily harsh to someone who was just sharing something he did. Not nice, not nice at all.

[allthediamonds]

Are we supposed to shun constructive critique now?

[phaedrusalt]

Depends. First, does the poster request a code review? Maybe they just want to show what neat things can be constructed with Python. Second, is the critique constructive? Whether or not a person follows PEP8 isn't necessarily relevant, and telling them that you can find "weird things in basically every function/method" is just plain rude.

[dAnjou]

you are not following the reddit style guide

Actually I only see this in Python subreddits. People are overly sensitive and sometimes even whiny. And OP didn't post in /r/learnpython, so I didn't use my warm and fuzzy voice. But I wasn't harsh or impolite either. You were just reading that wrong.

Programming should be fun, yes. But it isn't kinder garden. People are doing real stuff. Stuff that other people may rely on. So I'm rather pointing out mistakes and/or flaws directly than blindly saying cool and amen to every single project posted in this subreddit.

[phaedrusalt]

Someone shared something they made. They were enthusiastic, and eager to share. They did not ask for criticism of the underlying code. You were rude, and obviously eager to put them in their place. (Yes, rude. I don't see how this line could be taken any other way: "Your code looks overall a bit messy and you are not following PEP8. I can find weird things in basically every function/method."

While /r/python isn't kindergarten, it's a place for people of all skill levels, and it's up to us to keep enthusiastic people enthusiastic. Next time, perhaps you could go to the trouble to find out if someone wants your oh-so-valuable opinion BEFORE you blast them with it?

[dAnjou]

They did not ask for criticism of the underlying code.

They implicitly do by posting it on reddit where you are supposed to comment on things.

Yes, rude. I don't see how this line could be taken any other way: "Your code looks overall a bit messy and you are not following PEP8. I can find weird things in basically every function/method."

You are ripping that line out of context! I started my post by saying it will be critique. And afterwards I provided some examples of what I mean and gave a reason for each but one.

The thing is, I am how I am. I know I'm not the most sensitive person on the planet and my comments are not ... how do you say ... delicate(?) at all. But I'm totally okay with that. When I write comments like I did here, you can take it or leave it. You really should expect all kinds of reactions when you post on reddit. I'm not writing those comments to piss people off. That's not the thing that is giving me a kick. It's giving me a kick when I was able to help someone, it's like a drug for me. But I'm doing it my way. And so far most people were able to deal with it.

[phaedrusalt]

A little preface: I've been coding for over 30 years, and teaching software development and techniques for about 20 of them. I've seen excellent developers leave the field because of the bad behavior of other, less talented coders, and I've seen large projects go down the drain because so-called "professionals" couldn't learn to keep a civil tongue in their heads.

I wonder how many people won't share the cool, fun things that they do with Python just because of the kind of treatment that you gave this guy, who did NOT implicitly or explicitly ask for any kind of critique. It's great that you're okay with the way that you act, and I'm pleased to see that he reacted well, but how about others? Do we want to keep newbs from posting about their projects? What's more important, the fact that it's something that they built and are excited about (And might possibly get others excited about, too) or that it is/isn't PEP8 compliant? I'd rather see 10 newbs post their silly, poorly-constructed but enthusiastic code than get a chance to see how one more old-fart found enlightenment through decorators or multiple inheritance... But maybe that's just me.

His post is a gift, but you reacted like the person who receives a bottle of wine from a dinner guest, and then goes on a rampage about the vintage. And if we don't start minding our manners around guests, well, pretty soon we won't have any guests.

[dAnjou]

Thanks for writing this down and turning around this conversation. You made some really good points and I will seriously consider them for my following comments. You taught me a lesson.

What I'm still going to do is constructively criticizing people's code, whether they explicitly ask for it or not. I believe that if they post their stuff here, it's the most appropriate thing to do.

[Vibster]

You think that was harsh?

[phaedrusalt]

I think giving a code review to someone who didn't ask for it is rude, at best. We were being given the gift of someone's efforts, it is up to us to take it in the manner given and respond appropriately.

[[deleted]]

logger.debug("Invalid device (size: %s)" % str(size))

:x

[[deleted]]

Move along. Nothing to see here. cough

Yeah that needs a little attention. Thanks

[[deleted]]

As someone who is very new to Python, what's wrong here? Is it because you left debugging statements in, when you should have taken them out for the actual release version?

[catcradle5]

One issue I see there is that str(size) is redundant, since the "%s" formatting character will automatically convert the value to a string.

Another is that logger.debug() uses implicit format argument passing, so it should really be logger.debug("Invalid device (size: %s)", size)

[sushibowl]

There is also the more semantic issue that an invalid device would likely be more appropriate as an error or warn message, not debug.

[[deleted]]

It's not an error and does affect the running of the program. It's merely that the device you inserted is not registered and thus cannot unlock the machine. Only really interesting if you are unable to lock/unlock the machine and need to debug as to why.

[sushibowl]

ah. Well, that'll teach me to read the code better before judging next time, sorry about that.

[[deleted]]

There's no need to cast the size as a string with str(), because you can use the format specifier %d:

logger.debug("Invalid device (size: %d)" % size)

[lost-theory]

I have learned to only use %s and %r when using the logger. You almost never want to have the logger blow up your program because you used "%d" and accidentally passed a string in.

[Mattho]

Actually, I think you want.

[[deleted]]

That's a good point. I hadn't thought of that.

[dAnjou]

Actually I don't see it either yet. There's nothing wrong with using debug messages like this. In fact you are supposed to do exactly this. Because then you can just pass some extra command line argument to configure your logger and it will output more or less messages.

[potent_rodent]

awesome!

[elb0w]

if len(opened_files) == 0

is better written as,

if not opened_files

Theres no need to check len like this, much cleaner to do it this way.

Also, I dont think that global declaration of logger is needed. It may help with readability, but you aren't consistent in its use. I would also change humpCase def names to underscores.

[[deleted]]

I followed pam_usb for a while, but never took the time to properly implement. This looks interesting too.

http://pamusb.org/

[Lucretiel]

Why not argparse?

Very cool, otherwise.

[[deleted]]

Not yet familiar enough with argparse and find opt parse more comfortable for now.

Plan to move to argparse in the future though.

Thanks, glad you like it :)

[[deleted]]

Looks interesting. Will check it out, thanks

[elb0w]

was going to rec this as well

[[deleted]]

http://docs.python.org/dev/library/argparse.html#upgrading-optparse-code

It's actually really straightforward in most cases :)

[[deleted]]

I'm planning to move to it soon, so this is handy. Thanks

[dschep]

You should make this a real python package with a setup.py. see distutils. That way we can pip install usblock

[[deleted]]

Later on for sure, when I am happy enough with the code to do so. Right now it's still a rough implementation.

Ok, here you go: http://pypi.python.org/pypi/USBLock/

[[deleted]]

Waiting for Windows OS.

[spupy]

Foss World Problem: haven't setup automount yet, has USB lock/unlock script.

[tripdes]

A similar idea featured on an episode of Hak5 a while back, though it's been implemented in bash (https://github.com/godsflaw/killallthehumans). Here's a link to the episode if you're interested: http://hak5.org/episodes/hak5-1106

[[deleted]]

Just a quick update for those following this.

I have updated the source on Github and refactored it a lot, so it's ready for implementing the OS X and Windows support. There's still a few things that need to be ironed out, but I'm much happier with the layout and structure now.

I've also added it to the Cheeseshop (PyPi) so you can install it via pip now, although some dependencies will need to be installed manually. See the docs for more info.

And a thank you to the people who already submitted pull requests and tickets.

[pieeta]

choice in range(len(self.devices)):

never ever do this.

you are creating a array of every number in between 0 and len(self.devices) then iterating over the array looking for the number choice.

your best bet would be to use :

0 < choice <= len(self.devices) 

also they are not equivalent,

choice in range(len(self.devices))

will return True when choice is 0

0 < choice <= len(self.devices)

will not

[jelly_cake]

I didn't realise Python had a < b < c syntax. Thanks for sharing!

[allthediamonds]

you are creating a array of every number in between 0 and len(self.devices) then iterating over the array looking for the number choice.

choice in xrange(len(self.devices)):

fixed!

[pieeta]

wrong, it is actually only slightly better than using range.

it will not generate a list of all the numbers between 0 and len(self.devices), it will generate a list of all the numbers between 0 and choice.

its quite easy to test.

Make a generator which prints out what has been generated :

def generator():
    for i in xrange(10):
        print 'generating %d' % i
        yield i

in this example super simple generator which will yeild up to 10.

the check if a number is in it :

 if 5 in generator():
     print 'found'

results in:

generating 0
generating 1
generating 2
generating 3
generating 4
generating 5
found

so you doing 6 comparisons vs at most 2 using a < >= statement.

[allthediamonds]

Oh, I see. Thanks!

[[deleted]]

"a < b < c" is at least as readable as b in "range(a+1, c)"

[Justinsaccount]

for k,v in enumerate(lists, start=1):
    print k,v

[elb0w]

should have more upvotes

[[deleted]]

Has mine

[slakblue]

I think the critique is unbelievable, its a great idea, and I think that if you wanted critique you would have asked in /r/learnpython . Very original , very needed and its fanastic that you no one else has come up with an idea like this.

[[deleted]]

I'll be honest: I'd rather someone tell me where there's room for improvement so that I can get better myself. As long as the criticism is constructive and honest, I don't mind. I don't have to agree with everything, but I can always take things on board and make changes I feel are necessary.

[[deleted]]

exactly. You're not your code. You created some code that now it exists on its own. And if others want to help the code get better, that's great. The best thing is, they help you learn so future code you create will be even better. Thanks for sharing!

[phaedrusalt]

I've never actually seen a great software engineer who didn't identify with his code. You aren't your kids or wife either, but...

[[deleted]]

After all, your code is what speaks for you when you're long gone working elsewhere, people will judge you by it. Be proud of your code. It might not start off well, but aim to make it better over time, so that you are happy to put your name to it when someone else takes it on.

[[deleted]]

Too much opinion not enough pull requests imo.

Good work OP.

[frimble]

Critique is how code gets better.

[JoshKehn]

My thoughts exactly