2017-01-14 11 views
0

Мне нужно перекодировать реализацию функции getline(), но используя файловый дескриптор файла, а не FILE *. Мне разрешено использовать только malloc() и free(), а также 5 функций, максимум 25 строк длиной. Я думаю, что я сделал правильно проект, хотя я новичок в C, и мой код, вероятно, не хорош.Кодирование getline() implentation - ошибки Valgrind

Когда я запускаю его, он работает нормально, но valgrind показывает, что я definetely lost x bytes, x в зависимости от длины файла и READ_SIZE (макрос, определенный в заголовке).

Согласно --leak-check=full valgrind, у меня есть утечка памяти в функции str_realloc_cat, когда я malloc dest. Я пытался, но не мог найти, где я должен освободить/сделать что-то еще?

Здесь ниже мой код:

char *get_next_line(const int fd) 
{ 
    static char *remaining = ""; 
    char   *buffer; 
    ssize_t  cread; 
    size_t  i; 

    i = 0; 
    if (remaining == NULL) 
    return (NULL); 
    if ((buffer = malloc(SOF(char) * READ_SIZE + 1)) == NULL || 
     (cread = read(fd, buffer, READ_SIZE)) < 0) 
     return (NULL); 
    buffer[cread] = 0; 
    remaining = str_realloc_cat(remaining, buffer); 
    while (remaining[i]) 
    { 
     if (remaining[i] == 10) 
     { 
      remaining[i] = 0; 
      buffer = str_create_cpy(remaining); 
      remaining = remaining + i + 1; 
      return (buffer); 
     } 
     i++; 
    } 
    return (check_eof(fd, buffer, remaining, cread)); 
} 

char *str_realloc_cat(char *rem, char *buf) 
{ 
    size_t i; 
    size_t dest_i; 
    char *dest; 

    i = (dest_i = 0); 
    if ((dest = malloc(SOF(char) * (str_len(rem) + str_len(buf) + 1))) == NULL) 
    return (NULL); 
    while (rem[i]) 
    { 
     dest[dest_i] = rem[i]; 
     dest_i++; 
     i++; 
    } 
    i = 0; 
    while (buf[i]) 
    { 
     dest[dest_i] = buf[i]; 
     dest_i++; 
     i++; 
    } 
    dest[dest_i] = 0; 
    free(buf); 
    return (dest); 
} 

char *check_eof(const int fd, char *buffer, char *remaining, ssize_t cread) 
{ 
    if (cread == 0) 
    return (NULL); 
    if (cread < READ_SIZE) 
    { 
     buffer = remaining; 
     remaining = NULL; 
     return (buffer); 
    } 
    return (get_next_line(fd)); 
} 

char *str_create_cpy(const char *src) 
{ 
    char *dest; 
    size_t i; 

    i = 0; 
    if ((dest = malloc(sizeof(char) * str_len(src) + 1)) == NULL) 
    return (NULL); 
    while (src[i]) 
    { 
     dest[i] = src[i]; 
     i++; 
    } 
    dest[i] = 0; 
    return (dest); 
} 

int str_len(const char *str) 
{ 
    size_t i; 

    i = 0; 
    while (str[i]) 
    i++; 
    return (i); 
} 

И главный functon, если вы хотите, чтобы тест:

#define SOF(x) sizeof(x) // Why in the comments 

int main(int ac, char **av) 
{ 
    int fd; 
    char *s; 

    UNUSED(ac); 
    if (!av[1]) 
    return 1; 
    fd = open(av[1], O_RDONLY); 
    while ((s = get_next_line(fd))) 
    { 
     printf("%s\n", s); 
     free(s); 
    } 
    close(fd); 
} 

Благодаря любую помощь или советы.

+1

'SizeOf (Char)' всегда один, не загрязняет окружающую среду ваш код с ним. – DyZ

+3

Использование '#define SOF (x) sizeof (x)' - не показано, но выводится - кажется, немного бессмысленным.Он экономит 3 символа набора номера за звонок, за счет путаницы. Нехороший компромисс. –

+0

Привет, спасибо за ваши комментарии. Я знаю, что 'sizeof (char)' всегда 1, но я думал, что это была хорошая практика, поэтому вы никогда не забудете об этом, и не забывайте, что если какой-то символ имеет более 1 байт на другой системе (очень маловероятно хоть). Кроме того, SOF действительно является определением sizeof. Это уродливо, но я должен делать всего 80 символов в строке и 25 строк на каждую функцию, поэтому для меня это было любопытно, не беспокоясь о том, чтобы снова структурировать весь мой код. –

ответ

2

Ваш алгоритм плохо:

  1. Вы держите буфер в памяти выделить
  2. Вы не используете структуру, чтобы перегруппировать вашу переменную
  3. используется магическое число remaining[i] == 10
  4. Вы используете рекурсивный вы можете переполнять стек return get_next_line(fd). Неважно, я не хорошо читал, у вас есть хвост рекурсивный, просто убедитесь, что оптимизация на вашем компиляции для него.
  5. У вас есть код спагетти.
  6. т.д.

Вы должны переписать всю свою функцию с лучшей логикой первого использовать эту структуру:

#define GNL_SIZE 4096 

struct gnl_context { 
    char buffer[GNL_SIZE]; // Your buffer don't need to be dynamic 
    size_t i; // this is your index that you must use to iterate in the buffer 
    size_t read; // this is what read return 
}; 

С, что вы можете написать безопасный вариант резьбы:

char *get_next_line_r(int fd, struct gnl_context *gnl_context); 

и выполните следующие действия в своей первоначальной функции:

char *get_next_line(int fd) { 
    static struct gnl_context gnl_context; 
    return get_next_line_r(fd, &gnl_context); 
} 

А вот пример, это явно не совершенен, и я знаю, что вы не можете использовать это, но это должно помочь вам:

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

#define GNL_SIZE 4096 

struct gnl_context { 
    char buffer[GNL_SIZE]; 
    size_t i; 
    size_t read; 
}; 

char *get_next_line_r(int fd, struct gnl_context *gnl_context); 
char *get_next_line(int fd); 

static char *cpy_buffer(struct gnl_context *gnl_context, size_t i, 
         size_t *size) { 
    char *str = malloc(i - gnl_context->i + 1); 
    if (str == NULL) { 
    return NULL; 
    } 
    memcpy(str, gnl_context->buffer + gnl_context->i, i - gnl_context->i); 

    *size = i - gnl_context->i; 
    str[*size] = '\0'; 
    gnl_context->i = i; 

    return str; 
} 

static char *cat_str_and_buffer(struct gnl_context *gnl_context, char *str, 
           size_t *size, size_t i) { 
    char *ret = malloc(*size + (i - gnl_context->i) + 1); 
    if (ret == NULL) { 
    return NULL; 
    } 
    memcpy(ret, str, *size); 
    memcpy(ret + *size, gnl_context->buffer + gnl_context->i, i - gnl_context->i); 

    *size += i - gnl_context->i; 
    ret[*size] = '\0'; 
    gnl_context->i = i; 

    return ret; 
} 

static char *read_buffer(struct gnl_context *gnl_context, char *str, 
         size_t *size) { 
    size_t i = gnl_context->i; 
    while (i < gnl_context->read && gnl_context->buffer[i] != '\n') { 
    i++; 
    } 
    return str == NULL ? cpy_buffer(gnl_context, i, size) 
        : cat_str_and_buffer(gnl_context, str, size, i); 
} 

char *get_next_line_r(int fd, struct gnl_context *gnl_context) { 
    char *str = NULL; 
    size_t size = 0; 
loop: 
    if (gnl_context->i == gnl_context->read) { 
    ssize_t ret = read(fd, gnl_context->buffer, GNL_SIZE); 
    if (ret <= 0) { 
     return str; 
    } 
    gnl_context->read = (size_t)ret; 
    gnl_context->i = 0; 
    } 

    char *tmp = read_buffer(gnl_context, str, &size); 
    if (tmp == NULL) { 
    return str; 
    } 
    free(str); 
    if (gnl_context->i != gnl_context->read) { 
    gnl_context->i++; 
    return tmp; 
    } 
    str = tmp; 
    goto loop; 
} 

char *get_next_line(int fd) { 
    static struct gnl_context gnl_context; 
    return get_next_line_r(fd, &gnl_context); 
} 

int main(void) { 
    char *str; 
    while ((str = get_next_line(0)) != NULL) { 
    printf("%s\n", str); 
    free(str); 
    } 
} 
+0

Хорошо, я очень ценю усилия и критику, которые вы там вложили, но я больше беспокоился о том, чтобы исправить свой собственный алгоритм, хотя, быстро взглянув на вас, ваш явно лучше. Я кодировался на C чуть меньше 3 месяцев, поэтому, я думаю, мне еще предстоит многому научиться. Я действительно ценю это, хотя, и буду смотреть более глубоко на завтра или после того, как я покончу с этим проектом, чтобы понять лучший способ сделать это. –

1

Я обеспокоен этой линии:

remaining = remaining + i + 1; 

remaining указатель на выделенный буфер. На этой линии вы уничтожаете ее, а это значит, что вы больше не можете free().

+0

Разве это не единственный способ переместить мой указатель? Я не могу позволить себе функцию, которая бы скопировала все, начиная с 'i + 1'. –

+1

@ Greg01re Проблема с этой техникой заключается в том, что память, выделяемая для 'остатка', не может быть освобождена. Чтобы освободить его, вам нужно удержать исходный указатель. – Schwern

+0

Ну, тогда я думаю, что я застрял с оставшейся памятью, которая не может быть свободной? –

Смежные вопросы