0

So I have the following piece of code however when I try to run the function more than once, it does not work.

Any help would be great. I have tried to free the memory allocated to target but this function only works on the first call.

Edit: Have moved fopen to function and have included fclose. Also changed fopen error to perror().

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

FILE *fo;
int main (int argc, char *argv[]) {
  
  readFile("<Text1>","</Text1>");
  readFile("<Text2>","</Text2>");

  return (0);
}

int readFile(char *start_tg, char *end_tg) {         

  char line[100];
  char *start;
  char *end;
  char *target = NULL;
  char pattern1[30];
  char pattern2[30];

  fo = fopen("doc.lst","r");

  if(fo == NULL){
    perror("doc.lst");
    exit(1);
  }

  strcpy(pattern1,start_tg);
  strcpy(pattern2,end_tg);

  while(fgets(line, sizeof(line), fo)) {

    if((start = strstr(line,pattern1)) != null) {

      start += strlen(pattern1);

      if ((end = strstr(start,pattern2)) != null) {

        target = malloc( strstr(start,pattern2) - strstr(line,pattern1) + 1);

        if(target) {
          fprintf(stdout,"\n String %s\n",target);
          free(target);
        } 

        memcpy( target, start, end - start);
        target[end - start] = '\0';
      } 
    }
  }

  fclose(fo);

  return (0);

}
Carl
  • 1
  • 1
  • 6
    Because first time it is running it is reading the file to the end. You will need to rewind it or close/open again. – Eugene Sh. Jul 16 '21 at 17:44
  • 5
    I don't believe you've posted your actual code. You're declaring and opening `fo` in `main`, and using it in `readFile`, where it isn't visible. – Steve Summit Jul 16 '21 at 17:44
  • 1
    `"Unable to open the file\n"` is a useless error message, and you've written it to the wrong stream. Use `perror("doc.lst");` – William Pursell Jul 16 '21 at 17:46
  • Generally prefer `if ((start = strstr(x,y)) != null)` than `if (start = strstr(x,y))`. – jarmod Jul 16 '21 at 17:54
  • @EugeneSh. I have added the file open/close however it still runs only once. I have updated the code. – Carl Jul 16 '21 at 17:59
  • WilliamPursell and jarmod, Thanks for the tips. I have updated the code. – Carl Jul 16 '21 at 18:01
  • 1
    Thanks everyone, managed to get it working with the advice from @SteveSummit. – Carl Jul 16 '21 at 18:08
  • your `if(target)` check should be directly after `malloc`. If `malloc` returns NULL then `memcpy` and `target[end - start] = '\0';` invoke undefined behavior. – yano Jul 16 '21 at 18:11
  • it's also bad practice to cast the return value of `malloc`: https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc – yano Jul 16 '21 at 18:12
  • @yano Thanks for all this information, I am quite new to this. I have updated the code above, could you check whether I have addressed all your points. Greatly appreciated if you could. – Carl Jul 16 '21 at 18:21
  • 1
    `start = strstr(line,pattern1) != null` same as `start = (strstr(line,pattern1) != null)`. Certainly incorrect. Carl, save time, enable all warnings. – chux - Reinstate Monica Jul 16 '21 at 18:23
  • @yano I have moved the if (target) statement above the memcpy and tartet[end-start] however this core dumps. Work fine If I keep this under both of them. – Carl Jul 16 '21 at 18:26
  • Sorry, I didn't mean the statement as is. If `malloc` returns a non-NULL pointer, then all you have is a pointer to a chunk of uninitialized memory. You need to do your `memcpy` and `NUL` terminate it before printing as you had. What I meant was, it's possible for `malloc` to return NULL, and you want to check that it is non-NULL _before_ trying to use it in any way. If `target` is NULL, you don't have any memory to `memcpy` into, and doing so invokes UB. – yano Jul 16 '21 at 18:31
  • @yano I see what you mean. So I need to run memcpy and the 2nd line only if target is not null. Got it thanks. – Carl Jul 16 '21 at 18:33
  • ^^ yes exactly. You should do something like `target = malloc(..); if (target){ memcpy(...); target[end-start] = '\0'; fprintf( ... ) } else{ /* handle out of memory error */ }` – yano Jul 16 '21 at 18:34
  • Please read [ask]. "it does not work" is not a good problem statement. Your question should state what the function does do on the first pass and that it doesn't do those things on the following uses. – jwdonahue Jul 16 '21 at 18:46

0 Answers0