code review request: basic file I/O

[Adam Monsen]

I kindly request a code review. If this is not an appropriate place
for my request, where might be?

Specific questions are in the QUESTIONS section of the code.

==========================================================================

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#define SECONDS_PER_DAY 86400

/*
wuptime - calculate uptime for cable modem based on columnar data
Copyright (C) 2004 Adam Monsen <[email protected]>

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or
(at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
USA
*/

/*
DESCRIPTION:

This program caculates total uptime and downtime of my cable modem.
Another program gathers input data based on weather it thinks the
modem is up or down. This program is 200 times faster than a Perl
script which does the same thing.

Here are a few lines of sample input (no tabs):
---------------------------8<---------------------------
1073410484 0
1073410540 0
1073410594 1
1073410982 1
1073411950 1
--------------------------->8---------------------------

The first column is a UNIX timestamp, the second column is a flag
indicating weather my cable modem was up (1) or down (0). The first
column must be an integer greater than 1. The second column must be
either 0 or 1. Each successive line must have a greater integer
value in the first column than the preceding line.

This program uses a 2-element queue to calculate uptime. The queue
stores two-dimensional arrays. When the queue is full, the time
delta
is calculated by subtracting the timestamp in the first element from
the timestamp in the second element. Since the timestamp must be
a positive integer, a value of 0 indicates the queue position is
empty.

The in-between time is interpolated as follows:
modem status went from down to down
=> all time in interval counts as downtime
modem status went from up to up
=> all time in interval counts as uptime
modem status went from up to down or down to up
=> all time in interval is split between up and downtime

Here's a Makefile for this program:
---------------------------8<---------------------------
# can't use -ansi with snprintf() since it's C99 or something
CFLAGS=-g -Wall -pedantic -O3
wuptime:
--------------------------->8---------------------------

QUESTIONS:

1. would using structs instead of two-element arrays in the queue be
slower? Should I do it anyway?
2. I use fgets() to read the input file. This will fail if the file
contains NULLs, correct?
3. is there a convenient way to turn seconds into days, hours,
minutes, etc? Perl has Time:uration, for instance.
4. do I need some other header file to use snprintf()? The -ansi
compiler flag causes GCC to complain.
*/

/* populates a queue object */
void
push (int subqueue[], const int timestamp, const int errorcode)
{
subqueue[0] = timestamp;
subqueue[1] = errorcode;
}

/* moves the 2nd position of the queue into the 1st position, and
* marks the 2nd position as empty */
void
shift (int queue[][2])
{
queue[0][0] = queue[1][0];
queue[0][1] = queue[1][1];
queue[1][0] = 0;
/* queue[1][1] = 0; ... not necessary, we only look at [0] */
}

/* modifies total_uptime and total_downtime according to what's in
* the queue */
void
process_uptime (int queue[][2], long *total_uptime, long
*total_downtime)
{
long time_delta = queue[1][0] - queue[0][0];

if (!(queue[0][1] & queue[1][1]))
*total_downtime += time_delta; /* down to down, all down */
else if (queue[0][1] & queue[1][1])
*total_uptime += time_delta; /* up to up, all up */
else if (queue[0][1] ^ queue[1][1])
{
/* down to up or up to down, split the difference */
long half_time_delta = time_delta / 2;
*total_uptime += half_time_delta;
*total_downtime += half_time_delta;
}
}

int
main (void)
{
long total_uptime = 0;
long total_downtime = 0;
FILE *p_datafile = NULL;
char *p = NULL;
const char filename[] = "modem_uptime_data.txt";
char line[BUFSIZ];
/*
queue = { {1076924581, 1 }, {1076926382, 0 } };
| ^^^^^^^^^^ ^^^^^^ ^^^^^^^^^^ ^^^^^^
columns = timestamp status , timestamp status
*/
int queue[2][2] = { {0, 0}, {0, 0} };

printf ("Trying to open %s...\n", filename);
if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[BUFSIZ];
snprintf (errstr, BUFSIZ, "Error opening %s", filename);
perror (errstr);
exit (1);
}

while ((fgets (line, sizeof line, p_datafile)) != NULL)
{
int timestamp, errorcode;
if ((p = strchr (line, '\n')) != NULL)
*p = '\0';
sscanf (line, "%d\t%d", &timestamp, &errorcode);
push (queue[1], timestamp, errorcode);
if (queue[0][0] && queue[1][0])
process_uptime (queue, &total_uptime, &total_downtime);
shift (queue);
}

printf ("Uptime: %ld days\n", total_uptime / SECONDS_PER_DAY);
printf ("Downtime: %ld days\n", total_downtime / SECONDS_PER_DAY);

fclose (p_datafile);
return EXIT_SUCCESS;
}

 

[Barry Schwarz]

On 16 Aug 2004 15:47:25 -0700, (e-mail address removed) (Adam Monsen) wrote:

snip

/* modifies total_uptime and total_downtime according to what's in
* the queue */
void
process_uptime (int queue[][2], long *total_uptime, long
*total_downtime)
{
long time_delta = queue[1][0] - queue[0][0];

if (!(queue[0][1] & queue[1][1]))
*total_downtime += time_delta; /* down to down, all down */
else if (queue[0][1] & queue[1][1])
*total_uptime += time_delta; /* up to up, all up */

This else is the exact opposite of the previous if. Therefore, one of
them must evaluate to true. Therefore, the following else will never
be evaluated.

else if (queue[0][1] ^ queue[1][1])
{
/* down to up or up to down, split the difference */
long half_time_delta = time_delta / 2;
*total_uptime += half_time_delta;
*total_downtime += half_time_delta;
}
}

int
main (void)
{
long total_uptime = 0;
long total_downtime = 0;
FILE *p_datafile = NULL;
char *p = NULL;
const char filename[] = "modem_uptime_data.txt";
char line[BUFSIZ];
/*
queue = { {1076924581, 1 }, {1076926382, 0 } };
| ^^^^^^^^^^ ^^^^^^ ^^^^^^^^^^ ^^^^^^
columns = timestamp status , timestamp status
*/
int queue[2][2] = { {0, 0}, {0, 0} };

printf ("Trying to open %s...\n", filename);
if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[BUFSIZ];
snprintf (errstr, BUFSIZ, "Error opening %s", filename);
perror (errstr);
exit (1);

Use EXIT_FAILURE for portability.

}

while ((fgets (line, sizeof line, p_datafile)) != NULL)
{
int timestamp, errorcode;
if ((p = strchr (line, '\n')) != NULL)
*p = '\0';
sscanf (line, "%d\t%d", &timestamp, &errorcode);
push (queue[1], timestamp, errorcode);
if (queue[0][0] && queue[1][0])
process_uptime (queue, &total_uptime, &total_downtime);
shift (queue);
}

printf ("Uptime: %ld days\n", total_uptime / SECONDS_PER_DAY);
printf ("Downtime: %ld days\n", total_downtime / SECONDS_PER_DAY);

fclose (p_datafile);
return EXIT_SUCCESS;
}

<<Remove the del for email>>

 

[Michael Mair]

Hi,

just two things I know without looking it up or writing a long answer:

1. would using structs instead of two-element arrays in the queue be
slower? Should I do it anyway?

You use gcc and do not do any bad things involving pointers, so: no.
Even if not: It might be nicer to read o.timestamp, o.status, so: yes.

4. do I need some other header file to use snprintf()? The -ansi
compiler flag causes GCC to complain.

-ansi is equivalent to -std=c89.
That is _not_ what you want as snprintf is a "new" function.
Try -std=c99 instead, then you will find a prototype/declaration of
snprintf() in <stdio.h>.

Cheers
Michael

 

[Adam Monsen]

if (!(queue[0][1] & queue[1][1]))
*total_downtime += time_delta; /* down to down, all down */
else if (queue[0][1] & queue[1][1])
*total_uptime += time_delta; /* up to up, all up */

This else is the exact opposite of the previous if. Therefore, one of
them must evaluate to true. Therefore, the following else will never
be evaluated.

[...]

Ah yes, that was supposed to be a bitwise OR, as in !(queue[0][1] | queue[1][1]).
Thanks, Barry!

 

[Adam Monsen]

if (!(queue[0][1] & queue[1][1]))
*total_downtime += time_delta; /* down to down, all down */
else if (queue[0][1] & queue[1][1])
*total_uptime += time_delta; /* up to up, all up */

This else is the exact opposite of the previous if. Therefore, one of
them must evaluate to true. Therefore, the following else will never
be evaluated.

[...]

Ah yes, that was supposed to be a bitwise OR, as in !(queue[0][1] | queue[1][1]).
Thanks, Barry!

 

[Arthur J. O'Dwyer]

I kindly request a code review. If this is not an appropriate place
for my request, where might be?

Here is fine, for C programming questions. General algorithm
reviews ought to go to comp.programming or one of the specialized
groups (comp.graphics or sci.crypt, e.g.). Your questions belong
here.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#define SECONDS_PER_DAY 86400

/*
wuptime - calculate uptime for cable modem based on columnar data
Copyright (C) 2004 Adam Monsen <[email protected]>

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or
(at your option) any later version.

[Notice that this means you won't get help from a lot of people.
What's the point of spending time helping a guy make a piece of
software if I'm not going to be allowed to use it the way I see
fit? See below.]

/*
DESCRIPTION:

This program caculates total uptime and downtime of my cable modem.
Another program gathers input data based on weather it thinks the
modem is up or down. This program is 200 times faster than a Perl
script which does the same thing.

s/caculates/calculates/, s/weather/whether/, and I assume you meant
to imply you're a poor Perl programmer. Nice documentation, BTW;
docs are good. Just make sure you don't misspell every other word.

Here are a few lines of sample input (no tabs):

Okay, bad documentation. What does "(no tabs):" mean? I mean,
obviously there are no tabs in the sample input; we can see that.
So does it mean we /do/ need to insert tabs in the real input?
If so, where, and how many? And why? (As we discover from
reading the code, no tabs are required anywhere. This is just a
kind of "documentation bug.")

---------------------------8<---------------------------
1073410484 0
1073410540 0
1073410594 1
1073410982 1
1073411950 1
--------------------------->8---------------------------

The first column is a UNIX timestamp, the second column is a flag
indicating weather my cable modem was up (1) or down (0). The first s/weather/whether/
column must be an integer greater than 1. The second column must be
either 0 or 1. Each successive line must have a greater integer
value in the first column than the preceding line.

(It would be nice if your program enforced these requirements, and
complained if they weren't met.)

This program uses a 2-element queue to calculate uptime. The queue
stores two-dimensional arrays. When the queue is full, the time
delta
is calculated by subtracting the timestamp in the first element from
the timestamp in the second element. Since the timestamp must be
a positive integer, a value of 0 indicates the queue position is
empty.

The in-between time is interpolated as follows:
modem status went from down to down
=> all time in interval counts as downtime
modem status went from up to up
=> all time in interval counts as uptime
modem status went from up to down or down to up
=> all time in interval is split between up and downtime

Here's a Makefile for this program:
---------------------------8<---------------------------
# can't use -ansi with snprintf() since it's C99 or something
CFLAGS=-g -Wall -pedantic -O3

Right. It's C99. The C99 equivalent of '-ansi' is '-std=c99'.

wuptime:
--------------------------->8---------------------------

QUESTIONS:

1. would using structs instead of two-element arrays in the queue be
slower? Should I do it anyway?

Probably not, and probably not. In fact, you don't need the queue
at all. Four local variables will take care of everything, and you
can do away with the "queue" subroutines. I would show you my version
of your program (it's much simpler), but I don't think I'm allowed to
distribute it here unless I saddle it with the GPL. (See?)

2. I use fgets() to read the input file. This will fail if the file
contains NULLs, correct?

Null characters, yes. 'NULL' is a standard macro which evaluates to
a null /pointer/ constant. Files can't contain null pointer constants!
(Write "null character", "zero byte", or "'\0'" when you mean what you
meant above.)

3. is there a convenient way to turn seconds into days, hours,
minutes, etc? Perl has Time:uration, for instance.

Investigate the 'ctime' family of functions. I don't know, off
the top of my head, but I guess it can be done by e.g. setting the
seconds field of a "struct tm" structure and calling some routine
that as a side effect does normalization of the structure.

/* modifies total_uptime and total_downtime according to what's in
* the queue */
void
process_uptime (int queue[][2], long *total_uptime, long
*total_downtime)
{
long time_delta = queue[1][0] - queue[0][0];

if (!(queue[0][1] & queue[1][1]))

As previously noted, this should be '(!queue[0][1] & !queue[1][1])'.

*total_downtime += time_delta; /* down to down, all down */
else if (queue[0][1] & queue[1][1])
*total_uptime += time_delta; /* up to up, all up */
else if (queue[0][1] ^ queue[1][1])
{
/* down to up or up to down, split the difference */
long half_time_delta = time_delta / 2;
*total_uptime += half_time_delta;
*total_downtime += half_time_delta;

If 'time_delta' is odd, you're dropping seconds here. This is
almost certainly a bug, although it doesn't show up with your
sample data because every timestamp is even.

}
}

int
main (void)
{
long total_uptime = 0;
long total_downtime = 0;
FILE *p_datafile = NULL;
char *p = NULL;
const char filename[] = "modem_uptime_data.txt";
char line[BUFSIZ];
/*
queue = { {1076924581, 1 }, {1076926382, 0 } };
| ^^^^^^^^^^ ^^^^^^ ^^^^^^^^^^ ^^^^^^
columns = timestamp status , timestamp status
*/
int queue[2][2] = { {0, 0}, {0, 0} };

printf ("Trying to open %s...\n", filename);
if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[BUFSIZ];
snprintf (errstr, BUFSIZ, "Error opening %s", filename);

What on earth makes you think 'BUFSIZ' is appropriate here?
What you meant was, "I need a buffer big enough to hold 'filename'
plus some text." So write what you mean!

char errstr[sizeof "Error opening "+strlen(filename)];

since you're using C99 anyway; or just

char errstr[1000]; /* this ought to be big enough */

But 'BUFSIZ' is going to be either monstrously large, or else
much too small, depending on the whim of Murphy.

perror (errstr);
exit (1);

exit(EXIT_FAILURE); as previously noted

}

while ((fgets (line, sizeof line, p_datafile)) != NULL)
{
int timestamp, errorcode;
if ((p = strchr (line, '\n')) != NULL)
*p = '\0';

Unnecessary baggage.

sscanf (line, "%d\t%d", &timestamp, &errorcode);

Here is the "documentation bug." The line above does not
do what you think it does. The 'scanf' format string "\t" matches
any whitespace at all (just the same as "\n" and " "). So replace
the string with
"%d%d"
or
"%d %d"
as desired; both do the same thing, and I prefer the former because
it's shorter and simpler, but the latter might be considered more
readable.

push (queue[1], timestamp, errorcode);
if (queue[0][0] && queue[1][0])
process_uptime (queue, &total_uptime, &total_downtime);
shift (queue);
}

printf ("Uptime: %ld days\n", total_uptime / SECONDS_PER_DAY);
printf ("Downtime: %ld days\n", total_downtime / SECONDS_PER_DAY);

fclose (p_datafile);
return EXIT_SUCCESS;
}

 

[Adam Monsen]

Aurthur, thank you for your comments! Your help is invaluable.

[Notice that this means you won't get help from a lot of people.
What's the point of spending time helping a guy make a piece of
software if I'm not going to be allowed to use it the way I see
fit? See below.]

Hmm, well, here's a couple reasons a fellow might want to help a guy
make a piece of software, even if their use of that code was
restricted:
* Academic interest.
* Charity.
* For the sake of knowledge: learning/teaching.
* In case you end up maintaining some of my code some day, it won't
suck too bad.

I would be more than happy to buy you a beer for the help you've
provided. Let me add that to the list:

* free beer

s/caculates/calculates/, s/weather/whether/,

Ah yes. Thanks.

and I assume you meant to imply you're a poor Perl programmer.

Oops, that should have read, '20 times faster'. I think I'm a decent
Perl programmer, I meant to imply that doing the same thing in Perl is
slower.

Nice documentation, BTW;

Thank you for the corrections and compliments!

docs are good. Just make sure you don't misspell every other word.


Okay, bad documentation. What does "(no tabs):" mean? I mean,
obviously there are no tabs in the sample input; we can see that.

I meant "the following text contains no tabs", even better: "the
following text *must not* contain tabs". This would be another thing I
should check when adding input validation code.

So does it mean we /do/ need to insert tabs in the real input?
If so, where, and how many? And why? (As we discover from
reading the code, no tabs are required anywhere. This is just a
kind of "documentation bug.")

Good point! Noted. I definitely need practice writing documentation,
so your comments here are much appreciated.

(It would be nice if your program enforced these requirements, and
complained if they weren't met.)

I wholeheartedly agree. Good call.

[...]

Probably not, and probably not. In fact, you don't need the queue
at all. Four local variables will take care of everything, and you
can do away with the "queue" subroutines. I would show you my version
of your program (it's much simpler), but I don't think I'm allowed to
distribute it here unless I saddle it with the GPL. (See?)

Noted, and yes, I would've liked to have read your code.

However, this doesn't change my opinion that the GPL can be a useful
software license, and I stand by my decision to cover this code with
the GPL.

Null characters, yes. 'NULL' is a standard macro which evaluates to
a null /pointer/ constant. Files can't contain null pointer constants!
(Write "null character", "zero byte", or "'\0'" when you mean what you
meant above.)

Done. Thanks!

Investigate the 'ctime' family of functions. I don't know, off
the top of my head, but I guess it can be done by e.g. setting the
seconds field of a "struct tm" structure and calling some routine
that as a side effect does normalization of the structure.

My manpage for ctime(3) says the seconds field should only be an
integer in the range between 0 and 61.

[...]

*total_downtime += time_delta; /* down to down, all down */
else if (queue[0][1] & queue[1][1])
*total_uptime += time_delta; /* up to up, all up */
else if (queue[0][1] ^ queue[1][1])
{
/* down to up or up to down, split the difference */
long half_time_delta = time_delta / 2;
*total_uptime += half_time_delta;
*total_downtime += half_time_delta;

If 'time_delta' is odd, you're dropping seconds here. This is
almost certainly a bug, although it doesn't show up with your
sample data because every timestamp is even.

Right, good catch. I just couldn't decide who should get the
leftovers.

[...]

char errstr[BUFSIZ];
snprintf (errstr, BUFSIZ, "Error opening %s", filename);

What on earth makes you think 'BUFSIZ' is appropriate here?
What you meant was, "I need a buffer big enough to hold 'filename'
plus some text." So write what you mean!

char errstr[sizeof "Error opening "+strlen(filename)];

since you're using C99 anyway; or just

char errstr[1000]; /* this ought to be big enough */

But 'BUFSIZ' is going to be either monstrously large, or else
much too small, depending on the whim of Murphy.

Noted. Thank you!

I guess I could also reduce the size of 'line', since I could place a
restriction of 25 characters or so on line length.

[...]

Unnecessary baggage.

True, no need to chomp those newlines. Thanks!

Here is the "documentation bug." The line above does not
do what you think it does. The 'scanf' format string "\t" matches
any whitespace at all (just the same as "\n" and " "). So replace
the string with
"%d%d"
or
"%d %d"
as desired; both do the same thing, and I prefer the former because
it's shorter and simpler, but the latter might be considered more
readable.

Ok, great. Why don't I need a newline, ala "%d %d\n"?

[...]

Thank you,
-Adam

 

[Adam Monsen]

if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[BUFSIZ];
snprintf (errstr, BUFSIZ, "Error opening %s", filename);

What on earth makes you think 'BUFSIZ' is appropriate here?
What you meant was, "I need a buffer big enough to hold 'filename'
plus some text." So write what you mean!

char errstr[sizeof "Error opening "+strlen(filename)];

since you're using C99 anyway; or just

char errstr[1000]; /* this ought to be big enough */

But 'BUFSIZ' is going to be either monstrously large, or else
much too small, depending on the whim of Murphy.

Going along with your suggestion, how 'bout the following?

#define ERRSTR_MAXLEN 1000
if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[ERRSTR_MAXLEN] = "Error opening ";
strncat(errstr, filename, ERRSTR_MAXLEN - strlen(errstr) - 1);
perror (errstr);
exit (EXIT_FAILURE);
}

Now it's good ol' ANSI, too.

[...]

 

[Arthur J. O'Dwyer]

[Notice that this means you won't get help from a lot of people.
What's the point of spending time helping a guy make a piece of
software if I'm not going to be allowed to use it the way I see
fit? See below.]

Hmm, well, here's a couple reasons a fellow might want to help a guy
make a piece of software, even if their use of that code was
restricted:
* Academic interest.
* Charity.
* For the sake of knowledge: learning/teaching.
* In case you end up maintaining some of my code some day, it won't
suck too bad.

But in general, I could just ignore /your/ code, write my /own/
code, release it as public domain, and get all the above benefits
plus the benefit of free redistributability. (This approach
breaks down for large projects where the work of contributing is
less than the work of rewriting-from-scratch. But as I said, I
refactored your program in a very short time, and could certainly
write it from scratch in not much longer, if I did have aforementioned
academic interest or charity.
Anyhoo, I'm sure we've all had this discussion many times before,
so I'm dropping it now. I hope.

[...] This program is 200 times faster than a Perl
script which does the same thing.

and I assume you meant to imply you're a poor Perl programmer.

Oops, that should have read, '20 times faster'. I think I'm a decent
Perl programmer, I meant to imply that doing the same thing in Perl is
slower.

And I meant to imply that I wouldn't expect it to be that much slower.
Maybe a little bit, but any difference more than a factor of magnitude
would surprise me. I'm too lazy to generate the test data and find out,
though.

My manpage for ctime(3) says the seconds field should only be an
integer in the range between 0 and 61.

Yes, but in the same family of functions we have 'mktime', which
does normalize its argument. A minimalistic example:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main(int argc, char **argv)
{
int secs = atoi(argv[1]);
struct tm Time = {0};
Time.tm_sec = secs;
Time.tm_mday = 1;
Time.tm_year = 70;
mktime(&Time);
puts(asctime(&Time));
return 0;
}

We have to set 'Time.tm_year = 70' because if we call 'mktime' with the
year out of the range of 'time_t', it won't normalize the other values
for us.
This is still a more convoluted approach than simply dividing by
60, 60, and 24 yourself, and I think you need extra tricks to deal
with Daylight Savings Time in the United States.

Ok, great. Why don't I need a newline, ala "%d %d\n"?

Because 'sscanf' will just ignore any part of the string it didn't
match, just like regular 'scanf' will leave unmatched characters in
the input buffer. Writing "%d %d\n" won't do anything to improve
this code, since you don't check to see whether the 'sscanf' call
succeeded or not anyway. If you want fine control over your
input parsing, you should be using 'strtol' or 'strtoul' anyway.

HTH,
-Arthur

 

[Arthur J. O'Dwyer]

if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[BUFSIZ];
snprintf (errstr, BUFSIZ, "Error opening %s", filename);

What on earth makes you think 'BUFSIZ' is appropriate here?
What you meant was, "I need a buffer big enough to hold 'filename'
plus some text." So write what you mean!

[...]
Going along with your suggestion, how 'bout the following?

#define ERRSTR_MAXLEN 1000
if ((p_datafile = fopen (filename, "r")) == NULL)
{
char errstr[ERRSTR_MAXLEN] = "Error opening ";
strncat(errstr, filename, ERRSTR_MAXLEN - strlen(errstr) - 1);
perror (errstr);
exit (EXIT_FAILURE);
}

Now it's good ol' ANSI, too.

Yes, that works. My personal "toolbox" approach is to begin every
non-trivial program I write with three functions: 'main', 'do_error',
and 'do_help'. The second of those is the relevant one here. So I'd
write

Argv0 = argv[0];
[...]
fp = fopen(InputFilename, "r");
if (fp == NULL)
do_error("Couldn't open file '%s' for reading!", InputFilename);

where 'do_error' had previously been defined as

void do_error(const char *fmat, ...)
{
va_list ap;
printf("%s: ", Argv0);
va_start(ap, fmat);
vprintf(fmat, ap);
printf("\n");
va_end(ap);
exit(EXIT_FAILURE);
}

(except that sometimes, just for variety, I might send the error message
to 'stderr' instead of 'stdout', and sometimes it's not worth setting
up 'Argv0').

HTH,
-Arthur