Path: utzoo!attcan!uunet!lll-winken!ames!nrl-cmf!mailrus!uflorida!novavax!twwells!bill
From: bill@twwells.uucp (T. William Wells)
Newsgroups: comp.sources.d
Subject: Re: Comments on INSERT.c
Message-ID: <313@twwells.uucp>
Date: 10 Jan 89 07:49:13 GMT
References: <308@twwells.uucp> <Jan.8.18.48.59.1989.10861@athos.rutgers.edu>
Reply-To: bill@twwells.UUCP (T. William Wells)
Organization: None, Ft. Lauderdale
Lines: 129
Summary:
Expires:
Sender:
Followup-To:
Distribution:
Keywords:

Well, the expected mail caused by my flame has been running about 2-1
for those more-or-less agreeing with my critique, as opposed to those
who just don't like flamers.

But I'm going to surprise you all.

You see, while I don't like incompetence, and I reserve a particularly
toasty place for those who not only are incompetent but also teach
(hence the virulence of my previous posting), Mr. Webber has done the
only thing which, under the circumstances, could obtain my approval:
he worked to improve his program.

I also owe him a half an apology for missing the essential point of
his program: that it is intended to overwrite the output file
*without* freeing any blocks before the write is finished.  But only
half of one: this essential point was not documented.

As a consequence of these, I have removed him from my kill file and
shall remain civil till given cause.

So, to the meat of the matter:

In article <Jan.8.18.48.59.1989.10861@athos.rutgers.edu> webber@athos.rutgers.edu (Bob Webber) writes:
: > Second, it is nonportable. It is Berkeleyoid specific code.
: > Ftruncate marks it as such. There are also those UNIX-specific I/O
: > calls.
:
: It is as portable as possible.  Tell me how to accomplish the same
: correctly on a POSIX/v7 system and I will be happy to make the program
: more portable.

On ftruncate, I think you are right: other versions of UNIX don't have
the capability, as far as I know. However, the UNIX I/O calls should
not have been used, the standard I/O calls should have instead; the
call to ftruncate should have used fileno() to get the fd it wants.
Also, the use of ftruncate (and fileno, if used) needs to be
commented.  I note that you did so in your latest.

Another portability nit: the program depends overmuch on system-
specific include files. As near as I can tell, the only thing
obtained from the include files are the standard I/O stuff and the
constant MAXBSIZE. The include of stdio.h is, of course, necessary,
but the rest don't appear to be. As for the constant, BUFSIZ, from
stdio.h, should be used instead.

: >     1) The glorified numeric error codes. I mean really!  Each error
: >        message is just "Error so-and-so : read the source..."!  Each
: >        error message should have said something useful, dammit!
:
: That has been improved in the new release (Jan688 release of INSERT in
: alt.sources).  However, since perror was used to print the original
: messages, they were pretty useful to begin with.

The error codes in your new version are a slight improvement; however,
they could be significantly better.

: >     3) The unnecessary code sequences. The lseek's
:
: The lseeks position the file at the beginning.  Oddly enough, the man
: page on open doesn't assert that this is default on our system. From a
: diagnostic point of view, the lseeks let one know what the state of
: the file is before the first read, which could be interesting.

The lseek's are completely unnecessary, regardless of omissions in
the man page. And of course, one should use standard I/O anyway.

: >     4) The use of a cast to create a long constant.
:
: Aren't you worried about portability to systems with 16 bit ints
: running BSD?  The man page clearly says that arg is a long and lint
: expects it.

One should write a long integer constant as the value followed by a
lower or upper case `l', as in 0L.

: >     5) Probably others, but I'm too blind mad to read it again.
:
: I guess it was a rather long piece of code for you to have to read
: all the way thru.

Hardly. However, I did miss these three points about the program:

    1) The name of the program follows the grand UNIX tradition: it
       has significance only to the author. How about calling it
       something like `fileover' (for `file overwrite')?

    2) `Int ReadSize;'! Try `long ReadSize;'.

    3) The program is abysmally documented, though it has been
       slightly improved in the latest version.

Here are suggestions for improving the documentation:

    1) Fix the program name.

    2) Note the nonportabilities in the header.

    3) Fix the error messages. Remove the error numbers; to whomever
       they might be useful to, the exit codes will be as useful.
       Make the rest of the messages meaningful to one who does not
       have the source code.

       Example: change

      perror("Error 1 : ((fdin = open(argv[1],O_RDONLY)) == -1):");

       into (after setting up ProgramName)

      fprintf(stderr, "%s: error while opening %s for read: ",
	  ProgramName, argv[1]);
      perror("");

    4) State the purpose and functioning of the program more clearly.
       In particular, state that the program works even when there
       are other processes that could grab the file space that would
       have been freed had cp been used instead.

    5) There are two distinct code sections, one for argument parsing
       and one for copying the file. These each deserve at least a
       one line comment, to distinguish the two.

One final point: in your new-and-improved version, you essentially
duplicate the open code for the output file. Better would be to set a
variable to the argument to be used for the output file and then do
the output open in one place.

---
Bill
{ uunet!proxftl | novavax } !twwells!bill