Sorry, I see I never responded to this email. On 10/02/2014 11:29 AM, Torsten Bögershausen wrote: > On 01.10.14 13:14, Michael Haggerty wrote: > [] > Nice done, small comments inline >> diff --git a/lockfile.c b/lockfile.c >> index d27e61c..e046027 100644 >> --- a/lockfile.c >> +++ b/lockfile.c >> @@ -7,20 +7,29 @@ >> >> static struct lock_file *volatile lock_file_list; >> >> -static void remove_lock_files(void) >> +static void remove_lock_files(int skip_fclose) > Even if the motivation to skip is clear now and here, > I would consider to do it the other way around, > and actively order the fclose(): > > static void remove_lock_files(int call_fclose) I don't think inverting the logic will help the reader remember the motivation for skipping the call to fclose(). I think this way was clearer because skipping the call to fclose() is the "unusual" case; it has to actively sabotage the fclose() that would otherwise take place in rollback_lock_file(). Also, "call_fclose" slightly implies that fclose() will be called for the lockfiles, whereas in fact it will only be called for the lockfiles for which fdopen_lock_file() has been called. >> { >> pid_t me = getpid(); >> >> while (lock_file_list) { >> - if (lock_file_list->owner == me) >> + if (lock_file_list->owner == me) { >> + /* fclose() is not safe to call in a signal handler */ >> + if (skip_fclose) >> + lock_file_list->fp = NULL; >> rollback_lock_file(lock_file_list); >> + } >> lock_file_list = lock_file_list->next; >> } >> } >> >> +static void remove_lock_files_on_exit(void) >> +{ >> + remove_lock_files(0); > What does "0" mean ? > > remove_lock_files(LK_DO_FCLOSE) ? > >> +} >> + >> static void remove_lock_files_on_signal(int signo) >> { >> - remove_lock_files(); >> + remove_lock_files(1); > And what does this "1" mean ? > > remove_lock_files(LK_SKIP_FCLOSE) ? > > We can even have an emum, or use #define Meh. These are private functions, all defined within a few lines of each other. I think that an enum would be overkill here when a "boolean" suffices. Michael -- Michael Haggerty mhagger@xxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html