Two implementations of file-reading

I work quite a lot with file reading and writing in C/Win32 and I started wondering if my code is completely ugly and how I can improve it. I wrote two simple examples of reading a file, and I wonder how these functions could be improved. Are these methods bad - and how they can be improved? I use MSVC so the code might contain MS-specific macros or keywords.

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    DWORD check = 0;
    BOOL success = FALSE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile != INVALID_HANDLE_VALUE)
    {
        if((*outsize = GetFileSize(hFile, NULL)) != INVALID_FILE_SIZE)
        {
            if((*outdata = malloc(*outsize)) != NULL)
            {
                if(ReadFile(hFile, *outdata, *outsize, &check, NULL) && check == *outsize)
                {
                    success = TRUE;
                }
            }
        }
    }

    if(hFile != INVALID_HANDLE_VALUE)
    {
        CloseHandle(hFile);
    }

    return success;
}

BOOL read_file2(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    DWORD check = 0;
    BOOL success = TRUE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile == INVALID_HANDLE_VALUE)
    {
        success = FALSE; goto _end;
    }

    if((*outsize = GetFileSize(hFile, NULL)) == INVALID_FILE_SIZE)
    {
        success = FALSE; goto _end;
    }

    if((*outdata = malloc(*outsize)) == NULL)
    {
        success = FALSE; goto _end;
    }

    if(!ReadFile(hFile, *outdata, *outsize, &check, NULL) || check != *outsize)
    {
        success = FALSE; goto _end;
    }

_end:

    if(hFile != INVALID_HANDLE_VALUE)
    {
        CloseHandle(hFile);
    }

    return success;
}

Questions:

• Which one is better, and why?

• What is bad about the above examples?

• Can you show a better example or how you wou

You could improve your first example by returning early and moving variable definitions nearer to first use. I prefer this to the use of gotos but maybe I've just been conditioned against their use for too long:

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    HANDLE h = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if(h == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    BOOL success = FALSE;
    if((*outsize = GetFileSize(h, NULL)) != INVALID_FILE_SIZE)
    {
        if((*outdata = malloc(*outsize)) != NULL)
        {
            DWORD check = 0;
            if(ReadFile(h, *outdata, *outsize, &check, NULL) && check == *outsize)
            {
                success = TRUE;
            }
            else { free(*outdata); }
        }
    }
    CloseHandle(h);
    return success;
}

Another approach is to split into two functions, thereby removing the need for nesting and gotos. The compiler can optimize away the function call (eg by inlining) if read_file_data is made static, but this inner function might possibly be useful in itself:

static BOOL read_file_data(HANDLE h, DWORD size, PBYTE *outdata)
{
    *outdata = malloc(size);
    if (*outdata == NULL) {
        return FALSE;
    }
    DWORD size_read = 0;
    if (ReadFile(h, *outdata, size, &size_read, NULL) && (size_read == size)) {
        return TRUE;
    }
    free(*outdata);
    return FALSE;
}

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    HANDLE h = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if (h == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    BOOL success = FALSE;
    *outsize = GetFileSize(h, NULL);
    if (size != INVALID_FILE_SIZE) {
        success = read_file_data(h, *outsize, outdata);
    }
    CloseHandle(hFile);
    return success;
}

You can also improve the goto method a little by returning early and changing the default success to FALSE (and freeing the buffer on error).

BOOL read_file2(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    BOOL success = FALSE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    if((*outsize = GetFileSize(hFile, NULL)) == INVALID_FILE_SIZE) {
        goto _end;
    }
    if((*outdata = malloc(*outsize)) == NULL) {
        goto _end;
    }
    DWORD check = 0;
    if(!ReadFile(hFile, *outdata, *outsize, &check, NULL) || (check != *outsize)) {
        free(*outdata);
    }
    else success = TRUE;

_end:
    CloseHandle(hFile);
    return success;
}

StackExchange Code Review Q#68995, answer score: 4