Простая библиотека для формата недокументированного архива

Я пишу небольшую библиотеку для некоторых недокументированных форматов файлов. Исходная программа использует строку пути для поиска файлов в архиве, и эта функция пытается сделать то же самое.

Архив содержит волшебную строку, смещение в списке файлов, данные в файлах и сам список файлов. Запись в списке файлов состоит из пути (длиной 256 байт, дополненных нулями) и трех чисел, второе из которых представляет собой смещение от начала архива до файла записи. Записи отсортированы в алфавитном порядке с учетом регистра; вот несколько макетов, демонстрирующих порядок:

Entry/Graphics/Sprite.cgf
Entry/Sound/Ding.wav
Quit/Graphics/exit.cgf
_RAW8123

Вот соответствующие служебные функции и определения. Они определены в Utils.h и Utils.c, Я собрал их здесь для контекста.

#define BYTESZE sizeof(char)

typedef unsigned int uint;
typedef unsigned char byte;

uint readU4L(FILE* file) {
    byte buf[4];
    fread(buf, BYTESZE, 4, file);
    return buf[3] << 24 | buf[2] << 16 | buf[1] << 8 | buf[0];
}

Наконец, вот «библиотека»:

// ====== BIGfile.h
#define BIG_MAGIC_LEN 7

#define BIG_SEEK_SUCC 1
#define BIG_SEEK_FAIL 0

typedef struct BIGFile {
    char magic[BIG_MAGIC_LEN];
    uint offset;
    FILE* read;

} BIGFile;

BIGFile* load_BIGFile(char* fpath);

int seekToEntry(BIGFile* bigfile, char* path);

// ====== BIGFile.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "Utils.h"
#include "BIGFile.h"

BIGFile* load_BIGFile(char* fpath) {
    BIGFile* bigfile = malloc(sizeof(BIGFile));

    fopen_s(&(bigfile->read), fpath, "rb");

    if (!bigfile->read) {
        printf("Can't open file.n");
        return 0;
    }

    fread(bigfile->magic, BYTESZE, BIG_MAGIC_LEN, bigfile->read);
    bigfile->offset = readU4L(bigfile->read);
    return bigfile;
}

int seekToEntry(BIGFile* bigfile, char* path) {

    fseek(bigfile->read, bigfile->offset, SEEK_SET);
    
    byte buf[256];

    int pos = 0;

    // while not EOF, read string part
    while (fread(buf, BYTESZE, 256, bigfile->read)) {
        
        // seek over entry data
        fseek(bigfile->read, 12, SEEK_CUR);
        
        // loop until given path != read path
        while (path[pos]==buf[pos]) {
            pos++;

            // path has ended, so buf == path
            if (!path[pos]) {

                // seek back into entry data
                fseek(bigfile->read, -8, SEEK_CUR);

                // read unsigned little endian 4-byte int
                uint res = readU4L(bigfile->read);

                // seek there and return success
                fseek(bigfile->read, res, SEEK_SET);
                //printf("found at %dn", res);
                return BIG_SEEK_SUCC;
            }
        }
        // path char is smaller than buf char
        // we seeked past where the entry could bem return failure
        if (path[pos]<buf[pos]) {
            return BIG_SEEK_FAIL;
        }
    }
    // EOF, return failure
    return BIG_SEEK_FAIL;
}

Я хотел бы получить отзывы об общем стиле, а также о seekToEntry функция, возможно, это можно как-то улучшить? Заранее спасибо!

2 ответа
2

В заголовке нет охранников. Вы захотите исправить это немедленно.

#define BYTESZE sizeof(char)

Это довольно бессмысленное определение, поскольку sizeof дает количество char необходимо для его аргумента, что означает sizeof (char) равен 1 по определению.

uint readU4L(FILE* file) {

FILE не определено — похоже, вам не хватает включения <stdio.h> из этого заголовка.

    fread(buf, BYTESZE, 4, file);

Почему мы игнорируем возвращаемое значение из fread()? Если это не удастся, у нас будут унифицированные значения в buf, что означает, что наша программа имеет неопределенное поведение.

    return buf[3] << 24 | buf[2] << 16 | buf[1] << 8 | buf[0];

Здесь есть тонкая проблема с преобразованием типов. В unsigned char ценности из buf получить повышение до int для арифметики, но затем преобразован в unsigned int для возвращаемого значения. Я думаю, что мы действительно хотим рассматривать их как беззнаковые значения повсюду, поэтому мы должны преобразовать их в unsigned int перед арифметикой.

Отметим также, что << 24 может быть вне допустимого диапазона int. Возможно, рассмотрите возможность использования uint32_t за это значение? (Включают <stdint.h> чтобы определить это).

int seekToEntry(BIGFile* bigfile, char* path);

Нужно ли нам писать на *path? Судя по названию, это должен быть вход, который передается как const char*.

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

#include "Utils.h"
#include "BIGFile.h"

Лучше сделать "BIGFile.h" первое, что вы включаете из BIGFile.c — это помогает выявлять проблемы, такие как отсутствующее включение, отмеченное ранее.

<string.h> включен, но не нужен.

 BIGFile* bigfile = malloc(sizeof(BIGFile));

Что нам делать, если bigfile это нулевой указатель? Кажется, мы продолжаем и используем его в любом случае (подробнее Undefined Behavior).

Хорошая привычка — применять sizeof к месту назначения, а не к его типу; это помогает читателям, когда до объявления далеко. Например, в следующем случае это гораздо более очевидно правильное совпадение:

bigfile = malloc(sizeof *bigfile);
fopen_s(&(bigfile->read), fpath, "rb");

В чем смысл использования Приложения К fopen_s() функция, если мы просто проигнорируем возвращаемое значение? Кроме того, я не вижу, где мы это проверяем. __STDC_LIB_EXT1__ определено и где мы положили __STDC_WANT_LIB_EXT1__ чтобы обеспечить наличие функции. Кажется, нам лучше использовать старый добрый fopen() и проверка нулевого указателя обычным способом.

if (!bigfile->read) {
    printf("Can't open file.n");
    return 0;
}

Наконец, мы видим некоторую проверку ошибок. Здесь есть несколько ошибок:

  1. Мы пишем stdout вместо stderr.
  2. Мы пренебрегли free() память, которую мы (возможно) выделили malloc(), что приводит к утечке памяти.
fread(bigfile->magic, BYTESZE, BIG_MAGIC_LEN, bigfile->read);

Опять же, здесь нет проверки, что мы успешно прочитали столько, сколько хотели. И разве мы не должны проверять, соответствует ли магия файла ожидаемому значению, чтобы мы не пытались анализировать файлы не в нашем ожидаемом формате?

fseek(bigfile->read, bigfile->offset, SEEK_SET);

fseek() тоже может потерпеть неудачу, вы знаете — на данном этапе нечего сказать, например, что мы не читаем из канала.

byte buf[256];

while (fread(buf, BYTESZE, 256, bigfile->read)) {

Вместо того, чтобы писать это магическое число дважды (и рисковать пропустить одно при его изменении), мы можем автоматически использовать доступный размер: fread(buf, 1, sizeof buf, bigfile->read). Опять же, мы игнорируем жизненно важное возвращаемое значение.

    while (path[pos]==buf[pos]) {
        pos++;

Похоже, мы делаем что-то strcmp() вот — почему бы не воспользоваться стандартной функцией? Это определенно выглядит неправильно, что мы не инициализируем pos до 0, прежде чем начинать это сравнение — если это действительно так, следует дать комментарий, чтобы предупредить читателей.

    В целом ваш стиль кодирования довольно чистый, но вот несколько предложений по улучшению, которые сделают код более надежным и переносимым, а также более безопасным:

    Используйте целые числа фиксированного размера

    В C, sizeof(char) по стандарту гарантированно равен 1, но sizeof(unsigned int) не гарантируется, что будет 4.

    К счастью, C99 представляет целые числа фиксированного размера Такие как uint32_t который гарантированно будет 32-битным (4 байта), определенным в stdint.h.
    Поскольку вы используете нестандартные fopen_s Я предполагаю, что от вас не требуется придерживаться ANSI C.

    Добавить проверку ошибок

    В опубликованном вами коде вы никогда не проверяете возвращаемое значение операций чтения файла или операций выделения памяти.

    Вы также никогда не проверяете достоверность прочитанных данных.

    Для библиотеки важна надежная обработка ошибок, поэтому вы всегда должны проверять, успешны ли операции чтения, и если данные, такие как размеры записей или смещения, разумны — соответствовать ожидаемому размеру / не выходить за пределы конца файла.

    В случае ошибки вы должны вернуть один из нескольких кодов ошибок, которые вы определили для своей библиотеки и перечислили в документации.

    Уменьшите количество операций с диском

    Вы, вероятно, не почувствуете этого при тестировании, как из-за современного оборудования, небольшого размера файла и нескольких кешей на разных уровнях, но ваш seekToEntry функция неэффективна из-за множества операций с диском вперед и назад (как поиск, так и чтение).

    Здесь есть два подхода, которые вы можете выбрать в зависимости от того, как вы хотите обменять память на скорость и как будет использоваться ваша библиотека:

    1. Предварительно загрузите весь «каталог» файловых записей в массив структур в памяти и выполните поиск, когда вам нужно найти позицию записи в файле.
      Поскольку ваши записи гарантированно будут отсортированы, вы можете реализовать более эффективный поиск, возможно, расположить их в дереве по префиксу.

    2. Поскольку каждая запись имеет фиксированный размер, считайте по крайней мере текущую запись полностью в структуре.
      Если вы не хотите загружать их все в память заранее, по крайней мере, загрузите все данные (имя, размер, смещение) для каждой записи по мере выполнения итерации и сохраните себе кучу seek операции.

    Любой из подходов также даст вам возможность создать документированную структуру для «входа» в этот архивный файл, которая будет полезна вам и пользователям вашей библиотеки.

    Создайте пространство имен

    Поскольку это библиотека, ее можно использовать в большом проекте с другими библиотеками или множеством функций, определенных в самом проекте.

    Вы хотите избежать конфликта имен для ваших функций, структур и определений.

    Хотя C не поддерживает специальный синтаксис для пространств имен, одним из соглашений для библиотек является использование фиксированного и согласованного префикса для имен, за которым следует символ подчеркивания.

    Если ваша библиотека предназначена для работы с «Большим файловым архивом», вы можете поставить перед всеми своими именами префикс BFA_, например: BFA_loadFile, BFA_seekToEntry.

    Избегайте нестандартных функций

    Хотя MS может требовать fopen_s более безопасен, он также нестандартен, поэтому, если вы или один из пользователей вашей библиотеки когда-либо переключите компилятор или ОС, ваша библиотека больше не будет работать.

    Вы должны использовать стандарт C fopen вместо этого просто добавьте надлежащую проверку ошибок, как указано выше.

    Также нет причин для повторной реализации strncmp как и с петлями.
    В отличие от strcmp что небезопасно, если не гарантируется, что строка завершается нулем, strncmp совершенно безопасен для вашего случая использования, когда максимальная длина известна заранее.

    Используйте последовательный интерфейс

    Трудно сказать только о двух функциях, но похоже, что у вас есть некоторая разница в том, как работают функции вашей библиотеки:

    Один возвращает «объект» ( BIGFile структура), другой вместо этого возвращает коды ошибок / успеха.

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

    Ваша открытая функция может выглядеть так:

    /**
     * Opens an archive file and loads its header
     *
     * @param fpath   path to the archive file
     * @param header  place to return loaded header data if the file is successfully loaded (structure will be allocated by the function)
     * @return success / error status code
     */
    int BFA_loadFile(char *fpath, BFA_FileHeader **header) {
        ...
    }
    

    Здесь я также включил один общий метод документирования функций в библиотеках, который можно использовать с автоматическими инструментами, такими как доксиген для создания файлов документации в HTML и других форматах.

      Добавить комментарий

      Ваш адрес email не будет опубликован. Обязательные поля помечены *