File stream functions

Question

Is this good or not? Input arguments for OsFile_Open must be in UTF8 format.

#include <stdio.h>

#ifdef WIN32
    #include <Windows.h>
#endif

// Types declaration, originally they are located in header file
//
typedef int                 Bool_T;
typedef signed long long    Int64_T;

#define true    1
#define false   0

#define IN
#define OUT


Bool_T OsFile_Open( IN const char * name, IN const char * mode, OUT FILE ** file )
{
    FILE * handle;    

#ifdef WIN32

    wchar_t unicodeMode[MAX_PATH];
    wchar_t unicodeName[MAX_PATH];

    int unicodeNameLen;
    int unicodeModeLen;

    unicodeNameLen = MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, 0 );

    if( unicodeNameLen != 0 )
    {
        if( MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, unicodeNameLen ) != 0 )
        {
            unicodeModeLen = MultiByteToWideChar( CP_UTF8, 0, mode, -1, unicodeMode, 0 );

            if( unicodeModeLen != 0 )
            {
                if( MultiByteToWideChar( CP_UTF8, 0, mode, -1, unicodeMode, unicodeModeLen ) != 0 )
                {
                    handle = _wfopen( unicodeName, unicodeMode );

                    if( handle != NULL )
                    {
                        *file = handle;
                        return true;

                    }else{

                        return false;
                    }

                }else{

                    return false;
                }

            }else{ // if( unicodeModeLen != 0 )

                return false;
            }

        }else{ // if( MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, unicodeNameLen ) != 0 )

            return false;
        }     

    }else{ // if( unicodeNameLen != 0 )

        return false;
    }

#else

    handle = fopen64( name, mode );

    if( handle != NULL )
    {
        *file = handle;
        return true;

    }else{

        return false;
    }

#endif

}


Bool_T OsFile_Close( IN FILE * file )
{
    return ( fclose( file ) == 0 ) ? true : false;
}


Bool_T OsFile_Seek( IN FILE * file, IN Int64_T offset, IN int origin )
{
    int result;

#ifdef WIN32
    result = _fseeki64( file, offset, origin );
#else
    result =  fseeko64( file, offset, origin );
#endif

    return ( result == 0 ) ? true : false;
}


Bool_T OsFile_GetPos( IN FILE * file, OUT Int64_T * curPos )
{
#ifdef WIN32
    *curPos = _ftelli64( file );
#else
    *curPos =  ftello64( file );
#endif 

    return ( *curPos != -1 ) ? true : false;
}


size_t OsFile_Read( IN FILE * file, IN int numOfBytes, OUT unsigned char * buffer )
{
    return fread( buffer, sizeof( unsigned char ), numOfBytes, file );
}


int OsFile_Eof( IN FILE * file )
{
    return feof( file );
}

Answer 1

I'd re-structure (the Win32 part of) the open function to something like this:

#ifdef WIN32

    wchar_t unicodeMode[MAX_PATH];
    wchar_t unicodeName[MAX_PATH];

    if (MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, MAX_PATH) && 
        MultiByteToWideChar( CP_UTF8, 0, mode, -1, unicodeMode, MAX_PATH) && 
        (handle = _wfopen( unicodeName, unicodeMode)) != NULL) 
    {
        *file = handle;
        return true;
    }
    return false;

#else

You don't need to call MultiByteToWideChar twice for each conversion -- the first call not only returns the length, but does the conversion as well. As it stands, your code has potential buffer overruns as well -- it used the first call to MultiByteToWideChar to measure the length that would be needed for the result, but never checked that this was less than or equal to the buffer space you provided.

It can be legitimate to do the two consecutive calls, especially if (for example) you want to ensure you can handle any possible input by using the first call the measure the necessary length, then allocating the necessary space dynamically, and then doing the second call using the dynamically allocated space.

Several places, you have things like:

return ( fclose( file ) == 0 ) ? true : false;

These should be written as:

return fclose(file) == 0;

or just:

return !fclose(file);

There's no need to select and return a Boolean literal -- the Boolean values yielded by == and ! will work nicely as-is.

Edit: You also have a couple of things like:

#ifdef WIN32
    *curPos = _ftelli64( file );
#else
    *curPos =  ftello64( file );
#endif 

spread throughout the code. Where you're just dealing with different function names like this, I think I'd have one block up-front to deal with the name differences, and then use a single name through the rest of the code:

#ifdef WIN32
    #define ftello64(x) _ftelli64(x)
    #define fseeko64(x) _fseeki64(x)
#endif

/* ... */

Bool_T OsFile_Seek( IN FILE * file, IN Int64_T offset, IN int origin )
{
    return !fseeko64(file, offset, origin);
}

You might want to read #ifdef considered harmful for more about what to do and what to avoid.

Answer 2

Consider replacing a code structure like this:

if (predicate)
{
    // many lines of code here
    if (predicate2)
    {
        // even more lines of code here
    }
    else
    {
         return false;
    }
}
else
{
     return false;
}

to the much simpler:

if (!predicate)
    return false;

// long code here

if (!predicate2)
    return false

// second long paragraph