store characters in character pointer

Question

I have a thread which parses incomming characters/bytes one by one. I would like to store the sequence of bytes in a byte pointer, and in the end when the sequence of "\r\n" is found it should print the full message out.

unsigned char byte;
unsigned char *bytes = NULL;

while (true){              // thread which is running on the side
    byte = get();          // gets 1 byte from I/O
    bytes = byte;          //
    *bytes++;
    if (byte == 'x'){      // for now instead of "\r\n" i use the char 'x'
        printf( "Your message: %s", bytes);
        bytes = NULL;        //  or {0}?
    }
}

Answer 1

Your Code

unsigned char byte;
unsigned char *bytes = NULL;

while (true){              

Nothing wrong here, but some things must be cleared:

• 1. Did you alloc memory for your bytes buffer? That is, using malloc() family functions?
• 2. If so, did you check malloc() return and made sure the pointer is ok?
• 3. Did you include stdbool.h to use true and false?

Moving on...

byte = get();         
bytes = byte;          
*bytes++;

• 4. I'm assuming get() returns an unsigned char, since you didn't give the code.
• 5. Problem: bytes = byte. You're assigning an unsigned char to an unsigned char *. That's bad because unsigned char * is expecting a memory address (aka pointer) and you're giving it a character (which translates into a really bad memory address, cause you're giving addresses up to 255, which your program isn't allowed to access), and your compiler certainly complained about that assignment...
• 6. *byte++ has two "problems" (not being really problems): one, you don't need the * (dereferencing) operator to just increment the pointer reference, you could've done byte++; two, it was shorter and easier to understand if you switched this line and the previous one (bytes = byte) to *bytes++ = byte. If you don't know what this statement does, I suggest reading up on operator precedence and assignment operators.

Then we have...

if (byte == 'x'){      
    printf( "Your message: %s", bytes);
    bytes = NULL;      
}

• 7. if's alright.
• 8. printf() is messed up because you've been incrementing your bytes pointer the whole time while you were get()ting those characters. This means that the current location pointed by bytes is the end of your string (or message). To correct this, you can do one of two things: one, have a counter on the number of bytes read and then use that to decrement the bytes pointer and get the correct address; or two, use a secondary auxiliary pointer (which I prefer, cause it's easier to understand).
• 9. bytes = NULL. If you did malloc() for your bytes buffer, here you're destroying that reference, because you're making an assignment that effectively changes the address to which the pointer points to to NULL. Anyway, what you need to clear that buffer is memset(). Read more about it in the manual.
• 10. Another subtle (but serious) problem is the end of string character, which you forgot to put in that string altogether. Without it, printf() will start printing really weired things past your message until a Segmentation Fault or the like happens. To do that, you can use your already incremented bytes pointer and do *bytes = 0x0 or *bytes = '\0'. The NULL terminating byte is used in a string so that functions know where the string ends. Without it, it would be really hard to manipulate strings.

---

Code

    unsigned char byte;
    unsigned char *bytes = NULL;
    unsigned char *bytes_aux;


    bytes = malloc(500);
    if (!bytes) return;
    bytes_aux = bytes;

    while (true) {  /* could use while(1)... */
        byte = get();
        *bytes++ = byte;
        if (byte == 'x') {
            *(bytes - 1) = 0x0;
            bytes = bytes_aux;
            printf("Your message: %s\n", bytes);
            memset(bytes, 0, 500);
        }
    }

• 11. if ((*bytes++ = get()) == 'x') is a compound version of the three byte = get(); *bytes++ = byte; if (byte == 'x'). Refer to that assignment link I told you about! This is a neat way of writing it and will make you look super cool at parties!
• 12. *(bytes - 1) = 0x0; The -1 bit is to exclude the x character which was saved in the string. With one step we exclude the x and set the NULL terminating byte.
• 13. bytes = bytes_aux; This restores bytes default state - now it correctly points to the beginning of the message.
• 14. memset(bytes, 0, 500) The function I told you about to reset your string.
• 15. Using memset is not necessary in this particular case. Every loop repetition we're saving characters from the beginning of the bytes buffer forward. Then, we set a NULL terminating byte and restore it's original position, effectively overwriting all other data. The NULL byte will take care of preventing printf() from printing whatever lies after the end of the current message. So the memset() part can be skipped and precious CPU time saved!
• 16. Somewhere when you get out of that loop (if you do), remember to free() the bytes pointer! You don't want that memory leaking...

Answer 2

You should define bytes as array with size of max message length not a pointer.

unsigned char byte, i;
unsigned char arr[10];   // 10 for example
i=0;
while (true){              
    byte = get();          
    arr[i] = byte;       
    i++;
    if (byte == 'x'){      
        printf( "Your message: %s", arr);
    }
}

When you define bytes as a pointer, it points to nothing and writing to it may erase other data in your program, you can make it array or allocate space for it in run time using malloc