Ceri Davies writes:
> > -   cmd = parsecmd(*argv++);
> > +   if (fopen(*(argv+1)){
[...]
> Finally, this check should take place during argument parsing in main().

More comments:

  - Why "*(argv+1)"?  What's wrong with the equivalent but clearer
    argv[1]?

  - Won't this change -- if it could be made to compile -- just dump
    core?  The previous code dereferenced argv *first*, and *then*
    incremented the pointer.  You've changed that to skip over the
    first argument, and then (with the "+1") dereference the second,
    and to leave argv still pointing at the first argument.

  - The return value from fopen is discarded after being checked
    (implicitly against NULL).  Besides the usual ON rule that
    pointers aren't booleans and shouldn't be tested that way, this
    call leaks the file handle returned by fopen.  Are you sure you
    wanted to open the file at all?

Don't expect your code reviewers or sponsor or random contributors on
request-sponsor to make your code compile or fix the design.  Please
make it right _first_, and then seek a review.

-- 
James Carlson, Solaris Networking              <james.d.carlson at sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

Reply via email to