7
\$\begingroup\$

I'm implementing a function that behaves like getline() which reads a line from file descriptor and returns the results without \n. allowed functions, read(), free() and malloc. Please review my code.

#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#ifndef GET_C_BUFF_SIZE
# define GET_C_BUFF_SIZE 1023
#endif
#ifndef BUFF_SIZE
# define BUFF_SIZE 32
#endif

void    *my_memcpy(void *dest, const void *src, size_t n)
{
    unsigned char       *n_dest;
    const unsigned char *n_src;

    n_dest = (unsigned char *)dest;
    n_src = (unsigned char *)src;
    while (n--)
        *n_dest++ = *n_src++;
    *(++n_dest) = '\0';
    return (dest);
}

void    *my_realloc(void *ptr, size_t len)
{
    void    *real;

    real = malloc(len);
    if (real)
        my_memcpy(real, ptr, len);
    free(ptr);
    return (real);
}

int my_getchar(const int fd)
{
    static char buff[GET_C_BUFF_SIZE];
    static char *chr;
    static int  pos = 0;
    static int  ret = 0;

    if (pos >= ret)
    {
        if ((ret = read(fd, buff, GET_C_BUFF_SIZE)) > 0)
        {
            chr = buff;
            pos = 0;
            return (*(chr + pos++));
        }
        else
            return (0);
    }
    else
        return (*(chr + pos++));
}

int read_line(char *text, int buf_size, char **line, const int fd)
{
    int     position;
    int     c;

    position = 0;
    while (1)
    {
        c = my_getchar(fd);
        if (c == 0 || c == '\n')
        {
            text[position] = '\0';
            *line = text;
            return (1);
        }
        else
            text[position] = c;
        position++;
        if (position >= buf_size)
        {
            buf_size += BUFF_SIZE;
            text = my_realloc(text, buf_size);
            if (!text)
                return (-1);
        }
    }
    return (1);
}

int get_line(const int fd, char **line)
{
    char    *text;
    int     buf_size;

    buf_size = BUFF_SIZE;
    text = malloc(sizeof(char) * buf_size);
    if (fd < 0 || !text || !line)
        return (-1);
    return (read_line(text, buf_size, line, fd));
}
\$\endgroup\$
3
\$\begingroup\$

Static buffer

In my_getchar(), you use a static buffer buff along with some other static variables. The problem with this is that you can no longer call get_line() on two different files at the same time. For example, if you did this:

get_line(fd1, &line1);
get_line(fd2, &line2);

Then the second get_line() would actually read characters from the first file descriptor, because you have buffered its contents in the static buffer.

Reallocation strategy

Your current reallocation strategy is to increase the size of the buffer by BUFF_SIZE when you run out of space. This causes your function to have a \$O(n^2)\$ time complexity. It would be better if you doubled the size of your buffer instead of adding a constant to it.

\$\endgroup\$
1
\$\begingroup\$
  1. Avoid using a standard library-like name memcpy() and then performing a non-intuitive functionality. mem...() functions do not do anything special about a terminating null character.

    void *my_memcpy(void *dest, const void *src, size_t n) {
      ...
      *(++n_dest) = '\0';  //  <-- ????
    }
    
  2. Bug: Insufficient memory allocation.

    // my_memcpy() affects len+1 bytes
    real = malloc(len);
    if (real)
      my_memcpy(real, ptr, len);
    
  3. () not needed.

    // return (dest);
    return dest;
    
  4. Strange default buffer size 1023. For efficient reading of files, more common to use a power-of-2. Not likely to make much difference though on systems that perform "\r\n" to "\n" translation.

    #ifndef GET_C_BUFF_SIZE
    # define GET_C_BUFF_SIZE 1023
    
  5. As mentioned @JS1, using a static buffer space is a questionable design practice. Better to pass in a pointer to a structure with the needed variables.

  6. Code can't seem to decide if it should use int, size_t for array indexing. Best to use size_t throughout. Return value from read() should be ssize_t.

  7. int my_getchar(const int fd) returns 0 to indicate end-of-file, yet also returns a char - whihc could have the value of 0. This getchar-like function should 1) return a value different from a char to indicate end-of-file. 2) best to mimic getchar() and return EOF on end-of-file or [0...UCHAR_MAX].

  8. int read_line(char *text, int buf_size, char **line, const int fd) fails to indicate when end-of-file was reached as opposed to end-of-line

    c = my_getchar(fd);
    if (c == 0 || c == '\n') {
      // add
      if (c == 0 && position == 0) return end-of_file_indication;
    
      text[position] = '\0';
      *line = text;
      return 1;
    }
    
  9. If an if() blocks ends with return, no need for else

    if (c == 0 || c == '\n') {
       text[position] = '\0';
       *line = text;
       return (1);
    }
    // else
    text[position] = c;
    
  10. Allocate to the size of the de-referenced variable, not type. It is easier to code, maintain, review and not mess up.

    // text = malloc(sizeof(char) * buf_size);
    text = malloc(sizeof *text * buf_size);
    
  11. Consider not declaring a variable until it is needed. Easier to maintain. Eliminate uninitialized variables.

    // char    *text;
    // int     buf_size;
    // buf_size = BUFF_SIZE;
    // text = malloc(sizeof(char) * buf_size);
    int buf_size = BUFF_SIZE;
    char *text = malloc(sizeof *text * buf_size);
    
  12. Unclear or wrong code. When text == NULL, it means out-of memory or the size allocated was 0. rather than use naked -1, use a macro or at least a component.

    #define ERROR_OOM -1
       ...
       text = my_realloc(text, buf_size);
       // if (!text)
       if (!text && buf_size == 0)
            // return (-1);
            return ERROR_OOM;
    
  13. Consider adding {}. Less likely to code incorrectly, easier to maintain.

       if (!text && buf_size == 0) {
            return (-1);
       }
    
  14. Memory leak. Should fd < 0 || !line, then allocated memory is lost.

    text = malloc(sizeof(char) * buf_size);
    if (fd < 0 || !text || !line)
      return (-1);
    
\$\endgroup\$

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.