[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