6
\$\begingroup\$

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:

  1. Which one is better, and why?
  2. What is bad about the above examples?
  3. Can you show a better example or how you would do it?
\$\endgroup\$
2
  • 1
    \$\begingroup\$ This is a great example of why to use C++, you can make the compiler worry about making sure that cleanup happens. This code is a perfect example of C's main shortcoming: the programmer has to remember everything and the language gives him zero help in that regard. \$\endgroup\$ – Doug Gale Nov 5 '14 at 18:33
  • \$\begingroup\$ Yes I've been told many times to switch to C++. I tried it but didn't like the fact that the language is so big. I would like to master C before starting C++. Also, quite funny thing is that I see many times how 'C++' coders code C with *.cpp extension, and uses only a little bit of C++. Btw about that cleanup: GCC supports cleanup-attribute, which helps C coders to free acquired resources(RAII?) easily. Unfortunetaly it's not supported by MSVC, which many people use for C coding in Windows. OK, enough offtopic. Would be great if people could answer the question and share their methods. :) \$\endgroup\$ – Okkaaj Nov 5 '14 at 19:29
2
\$\begingroup\$

While there's nothing inherently wrong with either approach, your code does not follow usual Windows conventions for C APIs. Windows APIs will almost never allocate a buffer that the caller must free; those that do will almost always do so with a custom cleanup function. Secondly, by using ANSI characters and APIs instead of Wide characters and APIs, you limit the sets of files you can work with.

Now there are certainly tradeoffs in the question of whether caller or callee allocates the buffer. When the callee allocates it, there's little chance of buffer overruns. However there's an increased chance of either a memory leak or attempting to free the memory with the wrong function, although static analysis can help with either of these concerns. This concerns are especially relevant at library interfaces.

There's even technically a tradeoff on the ANSI vs Wide character question, but unless you're trying to support Windows 9x systems (now all firmly outdated), the fact that the Wide APIs do not work on those systems should no longer be relevant. If you don't know whether you need to support such cases, it's possible to use 'T' character typedefs and API macros that are dependent on the definition of UNICODE.

Finally there's a small concern that you're excluding possible return values from GetFileSize. This is probably okay, because if a file exceeds 4GB, you are less likely to be able to allocate the buffer and read it into memory anyway. But it's worth commenting that this is intentional so that the function can document its limits.

To sum up, I would suggest changing most of the above to improve your code. Add comments. Use the Unicode types and APIs. Especially if you are writing a library, have the caller allocate the buffer. Consider supporting static analysis (_In_, _Out_, etc.). And consider checking the high half of GetFileSize's result:

BOOL ReadFile_3(_In_ LPCWSTR szFilename, _Out_ LPBYTE lpBuffer, _In_ cbBuffer)
{
    : : :
    HANDLE hFile = CreateFileW(szFileName, ...);
    : : :
    LARGE_INTEGER liFileSize;
    liFileSize.LowPart = GetFileSize(hFile, &liFileSize.HighPart);
    if (liFileSize.HighPart > 0 || liFileSize.LowPart > cbBuffer)
        success = FALSE;
    : : :
}

Or, if your code must support Windows 9x, but you can compile it twice to also better support Windows NT:

BOOL ReadFile_4(_In_ LPCTSTR szFileName, ...) // LPCSTR or LPCWSTR depending on UNICODE
{
    : : :
    HANDLE hFile = CreateFile(szFileName, ...); // CreateFileA or CreateFileW similarly
    : : :
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the answer. I indeed should use UNICODE instead, doesn't functionA call functionW anyways? Also that GetFileSize checking is useful. I haven't actually never used it. \$\endgroup\$ – Okkaaj Nov 7 '14 at 18:48
  • \$\begingroup\$ @Okkaaj While functionA does call functionW, the conversion means it can then only handle strings valid on the current code page. This prevents mixing Cyrillic and CJK characters in a filename, for instance. \$\endgroup\$ – Michael Urman Nov 7 '14 at 19:12
4
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ Never actually thought of splitting it into two functions. Interesting way, but when working with other people, should be commented of course. Also about that variable locations, I thought about that too. After I wrote the code, I realized that it looks like it was written for some C89-only compiler. I've also heard many, many times that using codegotocode is bad - but I don't think so anymore. Sure, if it's used too much and/or in wrong places, it makes the code horrible. However, for error-handling like this, I think it fits perfectly :) I just wish that MSVC would support cleanup attr. \$\endgroup\$ – Okkaaj Nov 6 '14 at 7:17
  • \$\begingroup\$ I added an extra bit improving on your goto method. Note that it is not uncommon for company coding standards to forbid goto statements and even early returns. It is also not uncommon for coding standards to be ignored :-) If you want to use gcc on Windows you could install the cygwin environment. There's also another simpler environment for Windows the name of which escapes me just now... \$\endgroup\$ – William Morris Nov 6 '14 at 13:42
2
\$\begingroup\$
  • You (intentionally?) don't deallocate the value from malloc in your error path.
  • I'm always for read_file2. You have multiple points of failure, so you can properly organise the error handling in the same fashion, step by step. Oh yeah, and less nesting is a good thing IMO. Normally I'd expect multiple labels though, since in the failure case you need to reverse all the steps which happened before.

Why switch the return code handling in read_file2? I'd say

BOOL success = FALSE;
...
if (error) goto _end;
...
success = TRUE;
...
_end: ...

removes potential coding errors.

In general, why not take a look at some C code, or e.g. the Linux kernel coding style, the problem isn't new, although you can find both styles.

Edit: For multiple labels, I mean something like:

if (allocation1 fails)
  goto deallocate1;
if (allocation2 fails)
  goto deallocate2;
return result;
deallocate2: ...
deallocate1: ...
error:
  return error_code;

So you do your steps one by one, then, if any of those fail, you jump into the first label and undo everything in the reverse order.

\$\endgroup\$
1
  • \$\begingroup\$ Hello. About switching the return code handling, I don't actually know. I could've just used the same style, and only change it to true - if the last statement succeed. Also yes, the allocation is supposed to happen in the calling function(main). But can you show an example of using multiple labels (like how it helps?)? I usually just use debugging macros for showing GetLastError/Function/Line/etc when the error happens. \$\endgroup\$ – Okkaaj Nov 6 '14 at 7:10

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.