4
\$\begingroup\$

The purpose of the ChunkBuffer code below is to designate a "chunk" from a given input file and to loop that chunk (if needed).

Its operation is similar to the following pseudocode, with the important difference that the real code uses a heap-allocated buffer for performance:

chunk c;

chunk_init(&c, input_file, begin_offset, size_of_chunk);

while (/* still need to read bytes */)
{
    byte b = chunk_next_byte(&c); // may loop to `begin_offset`
}

The ChunkBuffer code is non-portable and meant for use in Windows programs, and was written in a style close to that of the Windows API.

My main goals regarding this code review:

  1. Find coding mistakes (incorrect buffering, memory leaks, etc.)
  2. Enhance the Windows API style of the code.
// enable Unicode support within the Windows API
#define UNICODE
#define _UNICODE

// skip the inclusion of certain internal headers
#define WIN32_LEAN_AND_MEAN

#include <windows.h>

typedef struct
{
    BOOL            fError;             ///< ChunkBuffer's error flag.
    HANDLE          hHeap;              ///< Handle of the heap to allocate memory from.
    HANDLE          hFile;              ///< Handle of the file to be read.
    LPBYTE          lpbBuffer;          ///< Buffer of bytes read.
    LARGE_INTEGER   liOffset;           ///< User-specified start offset in the file where the chunk begins.
    LARGE_INTEGER   liLength;           ///< User-specified chunk size in bytes.
    SIZE_T          cbBufferSize;       ///< User-specified size for the internal byte buffer.
    SIZE_T          index;              ///< Index of the current byte.
    SIZE_T          cbAvailable;        ///< Bytes available in the internal byte buffer.
    LARGE_INTEGER   liRemaining;        ///< Bytes remaining in the chunk.
} CHUNKBUFFER, *PCHUNKBUFFER;

typedef const CHUNKBUFFER * PCCHUNKBUFFER;

void ChunkBuffer_Init(
    PCHUNKBUFFER pCB,
    HANDLE hHeap, HANDLE hFile,
    LARGE_INTEGER liOffset,
    LARGE_INTEGER liLength,
    SIZE_T cbBufferSize);

void ChunkBuffer_Free(PCHUNKBUFFER pCB);
BOOL ChunkBuffer_Error(PCCHUNKBUFFER pCB);
BYTE ChunkBuffer_NextByte(PCHUNKBUFFER pCB);

///
/// @brief Initializes a ChunkBuffer.
/// @param [out] pCB        ChunkBuffer to initialize.
/// @param hHeap            Handle of the heap to allocate memory from.
/// @param hFile            Handle of the file to read data from.
/// @param liOffset         User-specified start offset in the file where the chunk begins.
/// @param liLength         User-specified chunk size in bytes.
/// @param cbBufferSize     User-specified size for the internal byte buffer.
///
void ChunkBuffer_Init(
    PCHUNKBUFFER pCB,
    HANDLE hHeap,
    HANDLE hFile,
    LARGE_INTEGER liOffset,
    LARGE_INTEGER liLength,
    SIZE_T cbBufferSize)
{
    pCB->hHeap          = hHeap;
    pCB->hFile          = hFile;
    pCB->liOffset       = liOffset;
    pCB->liLength       = liLength;
    pCB->cbBufferSize   = cbBufferSize;
    pCB->fError         = FALSE;
    pCB->lpbBuffer      = NULL;
    pCB->index          = 0;
    pCB->cbAvailable    = 0;
    pCB->liRemaining    = liLength;
}

///
/// @brief Frees the resources allocated by a ChunkBuffer.
/// @note This function does not close the handle of the file
///  associated with the ChunkBuffer; this is on purpose.
/// @param [in,out] pCB     ChunkBuffer to deinitialize.
///
void ChunkBuffer_Free(PCHUNKBUFFER pCB)
{
    if (pCB->lpbBuffer != NULL)
        HeapFree(pCB->hHeap, 0, pCB->lpbBuffer);

    pCB->hHeap      = INVALID_HANDLE_VALUE;
    pCB->hFile      = INVALID_HANDLE_VALUE;
    pCB->lpbBuffer  = NULL;
}

///
/// @brief Returns the ChunkBuffer's error flag.
/// @param [in] pCB         ChunkBuffer to inspect.
/// @returns Whether or not the ChunkBuffer is in an error state.
/// @retval TRUE            An error occurred.
/// @retval FALSE           No errors occurred.
///
BOOL ChunkBuffer_Error(PCCHUNKBUFFER pCB)
{
    return pCB->fError;
}

///
/// @brief Retrieves the next byte in the ChunkBuffer.
/// @param [in,out] pCB     ChunkBuffer to read.
/// @returns The next byte.
///
BYTE ChunkBuffer_NextByte(PCHUNKBUFFER pCB)
{
#define SET_FERROR_IF(x)    if (x)  {   \
    pCB->fError = TRUE;                 \
    return 0;                           \
} else (void)0

    if (pCB->fError) // the ChunkBuffer is in an error state
        return 0;

    // allocate memory for the internal byte buffer, if needed;
    // and if so, then also read data from the file
    if (pCB->lpbBuffer == NULL)
    {
        const LPBYTE    lpbTemp     = HeapAlloc(pCB->hHeap, 0, pCB->cbBufferSize);
        const SIZE_T    cbMinChunk  = (SIZE_T)min((LONGLONG)pCB->cbBufferSize, pCB->liLength.QuadPart);

        SET_FERROR_IF(lpbTemp == NULL);
        pCB->lpbBuffer = lpbTemp;
        SET_FERROR_IF(!SetFilePointerEx(pCB->hFile, pCB->liOffset, NULL, FILE_BEGIN));
        SET_FERROR_IF(!ReadFile(pCB->hFile, pCB->lpbBuffer, cbMinChunk, &pCB->cbAvailable, NULL));
        pCB->liRemaining.QuadPart -= pCB->cbAvailable;
    }

    if (pCB->index == pCB->cbAvailable) // may need to read from the file again
    {
        // only read new data if the user-specified chunk length is greater than
        // that of the internal buffer; otherwise all data is already available
        if ((LONGLONG)pCB->cbBufferSize < pCB->liLength.QuadPart)
        {
            if (pCB->liRemaining.QuadPart == 0) // reached the end of the user-specified chunk
            {
                SET_FERROR_IF(!SetFilePointerEx(pCB->hFile, pCB->liOffset, NULL, FILE_BEGIN));
                pCB->liRemaining = pCB->liLength;
            }

            const SIZE_T cbMinSize = (SIZE_T)min((LONGLONG)pCB->cbBufferSize, pCB->liRemaining.QuadPart);

            SET_FERROR_IF(!ReadFile(pCB->hFile, pCB->lpbBuffer, cbMinSize, &pCB->cbAvailable, NULL));

            if (pCB->cbAvailable == 0) // end of key file was reached
            {
                SET_FERROR_IF(!SetFilePointerEx(pCB->hFile, pCB->liOffset, NULL, FILE_BEGIN));
                SET_FERROR_IF(!ReadFile(pCB->hFile, pCB->lpbBuffer, cbMinSize, &pCB->cbAvailable, NULL));
            }

            pCB->liRemaining.QuadPart -= pCB->cbAvailable;
        }

        pCB->index = 0;
    }

    return pCB->lpbBuffer[pCB->index++];
#undef SET_FERROR_IF
}

//
// Basic usage preparations
//

#if 0

    CHUNKBUFFER     cb;
    HANDLE          heap        = GetProcessHeap();
    HANDLE          input_file  = CreateFile(/* ... */);
    LARGE_INTEGER   begin;      // ...
    LARGE_INTEGER   size;       // ...

    #define BUFFER_SIZE     4096

    ChunkBuffer_Init(&cb, heap, input_file, begin, size, BUFFER_SIZE);
    BYTE b = ChunkBuffer_NextByte(&cb);
    ChunkBuffer_Free(&cb);

#endif
\$\endgroup\$
4
\$\begingroup\$

This is a good exercise! I've actually had to implement buffering like this before, and I believe that most implementations of the standard library do something similar for file reading and writing by default. Below are my thoughts on how to improve it.

Naming

As always, naming is one of the most important ways you can make code clearer. For the most part your names are pretty good, but one that had me scratching my head a bit was lilength. What is it the length of? It appears to be the length of the chunk the caller wants to read from. If that's the case, I'd name it liChunkLength, and maybe even liNumChunkBytes to make it clear what the units are.

Likewise, I don't like the name of index. First, it doesn't have the cb prefix that the other SIZE_T variables do. Second, is it an index into the file, the chunk, or the buffer? In this case, it's an index into the buffer, so maybe call it cbBufferIndex?

Handling Errors

It seems really odd to me that the errors are stored in the structure instead of being returned by the functions. Are there other Windows functions that work that way? I would not generally do it that way.

And why does the caller need to call a function to determine whether there's an error when it's just a field in their struct? If I were a developer using this code, I would just look at the struct and not call a function.

Why is the error a BOOL? That makes it much harder for a caller to know what went wrong. Looking at the code I see several different classes of errors that are possible: not able to allocate memory, not able to set the file pointer, not able to read from the file, etc.

I would make an enum for the errors and return an error from every function in which an error can occur, and I'd remove the fError field from the struct. maybe something like this:

typedef enum CB_Error {
    CBE_OUT_OF_MEMORY = 100,
    CBE_INVALID_FILE_HANDLE = 101,
    CBE_UNABLE_TO_SET_FILE_POINTER = 102,
    // ...etc.
} CB_Error;

Macros

In general you should avoid macros for anything other than simple named constants. (And with modern C you should use const instead for that.) See here for just one simple example of why macros are problematic.

But if you are going to use macros, it's traditional to #define them at the top of the source rather than inside a function. Are you likely to have another macro with the same name later in the file that you need to #define it in the function and then #undef it in the same function? If so, that's a problem.

Resource Allocation Is Initialization

There's a helpful pattern called RAII. It stands for "Resource Acquisition Is Initialization.". What it means is that you should allocate any resources you need to use a structure or object when you initialize it. In your case, I'd apply it to ChunkBuffer_Init(). I would allocate the memory for pCB->lpbBuffer in ChunkBuffer_Init() instead of in ChunkBuffer_NextByte(). It seems silly that you can successfully initialize a chunk buffer only to fail to get the first byte because there was no memory to allocate the buffer. Moving the allocation will make ChunkBuffer_NextByte() cleaner and smaller.

Performance

Reading a single byte at a time from a buffer is very inefficient and painful to use. As a caller, I'd much rather pass in a buffer of some length and have it fill the entire buffer at once, rather than having to read a single byte at a time. Think about the overhead of checking to see if you've reached the end of the chunk buffer. You have to do that check on every single byte that will be read from the file. If a caller can pass in a buffer that's 100 bytes, or 1000 bytes, those checks will happen 1/100th or 1/1000th as often, meaning better performance!

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review; and let me know if you find any coding (logic) mistakes in ChunkBuffer_NextByte(). Though I have tested it without finding anything wrong, I still feel paranoid about that messy code. \$\endgroup\$ – user7023624 Jan 14 '17 at 16:56
  • \$\begingroup\$ "Are there other Windows functions that work that way?" No, Windows functions will either: (1) return a BOOL indicating success or failure, and, in certain cases (per the documentation), you can call GetLastError to get the actual error code in the event of failure, or (2) return an HRESULT value that is basically an error code or 0 to indicate success. Virtually all of the APIs designed in this century return HRESULT values. Instead of defining your own error codes, I would use one of the predefined HRESULT values. There a bunch of 'em, you should find what you want. \$\endgroup\$ – Cody Gray Jan 15 '17 at 17:54
3
\$\begingroup\$

Bug

This line is not correct:

    // only read new data if the user-specified chunk length is greater than
    // that of the internal buffer; otherwise all data is already available
    if ((LONGLONG)pCB->cbBufferSize < pCB->liLength.QuadPart)

You are assuming here that if the buffer size is larger then the length requested, that you will already have read the full amount. But if the initial ReadFile() call returned less bytes than requested (i.e. cbAvailable was less than liLength.QuadPart), then you will never read the rest of the file. I believe you can just remove this if statement, because your later checks on cbRemaining are enough to deal with all situations.

\$\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.