[Freeipa-devel] [PATCH] detect failure to write ipa_kpasswd.pid file

[Jim Meyering]

Simo Sorce <ssorce at redhat.com> wrote:
> On Wed, 2008-05-14 at 21:37 +0200, Jim Meyering wrote:
>> Hi,
>>
>> I was looking through freeIPA's C code and found a few ways to improve it.
>>
>> >From fac9600e3eb1204fd7a2d0d2c6f0b7be17a3dc02 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering at redhat.com>
>> Date: Sun, 4 May 2008 15:17:36 +0200
>> Subject: [PATCH] detect failure to write ipa_kpasswd.pid file
>>
>> * ipa_kpasswd.c (main): Detect not just open failure,
>> but also any write failure.
>> ---
>>  ipa-server/ipa-kpasswd/ipa_kpasswd.c |   20 ++++++++++++++------
>>  1 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/ipa-server/ipa-kpasswd/ipa_kpasswd.c b/ipa-server/ipa-kpasswd/ipa_kpasswd.c
>> index 5782367..86aa6c1 100644
>> --- a/ipa-server/ipa-kpasswd/ipa_kpasswd.c
>> +++ b/ipa-server/ipa-kpasswd/ipa_kpasswd.c
>> @@ -3,7 +3,7 @@
>>
>>  /* Authors: Simo Sorce <ssorce at redhat.com>
>>   *
>> - * Copyright (C) 2007  Red Hat
>> + * Copyright (C) 2007, 2008  Red Hat
>>   * see file 'COPYING' for use and warranty information
>>   *
>>   * This program is free software; you can redistribute it and/or
>> @@ -1188,13 +1188,21 @@ int main(int argc, char *argv[])
>>  	}
>>
>>  	/* Write out the pid file after the sigterm handler */
>> -	FILE *f = fopen("/var/run/ipa_kpasswd.pid", "w");
>> +	const char *pid_file = "/var/run/ipa_kpasswd.pid";
>> +	FILE *f = fopen(pid_file, "w");
>> +	int fail = 0;
>>  	if (f == NULL) {
>> -		syslog(LOG_ERR,"Couldn't create pid file /var/run/ipa_kpasswd.pid: %s", strerror(errno));
>> -		exit(1);
>> +		fail = 1;
>>  	} else {
>> -		fprintf(f, "%ld\n", (long) getpid());
>> -		fclose(f);
>> +		if (fprintf(f, "%ld\n", (long) getpid()) <= 0)
>> +			fail = 1;
>> +		if (fclose(f) != 0)
>> +			fail = 1;
>> +	}
>> +	if (fail) {
>> +		syslog(LOG_ERR,"Couldn't create pid file %s: %s",
>> +		       pid_file, strerror(errno));
>> +		exit(1);
>>  	}
>>
>>  	tai = ai;
>> --
>> 1.5.5.1.216.g33c73
>
> The code might look better if you do if(f) {} and completely remove the
> 'else' statement.

Maybe I'm being dense, but I don't see it.
Can you be more precise?