Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

A simple command that wraps another command, locking a file first. It is similar to flock, just simpler.

#include <stddef.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/file.h>
#include <err.h>
#include <stdlib.h>

void attemptLock(const char *lockFileName)
{
    int fd = open(lockFileName, O_CREAT | O_WRONLY, 0666);
    if (!fd || flock(fd, LOCK_EX))
        warn("Cannot lock %s", lockFileName);
}

int main(int argc, char *argv[])
{
    if (argc < 3)
        errx(EXIT_FAILURE, "Not enough arguments.\nUsage: %s LOCKFILENAME COMMAND...\n", argv[0]);
    attemptLock(argv[1]);
    argv += 2;  // Skip our own command name and LOCKFILENAME.
    execvp(argv[0], argv);
    err(EXIT_FAILURE, "Cannot execute %s", argv[0]);
}

Anything fishy? Bogus? Wrong? Anything that can be improved (besides adding options like -h)?

share|improve this question

1 Answer 1

up vote 2 down vote accepted

Besides reinventing the wheel the things I'm concerned about are:

  • the warn in attemptLock, because if either creating the lock file fails, or if you can't get the lock (or the call was interrupted), I would expect the wrapper to exit instead,
  • the return value of open, which might very well be negative, so just check for that separately and give an error message for that,
  • the return value of flock, where you should handle EINTR in a loop I believe?
share|improve this answer
    
Once again shows how good it is to have code reviewed. The warn instead of err was intentional because of support for a specific version of Cygwin which has issues with successfully locking, but there should at least have been a comment for explaining the unexpected warn instead of err. And the other two issues that you've found are real bugs. So, thanks a lot! –  Christian Hujer Feb 12 at 5:46

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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