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