Re: [C] Working with disk files. *long post warning*

From: Richard Heathfield (dontmail_at_address.co.uk.invalid)
Date: 03/03/04 

Date: Wed, 3 Mar 2004 18:31:44 +0000 (UTC)

Buck Rogers wrote:

<snip>

> So I just replaced "choice" with "(choice - 1)" in the fseek() function in
> OpenAccount(), and it works fine, as expected.

That was what I discovered, too, this morning. I had a few other comments to
make about your code, and was going to post a *long* reply. Fortunately,
your discovery has made that unnecessary. I'll try to be much briefer than
planned:

In fgets calls, if your array is in scope you can do this:

  fgets(buf, sizeof buf, fp);

(Use stdin instead of fp if need be, of course.)

This makes your fgets call independent of your MAX #define. That is, if you
later change the buffer to be, say, A_BIT_MORE_THAN_MAX :-) then you don't
have to go seeking out all your fgets calls to bring them up to date.

If your printf doesn't end in a \n, add:

fflush(stdout);

before blocking for input (e.g. scanf or fgets), to ensure that the prompt
appears at the right time. (On your system, this obviously isn't a problem,
or you'd have been puzzled by it - but not all systems are like yours.)

One of your exit() calls uses 1 instead of EXIT_FAILURE.

Talking of which, EXIT_SUCCESS and EXIT_FAILURE are defined for you in
<stdlib.h>, so there is no need to define them yourself.

When I first ran the program, it correctly displayed the account
information. So I then ended the program, to see if the data had been saved
correctly.

To do this, I used a hex-dumping program. Here is the output I got:

43 75 72 72 65 6E 74 0A 00 00 00 00 1C F5 FF BF | Current.........
01 00 00 00 48 AA 04 08 7C F5 FF BF E4 DD 0F 40 | ....H...|......@
38 60 01 40 6C F5 FF BF 74 8F 04 08 1C F5 FF BF | 8`.@l...t.......
50 00 57 61 6C 74 65 72 20 4D 61 64 68 6F 75 73 | P.Walter Madhous
65 0A 00 00 0A 00 00 00 22 F3 0F 40 98 9B 1B 40 | e......."..@...@
8C 5F 01 40 34 F6 FF BF 22 F3 0F 40 98 9B 1B 40 | ._.@4..."..@...@
8C 5F 01 40 30 0A 00 BF B2 48 0F 40 20 6D 1B 40 | ._.@0....H.@ m.@
8B 96 04 08 78 F5 FF BF 00 00 00 00 98 9B 1B 40 | ....x..........@
1E 00 00 00 9C F5 FF BF 41 8C 04 08 27 97 04 08 | ........A...'...
90 16 17 40 9C F5 FF BF D2 04 00 00 8B 96 04 08 | ...@............
00 00 C8 42 | ...B

This output worried me a little, since it demonstrated that
you were putting up with a lot of junk that might cause
problems later. It's quite easy to get rid of that junk.

I fixed the line:

  struct record account;

to read:

  struct record account = {0};

I then deleted budget.txt and re-ran the program, using the same
inputs as before. Here is the new hex dump.

43 75 72 72 65 6E 74 0A 00 00 00 00 00 00 00 00 | Current.........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | ................
00 00 57 61 6C 74 65 72 20 4D 61 64 68 6F 75 73 | ..Walter Madhous
65 0A 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | e...............
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | ................
00 00 00 00 30 0A 00 00 00 00 00 00 00 00 00 00 | ....0...........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 | ................
00 00 00 00 00 00 00 00 D2 04 00 00 00 00 00 00 | ................
00 00 C8 42 | ...B

Isn't that much easier to understand?

By the way, we can see from this dump that you didn't remove the newline
characters from your string inputs. :-) Here's a routine to fix that:

#include <string.h>

int chomp(char *s)
{
  char *p = strchr(s, '\n');
  if(p != NULL)
  {
    *p = '\0';
  }
  return p != NULL; /* returns 1 if a newline was chomped, else 0 */
}

Let's try to explain the output of the hex dump. The first three
rows (and first two characters of the next row) are clearly the
"type" field (50 characters). There appears to be no padding to
four-byte multiples by default on my system, because we go
straight into the next field, "name". That takes us up to and
including the fourth byte on the seventh row. Then we have another
50 characters for "institution". Now it gets curious. There are
just three fields to go, but 14 bytes unaccounted for. The last
four are 00 00 C8 42, which is presumably the "amount" field.
The four before that are 00 00 00 00, which must be the "ctr"
field, and the four before that are D2 04 00 00, which is
little-endian for 1234. That leaves two bytes unaccounted for.
It seems that what gcc has done is to store the arrays of char
contiguously, but then add two bytes of padding before the
ints. I suppose I was expecting either /every/ field to be
aligned to a four-byte offset, or none of them to be aligned
in that way. Well, we live and learn, don't we?

You never know when a call will fail. Had you checked the
fread call in OpenAccount(), you'd have realised that it
wasn't reading any information into the struct.

I put the following code after the fread:

     if(ferror(fp))
     {
       fprintf(stderr, "Disk read error.\n");
       exit(EXIT_FAILURE);
     }
     if(feof(fp))
     {
       fprintf(stderr, "Attempt made to read past end"
                       " of file. Wrong account number?\n");
     }

and rebuilt the program. When I tried to open the existing
account for viewing, I got this message:

Attempt made to read past end of file. Wrong account number?

That was how I found your bug.

Here are a few hints for you, which will help you to improve your program
still further:

1) remove the mutual recursion. Let GotoAccount() return
to MainMenu() rather than calling it again.
2) Put a loop into MainMenu(), so that it keeps going
round and round until (3) Exit Program is selected, rather
than dropping out straight away.
3) Whenever You Attempt To Acquire A Resource, Check
That It Worked!

That third hint means this:

Whenever you allocate memory, check that the allocation succeeded.
Whenever you open a file, check that the file opened okay.
Whenever you request input from a stream, check that you got
what you expected to get.
Whenever you write output to a stream, check that it was written
correctly.
Whenever you open a socket...
Whenever you request the printer to print a document...
Whenever you do ANYTHING that might go wrong, make sure it didn't.
If it did, do something intelligent about it.

Best of luck with your program. 

-- 
Richard Heathfield : binary@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton

Relevant Pages

  • Re: Help with account auditing win2k
    ... c:\winnt with the apparently random, ... The other could be solved by allowing the account Full Control ... if that is hard-coded into what Fusion is expecting you ... > ASPNET user account. ...
    (microsoft.public.windows.server.security)
  • To count ones Z80.LIB files...
    ... It was containing 2 MACLIB pseudo-ops (some other assemblers use ... everything in characters, not huge giant, inefficient PDF ... I was expecting to find one Z80.LIB file, and I ... (Control characters can produce very hard to find subtle errors ...
    (comp.os.cpm)
  • Re: Review - Torchwood: Fragments
    ... lot of what we're shown of the characters doesn't fit well with what ... going into any sci-fi show expecting something genuinely surprising or ... change than most series of this sort would experiment with. ...
    (rec.arts.drwho)
  • Re: Sin City BBC2
    ... Yeah, it was better than I was expecting as well, but the way ... the characters could be shot a number of times and still be ...
    (uk.media.tv.misc)
  • db access
    ... I have connected to our group's sql 2000 server with NT ... authentication but was not expecting it to work. ... account name in both domains (e.g. ...
    (microsoft.public.sqlserver.security)