Re: newbie question: whats wrong with my code?

On Mar 24, 10:36 am, Daniel Rudy <spamt...@xxxxxxxxxxxx> wrote:
At about the time of 3/24/2007 3:23 AM, Daniel Rudy stated the following:

At about the time of 3/23/2007 11:17 AM, ericun*** stated the following:
I'm trying to copying a file to another file using the follwoing code,
but it's not working properly, some data is missing so I can't finally
recover the file.

This is a complete version of what you are trying to do. As for the
regulars, critiques are welcome. :)

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

#define BUFFSIZE 65536 /* buffer size */

Why not just use BUFSIZ from stdio.h?

#define FILENAMESIZE 1024 /* max size of filenames */

int main(int argc, char **argv)
{
size_t readsize; /* size of data read */
size_t writesize; /* size of data written */
int eof_flag; /* flag if eof encountered */
FILE *infile; /* file pointer for input file */
FILE *outfile; /* file pointer for output file */
void *buffer; /* pointer to buffer space */

Hmmm, why not declare buffer as a char * or unsigned char *?

char infilename[FILENAMESIZE]; /* input filename */
char outfilename[FILENAMESIZE]; /* output filename */

/* get filenames */
if (argc != 3)
{
printf("Enter path/filename to copy -->");
fgets(infilename, sizeof(infilename), stdin);
printf("Enter destination path/filename -->");
fgets(outfilename, sizeof(outfilename), stdin);
}
else
{
if (strlen(argv[1]) > sizeof(infilename) - 2)
{
printf("error: input filename too long\n");

This is an error message. It should probably go to stderr rather
than stdout.

exit(EXIT_FAILURE);
}
if (strlen(argv[2]) > sizeof(outfilename) - 2)
{
printf("error: output filename too long\n");
exit(EXIT_FAILURE);
}
strncpy(infilename, argv[1], sizeof(infilename) - 2);

Why make copies of the filenames? Why not just declare infilename
as a char * and have "infilename = argv[1];" Also, since
you've already checked the lengths of the filenames, you know
that strncpy is going to write the null at the end, so
there's no need to do it.

infilename[sizeof(infilename) - 1] = '\0';
strncpy(outfilename, argv[2], sizeof(outfilename) - 2);
outfilename[sizeof(outfilename) - 1] = '\0';
}

/* allocate buffer space */
buffer = malloc(BUFFSIZE);
if (buffer == NULL)
{
printf("error: unable to allocate buffer space: %s\n", strerror(errno));
exit(EXIT_FAILURE);
}

/* open input and output files */
infile = fopen(infilename, "rb");
if (infile == NULL)
{
printf("error: unable to open input file %s: %s\n", infilename, strerror(errno));
free(buffer);
exit(EXIT_FAILURE);
}
outfile = fopen(outfilename, "wb");
if (outfile == NULL)
{
printf("error: unable to open output file %s: %s\n", outfilename, strerror(errno));
free(buffer);
fclose(infile);
exit(EXIT_FAILURE);
}

/* copy input file to output file in a loop */
eof_flag = 0;
while (eof_flag == 0)
{

/* read input file into buffer */
readsize = fread(buffer, 1, BUFFSIZE, infile);

Just a style point, I prefer
fread(buffer, sizeof *buffer, ...), which won't work since you
declared buffer as a void *.

if (readsize < BUFFSIZE) /* can only be caused by a eof or error */
{
if (feof != 0) eof_flag = 1;

feof shuould probably be called. eg feof(infile)

else
{
printf("error: error reading file: %s\n", strerror(errno));
fpurge(outfile);
fclose(infile);
fclose(outfile);
remove(outfilename);
free(buffer);
exit(EXIT_FAILURE);
}
}

/* write contents of buffer to output file */
writesize = fwrite(buffer, 1, readsize, outfile);
if (writesize < readsize) /* can only be caused by an error */
{
printf("error: error writing to file: %s\n", strerror(errno));
fpurge(outfile);
fclose(infile);
fclose(outfile);
remove(outfilename);
free(buffer);
exit(EXIT_FAILURE);
}
}

/* clean up */
free(buffer);
fflush(outfile);
fclose(infile);
if (fclose(outfile) == EOF)
{
printf("error: problem closing output file: %s\n", strerror(errno));
exit(EXIT_FAILURE);
}

/* return to operating system */
exit(EXIT_SUCCESS);
}

As you can see, about half the code is dealing with errors. I
didn't use perror though like you did.

Overall, the main function is way too long, and I don't like
seeing the error checking stuff strewn throughout. It's hard
to see the flow of the program. Here's my attempt, and I'm
not sure I completely like it. Rather than checking for
read errors immediately, I handle a read error by storing
errno, calling fwrite with a zero count, and then giving
a warning about the read error later (I'm not convinced
that I like that technique, but it makes the processing
loop very readable.)

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

struct args {
FILE *in;
FILE *out;
char *in_name;
char *out_name;
};

int clean_up( int readerr, struct args *A );
void parse_args( struct args *A, int argc, char **argv );


int
main( int argc, char **argv )
{
struct args A;
size_t rc; /* Read count. */
int re; /* Read error. */
size_t wc; /* Write count. */
unsigned char buf[ BUFSIZ ];

parse_args( &A, argc, argv );
do {
rc = fread( buf, sizeof *buf, BUFSIZ, A.in );
re = errno;
wc = fwrite( buf, sizeof *buf, rc, A.out );
} while( rc == wc && rc == BUFSIZ );

return clean_up( re, &A );
}

int
clean_up( int readerr, struct args *A )
{
int status = EXIT_SUCCESS;

if( ferror( A->out )) {
status = EXIT_FAILURE;
perror( A->out_name );
}

if( ferror( A->in )) {
status = EXIT_FAILURE;
fprintf( stderr, "%s: %s", A->in_name,
strerror( readerr ));
}

if( fclose( A->in )) {
status = EXIT_FAILURE;
perror( A->in_name );
}

if( fclose( A->out )) {
status = EXIT_FAILURE;
perror( A->out_name );
}

return status;
}


void
parse_args( struct args *A, int argc, char **argv )
{
A->in = stdin;
A->out = stdout;
A->in_name = "stdin";
A->out_name = "stdout";

if( argc > 1 ) {
A->in_name = argv[1];
A->in = fopen( A->in_name, "rb" );
if( A->in == NULL ) {
perror( A->in_name );
exit( EXIT_FAILURE );
}
}

if( argc > 2 ) {
A->out_name = argv[2];
A->out = fopen( A->out_name, "wb" );
if( A->out == NULL ) {
perror( A->out_name );
if( fclose( A->in ))
perror( A->in_name );
exit( EXIT_FAILURE );
}
}
}


--
Bill Pursell

.