5
\$\begingroup\$

I've got a task to create a readline() function that would have the exact same behavior as read():

ssize_t read(int fd, void *buf, size_t count); (man read)

So it should return -1 on error and number of bytes read on success. Also, if the function receives a string with multiple \n chars, it should return only the first line and store the rest for a future use.

I was scrolling through this article: Reading a line ending with a newline character from a file descriptor

But I do not think the author had the same purpose - that is to work just as read()

I came up with this function:

//global buffer
char gBUF[BUFSIZ];

int readline2(int fd, char* buf){

    static char* resultBuffer = gBUF;
    int ret, mv;
    char temp[256];
    char* t;

    bzero(temp, sizeof(temp));



    t = strchr(resultBuffer,'\n');
    if(t != NULL){
        mv = t-resultBuffer+1;
        strncpy(temp,resultBuffer, mv);
        resultBuffer = resultBuffer + mv;

        temp[strlen(temp)] = '\0';
        strcpy(buf,temp);
        bzero(temp, sizeof(temp));
        ret = read(fd,&temp,256);
        temp[ret]='\0';
        strcat(resultBuffer,temp);

        return mv;
    }
    ret = read(fd,&temp,256);
    temp[ret]='\0';
    t = strchr(temp,'\n');
    mv = t-temp+1;
    strncpy(buf,temp,mv);

    strcat(resultBuffer,temp+mv);
    return ret;

}    

and I mean, it works well, although I had some real struggles with pointers and copying addresses instead of values.

My question is, how to improve this? I still have a feeling there's something missing or that something could significantly improve it.

I guess I should put a while inside and check whether the read string actually has a \n char and only return when it does, right?

I also do not like the idea of a global buffer, maybe I should just create a class/struct, something like this:

struct {
char BUF[BUFSIZ];
char *p = BUF;
}dataStruct;

to store the buffer and a pointer to it, but I do not really know how I could use that properly

Thanks for any suggestions!

Reading max 256bytes is a purpose, that should be the max;

I could've probably added that this function is to be used after in a multiprocess/multithread client server project. The server reads these lines from clients and should know what input came first, in what order the lines came, etc. The buffer should essentially work as a queue.

\$\endgroup\$
  • 1
    \$\begingroup\$ t in mv = t-temp+1; is not initialized. UB it is. What is the purpose of this line anyway? \$\endgroup\$ – vnp Apr 22 at 20:14
  • \$\begingroup\$ return only the first line and store the rest for a future use - Can you expand on this? Do you mean that the function is holding a cache and so the number of file reads will potentially be less than the number of function calls? \$\endgroup\$ – Reinderien Apr 22 at 20:33
  • \$\begingroup\$ @vnp it was actually just a mistake, the line shouldn't have been there \$\endgroup\$ – t0is Apr 22 at 20:34
  • \$\begingroup\$ @Reinderien Yes, exactly. Let's say I read a string containing two \n chars. Then only the text till the first one should be returned, the rest is stored to a buffer. When the call comes again, it returns what's inside the buffer first, before taking care of another input string. \$\endgroup\$ – t0is Apr 22 at 20:38
  • 2
    \$\begingroup\$ So why not just use fgets? \$\endgroup\$ – FoggyDay Apr 22 at 20:56
6
\$\begingroup\$

As we've basically worked out in the comments: when there's already a thing, use the thing. To get a line from a file descriptor, you can

  1. Call fdopen on your file descriptor to get a FILE*
  2. Check for NULL, perror and exit if necessary
  3. Call fgets
  4. Check for NULL again
  5. Repeat 3-4 as necessary
  6. fclose your FILE*
  7. Do not call close on your file descriptor
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ This is probably good advice in practice — but then, you’d just use the existing read function in practice. In OP’s case this probably defeats the purpose of the exercise, which is to solve this using POSIX file operations without calling into libc. \$\endgroup\$ – Konrad Rudolph Apr 23 at 8:13
  • \$\begingroup\$ Thanks @Reinderien I will try to work with this, thanks a lot for your suggestion. \$\endgroup\$ – t0is Apr 23 at 16:53
  • \$\begingroup\$ In item 2, shouldn't if necessary be as necessary? \$\endgroup\$ – pacmaninbw Apr 23 at 16:53
  • \$\begingroup\$ Uh. Not really? What issue do you see with the current wording? \$\endgroup\$ – Reinderien Apr 23 at 16:56
4
\$\begingroup\$

You've got a bit of a nightmare of lengths. You should be using explicit lengths for everything, and to heck with NUL termination.

Lets look at some cases:

    ret = read(fd,&temp,256);
    temp[ret]='\0';

Well, temp is of size 256. (And you should write sizeof(temp) instead of 256.) This means, if you read 256 bytes, you write a null into the 257th byte in the buffer, and smash memory.

    temp[strlen(temp)] = '\0';

This finds the first NUL in temp, by offset, and then overwrites it with a NUL. A useless statement. And you should instead know how many bytes you have in temp. Then use memchr instead of strchr, memcpy instead of strcpy and strcat, etc...

 int readline2(int fd, char* buf){

This is trying to reproduce the prototype of read(), but you forgot the buffer length altogether. This makes your function more comparable to gets() then fgets(). gets() is one of the large security holes in C. Don't be like gets().

Edit: One more thing. If you pass in an fd for a terminal, and it is in raw mode, you might get just a couple characters, not a complete line. This is sometimes known as a "short read". You need to keep reading until you get a line. For testing, this can be easily simulated by reading only a few bytes at a time.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your input @David Thanks for correcting me on the sizeof. I mean, i used memcpy instead of strcpy before as I was testing out the solutions, to copy the contents of temp to the actuall buffer to then be read by the function. But that copied the memory address instead of the value so once the function ended, the buffer then pointed to an empty string. That is why I used strcpy for that. You are correct that I should then specify the number of chars read, but I guess I could keep it like that since the usage of it afterwards is rather simple. Thanks a lot anyway! \$\endgroup\$ – t0is Apr 23 at 16:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.