Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 18, 2014, 07:47 UTC
Message-ID: <541A8E06.8080609@alum.mit.edu>
In-Reply-To: <541A6047.6000707@web.de>

On 09/18/2014 06:32 AM, Torsten Bögershausen wrote:

Show 28 quoted lines

> On 09/16/2014 09:33 PM, Michael Haggerty wrote:
> []
>>
>> diff --git a/lockfile.c b/lockfile.c
>> index 983c3ec..00c972c 100644
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -129,6 +129,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>>  	 */
>>  	static const size_t max_path_len = sizeof(lk->filename) - 5;
>>  
>> +	if (!lock_file_list) {
>> +		/* One-time initialization */
>> +		sigchain_push_common(remove_lock_file_on_signal);
>> +		atexit(remove_lock_file);
>> +	}
>> +
>> +	if (!lk->on_list) {
>> +		/* Initialize *lk and add it to lock_file_list: */
>> +		lk->fd = -1;
>> +		lk->owner = 0;
>> +		lk->on_list = 1;
>> +		lk->filename[0] = 0;
> Does it makes sense to change the order here:
> 
> Do the full initialization, and once that is completed, set on_list = 1
> +		lk->filename[0] = 0;
> +		lk->on_list = 1;

From a functional standpoint, it doesn't matter. This function is the only place that uses on_list, and it is basically only used to make sure that each lock_file structure is initialized and registered in lock_file_list exactly once. In particular, the signal handling code doesn't care about the on_list field.

So the only important timing requirement WRT on_list is that it be set before this function is called again with the same lock_file object. But any code that would call this function twice, simultaneously, with the same lock_file argument would be broken far more seriously than could be fixed by changing the order that the fields are initialized.

But I guess you are right that it looks more natural to set this field only after all of the initialization is done. I will make the change.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu