strcpy error

[h4ever]

Can you tell me why I have error when I try to copy

ch = strtok(ent->d_name, ".");
char array from ch to chTmp?

I did this according http://www.cplusplus.com/reference/cstring/strcpy/
SIGSEGV error in code::blocks

#include "include/types.h"
#include "include/gaussian.h"
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

#include <dirent.h>
#include <string.h> // strcmp, strtok
#include <sys/types.h> // mkdir
#include <sys/stat.h> // mkdir

int readFiles(char ** path, FILES * files)
{
    DIR *dir;
    struct dirent *ent;
    int ckey = 0; int c = 0; int count = -1;
    char * ch; char * chTmp;

    if ( ( dir = opendir (*path) ) != NULL)
    {
        /* print all the files and directories within directory */
        while ((ent = readdir (dir)) != NULL)
        {
            if ( ckey<2 && ( strcmp(".",ent->d_name) == 0 ||  strcmp("..",ent->d_name) == 0 )  )
                { ckey++; continue; }

            ch = strtok(ent->d_name, ".");
            strcpy(chTmp,ch);
            while (ch != NULL)
                {
                c++;
                printf("%s\n", ch);
                ch = strtok(NULL, ".");
                if (c>1){
                    printf("Incorrect kernel source name. File name %s, must not contain dots, but must have .cl extension.\n",ent->d_name);
                    }
                else if (c==1 &&
                             strcmp("cl",ch)==0 )
                        {
                        count++;
                        (files[count]).name = chTmp;
                        (files[count]).filename = ent->d_name;
                        puts("Ok, this is correct\n");
                        }
                }

            printf (".'%s'\n", ent->d_name);
        }
        closedir (dir);
    }
    else
    {
        /* could not open directory */
        char error [255];
        sprintf(error, "Directory not found: %s", *path);
        perror (error);
        return EXIT_FAILURE;
    }
return true;
}

[keskiverto]

Your code:

char * ch;
char * chTmp;

ch = strtok(ent->d_name, ".");
strcpy(chTmp,ch);
while ( NULL != ch ) {
  ch = strtok( NULL, ".");
}

1. Do you remember that strtok modifies the ent->d_name?

2. Is the first token special, because you don't copy the others?

3. "According to strcpy manual." Expletive!

Q: Which part of the following did you use?

To avoid overflows, the size of the array pointed by destination shall be long enough to contain the same C string as source (including the terminating null character), and should not overlap in memory with source.

A: None, apparently.

[h4ever]

Ad1) I had no idea
Ad2) I don't know what you mean, but I have to strcpy it when it would be modified
Ad4) Should I use malloc to allocate the memory?

Something like this?

ch =  (char*) malloc( sizeof(char)*256 );
chTemp =  (char*) malloc( sizeof(char)*256 );

If I would do char * ch[256]; so the strtok() will complain about different types

[keskiverto]

Ad1) I had no idea

It was told to you in your earlier thread. Should we assume that you read nothing that we write to you?

Should I use malloc to allocate the memory?
If I would do char * ch[256]; so the strtok() will complain about different types

The very same strcpy manual page that you did point to does have a usage example. Does it use malloc? Does it use array of pointers?

[dhayden]

If you're going to use char *, you need to think about who owns the string: who will delete it? When? This is good reason to use std::string instead. For example, your code has several problems:
Line 27: strtok() will put replace the '.' with a null character. That means lines 35 & 47 won't print the full file name.
Lines 28 copies the name into some undefined area of memory since chTmp is uninitialized.
Lines 41 & 42: If FILES::name and FILES::filename are char* then this is wrong because chTmp is uninitialized and ent->d_name is owned by ent.

You're better off using std::string for most of this stuff. Make sure FILES::name and FILES::filename are std::string and then inside your loop, grab a copy of the file name in a string:
std::string name(ent->d_name);

To see if the file name ends with ".cl" and has no other '.' characters, find the first '.' and then see if that matches ".cl" at the end:

size_t pos = name.find('.');
if (pos != string::npos && pos == name.find_first_of(".cl") && pos == name.size()-3)

I hope this is helpful.

[h4ever]

keskiverto:
Ad1) I don't remember that
Ad1) They don't use strtok

[TheIdeasMan]

@dhayden

The OP is doing C , and may not be permitted to do C++.

[keskiverto]

Ok, lets take the strcpy and strtok examples from respective manual pages, mutilate them together, and leave a little bit for you to cross-reference:

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

int main ()
{
  const char source[] ="- This, a sample string.";
  // like ent->d_name, we keep source unchanged
  char * pch;

  // declare suitable str2

  strcpy( str2, source );

  printf( "Splitting string \"%s\" into tokens:\n", str2 );
  pch = strtok( str2, " ,.-" );
  while ( pch != NULL )
  {
    printf( "%s\n", pch );
    pch = strtok( NULL, " ,.-" );
  }

  return 0;
}

How did the strcpy example declare the str2?

[h4ever]

But I cannot set ent->d_name as const. The variable is changing while reading file names from directory. I don't solve the strtok right now, I don't think I have problem with it (I did some changes). New problems arosen

[keskiverto]

Why should ent->d_name be const?

What is your changed version?

Despite solving your problem you should still, for educational purposes, tell how to declare str2 in my sample code.

[h4ever]

Once text lost

#include "include/types.h"
#include "include/gaussian.h"
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

#include <dirent.h>
#include <string.h> // strcmp, strtok
#include <sys/types.h> // mkdir
#include <sys/stat.h> // mkdir

int readFiles(char ** path, FILES * files)
{
    DIR *dir;
    struct dirent *ent;
    int ckey = 0; int c; int count = -1;
    char * ch;
    ch =  (char*) malloc( sizeof(char)*256 );
    char nameTmp[260];
    char fileNameTmp [260];

    if ( ( dir = opendir (*path) ) != NULL)
    {
        /* print all the files and directories within directory */
        while ((ent = readdir (dir)) != NULL)
        {
            if ( ckey<2 && ( strcmp(".",ent->d_name) == 0 ||  strcmp("..",ent->d_name) == 0 )  )
                { ckey++; continue; }

            // backup ent->d_name because chTmp2 is changed by strtok
            strcpy(fileNameTmp,ent->d_name);
            ch = strtok(fileNameTmp, ".");
            strcpy(nameTmp,ch);
            c = -1;
            while (ch != NULL)
                {
                c++;
                printf("%s\n", ch);
                ch = strtok(NULL, ".");
                if (c>1){
                    printf("Incorrect kernel source name. File name %s, must not contain dots, but must have .cl extension.\n",ent->d_name);
                    }
                else if (c==1 &&
                             strcmp("cl",ch)==0 )
                        {
                        count++;
                        files[count].name =  (char*) malloc( sizeof(char)*64 );
                        files[count].filename =  (char*) malloc( sizeof(char)*256 );
                        strcpy(files[count].name,nameTmp);
                        strcpy(files[count].filename,ent->d_name);
                        }
                }

            printf (".'%s'\n", ent->d_name);
        }
        closedir (dir);
    }
    else
    {
        /* could not open directory */
        char error [255];
        sprintf(error, "Directory not found: %s", *path);
        perror (error);
        return EXIT_FAILURE;
    }
return true;
}

error on the line
strcmp("cl",ch)==0 )
ch is 0x0

http://oi57.tinypic.com/105t7y0.jpg

why it does not evaluate the condition to be true? ch was "cl"

[keskiverto]

Elementary: c != 1


What are you doing on line 18?

What is FILES?

[coder777]

why it does not evaluate the condition to be true?

Because when c==1 you've called strtok() the third time.

[keskiverto]

Written without thinking. What would it print on your framework?

while ((ent = readdir (dir)) != NULL)
{
  if ( 0 == strcmp(".", ent->d_name) ) continue; // curr dir
  if ( 0 == strcmp("..", ent->d_name) ) continue; // parent dir

  char * dot = strchr( ent->d_name, '.' );
  if ( dot )
  {
    if ( 0 == strcmp( "cl", dot+1 ) )
    {
      char name[64];
      char filename[64];
      strcpy( filename, ent->d_name );
      strcpy( name, filename );
      int len = strlen( name ) - 3;
      if ( 0 <= len ) {
        name[ len ] = '\0';
        printf( "Source '%s' is in ", name );
      }
      printf( "'%s'\n", filename );
    }
    else
    {
      // greedy extension is not 'cl'
    }
  }
  else
  {
    // no dot on name. Could be dir or whatever
  }
}

[h4ever]

Thanks, I think it works now. But I don't know if lines 47-48 are OK.

Keskiverto:

char name[64];
char filename[64];

I thought it would be redeclaration. But you dont save the values to array but just reset the variables. I added the ckey<2 condition to skip calling of strcmp on every file.

[keskiverto]

Regarding the ckey, make no assumptions. Besides, those are rather short comparisons. Your optimization does not gain you anything but complexity.

I was not copying data to anywhere; that part you have to work on anyway.

Can you tell how my code differs from your code?



I do predict that the caller of readFiles() will crash.

What is FILES?
What is files (in the function that calls readFiles)?
How does the caller know how many files were found?

[h4ever]

your code is simpler coz U dont use strtok

[h4ever]

I have yet another idea how to simplify it. I don't need the array right now,
in the -install mode of my program. I would just gather the names and filenames and add the to txt file so it can be simply parsed in -load mode.

Another simplification what I could do is first to substract the .cl and test if this exists, which should have more priority because you need not to search complete string but you know the position. However, if this would be more effective depends on on how the strlen() function works. If it uses loop to search the \000 character then it would be same effectiveness because strchr() also uses loop.

In the first case I would be interested how to make the test working:

char ext[3];
unsigned char len = strlen(ent->d_name);
ent->d_name=*(ent->d_name)+strlen(ent->d_name)-3; // here I need to set the pointer of ent->d_name to refert to adress ent->d_name + +strlen(ent->d_name)-3
memcpy ( ext, ent->d_name, 3 );

[keskiverto]

You are partially right, there is no need to test for the "." and ".." at all.

The real difference between your approach and mine is that you step each filename one dot at a time until you get to the end, while I only check whether the filename contains exactly the "cl" after the first dot. It is not about which functions are used, but the logic.


What if filename has less than 3 characters? The len-3 goes haywire.

Your line 2:
The return type of strlen is not unsigned char. You must read the documentation of the functions that you do use.

Your line 3:
d_name is a pointer or array, so (*ent->d_name) means same as ent->d_name[0], i.e. one character. What happens, if you write:

char a[8] = "Hello";
a[3] = 7;

Your line 3 does something very similar.

Lets assume that ent->d_name is a pointer. If you do advance it, then it will not point to the beginning of the filename any more. What you want is a new pointer:

size_t len = strlen( ent->d_name );
char * p = ent->d_name + len - 3;

However, the strchr does practically the same thing, and is safer.

Your line 4:
Why copy at all? If you do have a pointer to third character from end, then you effectively have a 3-character string that you can compare with ".cl" (like my code does).

Besides, if you just look at the end, then you don't test whether there are more dots earlier.


Do not try to optimize. Seriously. You have enough challenge on writing both syntactically and logically correct code. Thinking about whether this or that standard function uses one less CPU cycle is utter waste of your time.


Unrelated note, you return either EXIT_FAILURE or true. That looks inconsistent. What are the values of those? Are they both 1?

[h4ever]

I have removed the EXIT_FAILURE and make it -1

But ent->d_name is not pointer but char[260]
so would I need to make a pointer from it?

But it does not work

char * pName = ent->d_name;
if ( strlen(ent->d_name) >2 )
  {
  char * ext = pName + strlen(ent->d_name) - 3;
  }

value of ext: "Ext not available in current context"