Thread: I can't concatenate a string

I have this function in my code:

Code:

{
    close(pipe_dos[1]);
    close(pipe_uno[1]);
    close(pipe_uno[0]);
    int e = 0, i = 0, x = 0;
    char buffer[100] = "";
    char palabra[15] = "\0";
    char aux;
    while((i = read(pipe_dos[0], buffer, 100)) > 0)
    {
        for(e = 0; e < 100; e++)
        {
            if(buffer[e] != ' ' && buffer[e] != '.' && buffer[e] != ',' && buffer[e] != '\n')
            {
                aux = buffer[e];
                strcat(palabra, &aux);
            }
            else
            {
                if(strcmp(palabra, "cinco"))
                {
                    //hola 
                    //12345
                    for(x = 0; x < strlen(palabra)-1; x++)
                    {
                        buffer[e-1-x] = toupper(buffer[e-1-x]);
                    }
                }
            }
        }        
    }
}

In the line 16 I try to concatenate char palabra[15] with a char aux.
I do that several times to extract a word from char buffer[100].
The problem is palabra only stores trash. If I print char palabra[15] inside de else (line 18), I get "Palabra: HoHlHoHaHoHlHoH"

What is happening?
This is the entire code

Code:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <string.h>
#include <ctype.h>


void proc_uno();
void proc_dos();


int pipe_uno[2], pipe_dos[2];


int main(int argc, char ** argv)
{
    int pid1, pid2, i;
    getopt(argc, argv, "f:");    
    int fd = open(optarg, O_RDONLY);
    pipe(pipe_uno);
    pipe(pipe_dos);
    char buffer[100] = "";
    if((pid1 = fork()) == 0)
    {
        proc_uno();
        return 0;
    }
    close(pipe_uno[0]);
    if((pid2 = fork()) == 0)
    {
        proc_dos();
        return 0;
    }
    close(pipe_dos[0]);
    while((i = read(fd, buffer, 100)) != 0)
    {
        write(pipe_uno[1], buffer, 100);
        write(pipe_dos[1], buffer, 100);
    }    
    return 0;
}




void proc_uno()
{
    int e, letras, i;
    int palabras[15] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
    char buffer[100] = "";
    letras = -1;
    i = 1;
    close(pipe_uno[1]);
    close(pipe_dos[0]);
    close(pipe_dos[1]);
    while(( i = read(pipe_uno[0], buffer, 100)) > 0)
    {
        for(e = 0; e < 100; e++)
        {
            if(buffer[e] != ' ' && buffer[e] != '.' && buffer[e] != ',' && buffer[e] != '\n')
                letras++;
            else
            {
                palabras[letras]++;
                letras = -1;
            }
        }
    }
    for(e = 0; e < 15; e++)
    {    
        if(palabras[e] != 0)
        {
            printf("Hay %d palabras de %d letras.\n", palabras[e], e+1);
        }
    }


}
void proc_dos()
{
    close(pipe_dos[1]);
    close(pipe_uno[1]);
    close(pipe_uno[0]);
    int e = 0, i = 0, x = 0;
    char buffer[100] = "";
    char palabra[15] = "\0";
    char aux;
    while((i = read(pipe_dos[0], buffer, 100)) > 0)
    {
        for(e = 0; e < 100; e++)
        {
            if(buffer[e] != ' ' && buffer[e] != '.' && buffer[e] != ',' && buffer[e] != '\n')
            {
                aux = buffer[e];
                strcat(palabra, &aux);
            }
            else
            {
                if(strcmp(palabra, "cinco"))
                {
                    //hola 
                    //12345
                    for(x = 0; x < strlen(palabra)-1; x++)
                    {
                        buffer[e-1-x] = toupper(buffer[e-1-x]);
                    }
                }
            }
        }        
    }
}

Both arguments to strcat must be strings. Just passing the address of a char variable is not good enough (unless that char happens to hold '\0', which would be pointless anyway). Even if you only want to "concatenate" one char, there needs to be a '\0' after the char. So make aux a char[2] and ensure that aux[1] is '\0'.

Also, the e loop should only go up to i, not 100, since the read may have read less than 100 chars.

Code:

    // ...
    char palabra[100] = {0};  // no need to be stingy with the size
    char aux[2] = {0}; // set all chars to '\0', ensuring that aux[1] is '\0'
    while((i = read(pipe_dos[0], buffer, 100)) > 0)
    {
        for(e = 0; e < i; e++)   // only loop up to i
        {
            if(buffer[e] != ' ' && buffer[e] != '.' && buffer[e] != ',' && buffer[e] != '\n')
            {
                aux[0] = buffer[e];   // set aux[0] to the char
                strcat(palabra, aux);  // now this should work properly
            }
    // ...

And I hope you realize that his is saying "if palabra is NOT equal to "cinco"."

Code:

if (strcmp(palabra, "cinco"))

It's better to explicitly compare it to 0 since it communicates more clearly that you actually know what you are doing (and it reads better since it has a != in it).

Code:

if (strcmp(palabra, "cinco") != 0)    // if they are not equal

I didn't look at the rest of your code because I was in a hurry, so there may be other problems.

EDIT: Taking a quick peek at the rest of your code, it looks like there are other problems, but I don't have time to deal with them right now.
BTW, you should explain what your program is supposed to do and also how to run it, since it takes command line arguments. Give an example.

> if((pid2 = fork()) == 0)
How many processes are you going to have in total when you pass this line?

> Three, the father and two sons
And the first child also gets a child of it's own as well.

Originally Posted by qqsszz

What other problems do you see in the code?

I'll look at it if you post your current code and also explain what you expect it to do. Give example input/output.

proc2 does nothing. It converts its own buffer to uppercase. What's the point?

You should check that the file opened.

Your pipes don't need to be global. Make them local to main and pass only the read side of the relevant pipe to each function.

You should close the write ends of the pipes in main before the return 0. You should probably also wait() for the children.

You don't need to do this:

Code:

    int palabras[21] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};

Instead you can just put one 0 and the rest will all be set to 0's, too.

Code:

    int palabras[21] = {0};

Instead of this

Code:

            if(buffer[e] != ' ' && buffer[e] != '.' && buffer[e] != ',' && buffer[e] != '\n')

you might want to say this

Code:

            if(isalpha(buffer[e]))

The printf("%s", buffer) in proc_dos needs to be inside the loop. But buffer is not guaranteed to be zero-terminated, so you can't actually print it as a string. If you want to output a non-zero-terminated char array you need to specify the number of chars to output. You could use write to do that: write(1, buffer, n). (The 1 just stands for stdout. n is the number of valid chars in buffer.)

But even then, your algorithm will fail if the word crosses a buffer boundary. You need to get a little more sophisticated to fix that. See below.

And you have no guarantee that the output of the two child processes won't get mixed together. Maybe one of the children should write to a file instead of stdout.

Code:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>
#include <ctype.h>
#include <sys/wait.h>

#define BUFFER_SIZE 100
#define MAX_PALABRA 21

void proc_uno(int fd);
void proc_dos(int fd);

int main(int argc, char ** argv) {
    getopt(argc, argv, "f:");   
    int fd = open(optarg, O_RDONLY);
    if (fd == -1) {
        perror("open");
        return EXIT_FAILURE;
    }

    int pipe_uno[2];
    pipe(pipe_uno);
    if(fork() == 0) {
        close(pipe_uno[1]);
        proc_uno(pipe_uno[0]);
        return 0;
    }
    close(pipe_uno[0]);

    int pipe_dos[2];
    pipe(pipe_dos);
    if(fork() == 0) {
        close(pipe_dos[1]);
        close(pipe_uno[1]);
        proc_dos(pipe_dos[0]);
        return 0;
    }
    close(pipe_dos[0]);

    char buffer[BUFFER_SIZE] = "";
    int n;
    while((n = read(fd, buffer, BUFFER_SIZE)) > 0) {  // you had != 0 here
        write(pipe_uno[1], buffer, n);   // use n, not 100
        write(pipe_dos[1], buffer, n);
    }   

    close(pipe_uno[1]);
    close(pipe_dos[1]);
    wait(NULL);
    wait(NULL);
    return 0;
}

void proc_uno(int fd) {
    int palabras[MAX_PALABRA] = {0};
    char buffer[BUFFER_SIZE] = "";
    int letras = 0;
    int n;
    while((n = read(fd, buffer, BUFFER_SIZE)) > 0) {
        for(int e = 0; e < n; e++) {
            if(isalpha(buffer[e]))
                letras++;
            else {
                if(letras <= MAX_PALABRA)  // ensure no access outside array
                    palabras[letras - 1]++;
                letras = 0;
            }
        }
    }
    for(int e = 0; e < MAX_PALABRA; e++)
        if(palabras[e] != 0)
            printf("%2d. %5d\n", e+1, palabras[e]);
}

void uppercase(char *palabra) {
    if(strcmp(palabra, "al")  == 0 ||
       strcmp(palabra, "los") == 0 ||
       strcmp(palabra, "rey") == 0)
        for(int x = 0; x < strlen(palabra); x++)
            palabra[x] = toupper(palabra[x]);
}

void proc_dos(int fd) {
    char buffer[BUFFER_SIZE] = "";
    char palabra[MAX_PALABRA+1] = "";   // "\0" is basically identical to ""
    int k = 0;
    int n;
    while((n = read(fd, buffer, BUFFER_SIZE)) > 0) {
        for(int e = 0; e < n; e++) {
            if(isalpha(buffer[e])) {
                if (k <= MAX_PALABRA)  // protect from overflow
                    palabra[k++] = buffer[e];
            }
            else {
                palabra[k] = '\0';
                uppercase(palabra);
                printf("%s%c", palabra, buffer[e]);
                k = 0;
            }
        }
    }
    // Just in case the last word doesn't have a delimiter after it
    palabra[k] = '\0';
    uppercase(palabra);
    printf("%s", palabra);
}