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

[Simo Sorce]

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 */ }
	
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.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York