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

Jim Meyering jim at meyering.net
Wed May 14 23:10:50 UTC 2008


Simo Sorce <ssorce at redhat.com> wrote:

> On Thu, 2008-05-15 at 00:25 +0200, Jim Meyering wrote:
>> 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?
>
> The flow is more readable this way:
>
> int fail = 1;
>
> if (f) {
>    /* do stuff */
>    if (all_ok) fail = 0;
> }
>
> if (fail) { /* log and exit */ }

Making it more readable while retaining correctness is tricky.
The catch lies in always closing F, even when fprintf fails.
The following is shorter and no less correct -- maybe even more
readable.  At least it doesn't set fail=1 three times:
[note that technically, we should save errno from
 a failed fprintf, so it can't be clobbered by fclose,
 but in practice it probably doesn't matter, since that
 use of fprintf won't ever fail. ]

	const char *pid_file = "/var/run/ipa_kpasswd.pid";
	int fail = 1;
	FILE *f = fopen(pid_file, "w");
	if (f) {
		int n_bytes = fprintf(f, "%ld\n", (long) getpid());
		if (fclose(f) == 0 && 0 < n_bytes)
			fail = 0;
	}
	if (fail) {
		syslog(LOG_ERR,"Couldn't create pid file %s: %s",
		       pid_file, strerror(errno));
		exit(1);
	}

> Also I personally prefer not to execute functions on variables
> declaration.
> Like: FILE *f = fopen(pid_file, "w");
> (yes I know the original code did it as well :)
>
> It is clearer if functions are explicitly run after all variables have
> been declared IMO.

I have the opposite preference.
I prefer to avoid the added line (more code fits on a screen/page),
and to avoid the duplicate use of the variable name (one fewer detail
that can get out of sync).




More information about the Freeipa-devel mailing list