Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Oct 12, 2014, 06:17 UTC
Message-ID: <543A1CEF.7020700@alum.mit.edu>
In-Reply-To: <542D1AF2.8050508@web.de>

Sorry, I see I never responded to this email.

On 10/02/2014 11:29 AM, Torsten Bögershausen wrote:

Show 18 quoted lines

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

Show 33 quoted lines

>>  {
>>  	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@alum.mit.edu