Here are few observation. Firstly, here
fp = fopen("pO5.c", "rb");
one should always check the return type of fopen()
to know whether call to fopen()
was success of failure. It may cause issue if fopen()
failed & you are operating on fp
further. Proper error handling is required, for e.g
fp = fopen("pO5.c", "rb");
if(fp == NULL) {
fprintf(stderr, "Can't open %s: %s\n", "pO5.c", strerror(errno)); /* include header for errno */
exit(EXIT_FAILURE);
}
Secondly, here
buf = (char*)malloc(sizeof(buf) * 1000);
int n = fread(buf, 1, 100, fp);
don't use magic number like 100
or 1000
, instead its advisable to use macro for this purpose.
Also sizeof(buf)
is size of pointer, but you wanted it to be sizeof(*buf)
.
And typecasting malloc()
result is not required here. For e.g
buf = malloc(sizeof(*buf) * BUF_MAX_SIZE); /* define BUF_MAX_SIZE */
Also do check return value of malloc()
for e.g
buf = malloc(sizeof(*buf) * BUF_MAX_SIZE); /* define BUF_MAX_SIZE */
if(buf == NULL) {
/* @TODO error handling if malloc failed */
}
And below code block is flawed
while (1) {
buf = (char*)malloc(sizeof(buf) * 1000);
int n = fread(buf, 1, 100, fp);
if (n == 0) {
break;
}
fwrite(buf, 1, n, stdout);
}
Because of mainly one reason
- you are not freeing each malloc'ed memory ? Its memory leak as every time
buf
reassigned with new dynamic address & no object or pointer
pointing to previously allocated memory.
One solution for the above issue can be this one
buf = malloc(sizeof(*buf) * BUF_MAX_SIZE); /* calling malloc() n times costs more, it can be avoided by allocating memory before while(1) block */
if(buf == NULL) {
/* @TODO error handling of malloc */
}
int n = 0; /* declare here */
while (1) {
n = fread(buf, 1, BUF_MAX_SIZE, fp); /* reading BUF_MAX_SIZE bytes every time from file & storing into buf */
if (n == 0) {
break;
}
fwrite(buf, 1, n, stdout);/* put buf to stdout */
/* zero'd the BUF_MAX_SIZE bytes of buf, can use memset() here */
}
free(buf); /* free here */