4
\$\begingroup\$

Prototype:

char *fgetline_alloc( FILE *fp, bool flush, int *nchars, bool eolend );

I realize this C99 function may be a bit more involved than expected, hence asking for reviews before using it for real (it's tested, but not extensively). I'll post the source code first, followed by detailed description (which I removed from the source code). I am leaving it in (simplified) debug mode.

I'm also providing quite a few sample-use examples, at the end of the post.

Briefly, the general idea is to return the input as a dynamically allocated c-string (nul-terminated buffer of chars). flush may be set to true for interactive input validation in a loop, from stdin. eolend specifies whether the result will end with EOL or not. nchars specifies either the max count of chars to be read (positive value), or an initial alloc-ahead size in chars (0 or negative value). It also passes back the count of successfully read chars. For details, please see the Details section below the source code.

If cleared, I intend to also make a wchar_t version of it.

Besides anything else you spot, I'd be interested in your opinions about:

  • guarding against passed-in overflow of *nchars (is it possible? if so is it worth it?)
  • my approach of preventing int overflow for the params of malloc() and realloc()
  • realloc()ing the resulting buf to its exact size at the end (I currently don't)

Thanks in advance to anyone taking the time to review this.

Source Code

/* tab space: 4 (regular, not spaces) */

#include <stdio.h>      // for debug messages
#include <stdlib.h>
#include <stdbool.h>    // C99 - bool, true, false
#include <limits.h>     // C99 - for INT_MAX, etc
#include <errno.h>

#define MYDEBUG 1
#if MYDEBUG
    #define DBGMSG( ... )\
    do{\
        fprintf(stderr, "*** %s(): ", __func__ );\
        fprintf( stderr, __VA_ARGS__ );\
    }while(0)
#endif

// -----------------------------------------------
// Read a line from an already open text-stream, and Return it as a dynamically
// allocated c-string. Return NULL on failure (errno is also set). The caller
// is responsible for freeing the returned c-string.
// NOTE: Description details removed for brevity.
//
char *fgetline_alloc( FILE *fp, bool flush, int *nchars, bool eolend )
{
    errno = 0;

    // sanity checks and early exits
    if ( !fp ) {
        DBGMSG( "invalid stream\n" );
        errno = EDOM;
        goto fail;
    }
    if ( ferror(fp) ) {
        DBGMSG( "stream error\n" );
        goto fail;
    }
    if ( feof(fp) ) {
        DBGMSG( "already at EOF\n" );
        goto fail;
    }

    // This can't handle passed-in wrap-around overflow, but with sane values
    // it quits before passing an overflowed param to malloc() later on. The max
    // initially allocated size is abs(*nchars)+1+1 chars when eolend is true
    // (the 2nd +1 is for `\0'), so here we actually define the sane domain for
    // *nchars' to [INT_MIN+2, INT_MAX-2] inclusive.
    if ( nchars ) {
        if ( *nchars > 0 && 2 > (INT_MAX - *nchars) ) {
            DBGMSG( "int arg too big (*nchars=%d)\n", *nchars );
            errno = EDOM;
            goto fail;
        }
        if ( *nchars < 0 && -2 < (INT_MIN - *nchars) ) {
            DBGMSG( "int arg too small (*nchars=%d)\n", *nchars );
            errno = EDOM;
            goto fail;
        }
    }

    // distinguish fixed mode (true) from alloc-ahead mode (false)
    bool ncharsFixed = ( nchars && *nchars > 0 );

    // # of chars to allocate (either fixed or initial alloc-ahead)
    int _nchars = 128;              // default alloc-ahead (*nchars == 0)
    if ( nchars ) {
        if ( *nchars < 0 ) {        // user-defined alloc-ahead
            _nchars = -(*nchars);
        }
        else if ( *nchars > 0 ) {   //fixed *nchars (no alloc-ahead)
            _nchars = *nchars;
        }
    }

    // initial alloc size depends on eolend (but always +1 for nul-terminator)
    char *buf = malloc( eolend ? _nchars+1+1 : _nchars+1 );
    if ( !buf ) {
        DBGMSG( "failed to alloc %d bytes\n", eolend ? _nchars+1+1 : _nchars+1 );
        goto fail;
    }

    // read chars into buf until EOL/EOF, or until *nchars if in fixed mode
    int i = 0;                  // count of successfully read chars
    int c = EOF;
    while ( (c=fgetc(fp)) != '\n' && c != EOF ) {
        if ( i == _nchars ) {
            // fixed *nchars reached? we are done
            if ( ncharsFixed ) {
                break;
            }
            // alloc-ahead _nchars reached? grow buffer size and keep going
            if ( _nchars > (INT_MAX - _nchars - 1) ) { // guard int overflow
                free( buf );
                DBGMSG( "(_nchars=%d) *= 2 would overflow \n", _nchars );
                errno = ERANGE;
                goto fail;
            }
            _nchars *= 2;
            char *tmpbuf = realloc( buf, _nchars+1 );
            if ( !tmpbuf ) {
                free( buf );
                DBGMSG( "failed to realloc %d bytes\n", _nchars+1 );
                goto fail;
            }
            buf = tmpbuf;
        }
        buf[i++] = c;
    }
    // line longer than *nchars? Put back the last unprocessed c, so subsequent
    // function call will not skip it. NOTE: when in alloc-ahead mode the loop
    // stops ONLY after reading EOL or EOF, thus the following if-check fails.
    if ( c != '\n' && c != EOF ) {
        ungetc(c, fp);
        if ( flush ) {          // if specified, also flush extra chars
            for (int ch; (ch=fgetc(fp)) != '\n' && ch != EOF;)
                ;
        }
    }
    // if specified, append EOL
    if ( eolend ) {
        buf[i++] = '\n';
    }
    buf[i] = '\0';              // nul-terminator

    // At this point, should we realloc() buf to its exact size?
    // if ( !ncharsFixed && i < _nchars) {
        // char *tmpbuf = realloc(buf, _nchars+1);
        // if ( tmpbuf ) {
                // buf = tmpbuf;
        // }
    // }

    if ( nchars ) {             // pass to caller # of read chars
        *nchars = i;
    }
    return buf;

fail:
    if ( nchars ) {
        *nchars = 0;
    }
    return NULL;
}

Details

char *fgetline_alloc( FILE *fp, bool flush, int *nchars, bool eolend );

Arguments:

fp: (FILE *) An already opened input text-stream (it may be stdin).

flush: (bool) true if extra chars should be flushed when the line to be read is longer than positive *nchars. It is ignored when *nchars is negative or 0 (see below). In most cases it should be false, but setting it to true can help in validating interactive input with a loop.

nchars: (int *) Pointer to an integer specifying either a fixed count of chars to be read, or an initial alloc-ahead size (in chars). When non-NULL, it passes-back the count of successfully read chars, excluding nul-terminator (0 on error). Passing *nchars as 0, or nchars as NULL, enables alloc-ahead mode with default initial size (128 chars). To enable alloc-ahead mode with a different initial size, pass *nchars with a negative value (see examples). In alloc-ahead mode the initial size auto-grows (doubled) if need be. Also, in alloc-ahead mode the flush argument is ignored by the function.

eolend: (bool) Determines if the returned c-string will end with EOL (true) or not (false). When looping the function to read multiple lines, this arg helps in getting consistent results regardless of the stream's last line, which may or may not end with EOL (it may end with just EOF).

Remarks:

  1. Failed calls of the function can be identified by checking its return value against NULL, or by checking the updated *nchars value against 0. errno is also set either directly by the function or implicitly by the C runtime, thus the caller may additionally use perror().

  2. There is no "partial success", meaning that even if partial data have been successfully read when an error occurs, they are discarded and the function fails.

  3. Valid domain for *nchars is [INT_MIN+2 to INT_MAX-2] inclusive, but memory allocation may be denied for extreme values. If so, the function fails and a subsequent call to perror() will report "not enough space", or similar.

  4. When eolend is true and *nchars is positive (fixed allocation), lines longer than *nchars are truncated to *nchars, followed by EOL as an extra char.

    For example, when:

     int nchars = 2;
     char *str = fgetline_alloc(stdin, flush, &nchars, true);
    

    and then feeding stdin with "12345", will set str to "12\n". Thus the passed-back value of nchars willbe 3 (not the original 2).

Sample Usage Examples

With Text Files:

/* C99 */

// prep
char *input = NULL;
int nchars;
bool flush = true;
bool eolend = true;      // always EOL (false or !eolend means: never EOL)

// Read every line of a text-file, ensuring EOL and outputting the line along with its length

FILE *fp = fopen( "in.txt", "r" );    // any text file
if ( fp ) {
    nchars = 0;          // default alloc-ahead (also causes flush to get ignored)
    while ( (input = fgetline_alloc(fp, !flush, &nchars, eolend)) ) {
        printf( "(%u/%d): %s", strlen(input), nchars, input );
        free( input );
        nchars = 0;      // reset for next call
    }
    fclose( fp );
}

// Store up to MAXLINES lines of a text-file in a custom array of lines, rejecting EOLs

struct Line {
    char *data;
    int len;
} lines[MAXLINES+1] = {{NULL,-1}};    // assuming proper MAXLINES (+1 for sentinel)

FILE *fp = fopen( "in.txt", "r" );    // any text file
if ( fp ) {
    int i = 0;            // lines counter
    nchars = -256;        // 256 chars alloc-ahead (also causes flush to get ignored)
    while ( i < MAXLINES
    && (input=fgetline_alloc(fp, !flush, &nchars, !eolend)) ) {
        printf( "Adding line[%d] (%d chars): %s\n", i, nchars, input );
        lines[i].data = input;
        lines[i].len = nchars;
        nchars = -256;    // reset for next call
        i++;
    }
    fclose( fp );
    printf( "%d lines read\n", i );
    lines[i].len = -1;  // sentinel for lines array
}
// verify and cleanup lines array
for (int i=0; lines[i].len > -1 && i < MAXLINES; i++) {
    printf( "Line[%d] (%d chars): %s\n", i, lines[i].len, lines[i].data );
    free(  lines[i].data );
    lines[i].data = NULL;
    lines[i].len = -1;
}

With stdin (Interactive Input):

/* C99 */

/* Omitting proper checking and freeing for brevity */

// prep
char *input;
int nchars;
bool flush = true;       // flush if line is longer than non-zero positive nchars
bool eolend = true;      // always EOL (false or !eolend means: never EOL)

// Read up to 100 chars (strip EOL, flush extra chars)
nchars = 100;
input = fgetline_alloc(stdin, flush, &nchars, !eolend);

// Read whole line with default alloc-ahead (strip EOL, ignore flush)
nchars = 0;              // also causes flush to get ignored
input = fgetline_alloc(stdin, flush, &nchars, !eolend);

// Read whole line with 64 chars alloc-ahead (strip EOL, ignore flush)
nchars = -64;            // intentionally negative (also causes flush to get ignored)
input = fgetline_alloc(stdin, flush, &nchars, !eolend);

/* Using proper checking and freeing */

// Validation loop (32 chars alloc-ahead, strip EOL, ignore flush)
puts( "Enter phrases (\"end\" without quotes to stop):");
for (;;) {
    nchars = -32;        // intentionally negative (also causes flush to get ignored)
    input = fgetline_alloc(stdin, flush, &nchars, !eolend);
    if ( !input ) {
        perror( "*** fgetline_alloc() failed" );
        break;
    }
    if (input && 0 == strcmp(input, "end") ) {
        free( input );
        break;
    }
    printf( "  %d chars in \"%s\"\n", nchars, input );
    free( input );
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see What should I do when someone answers my question? as well as what you may and may not do after receiving answers. \$\endgroup\$ May 19 at 18:48
  • 1
    \$\begingroup\$ @SᴀᴍOnᴇᴌᴀm, thank you for rolling it back and apologies for my ignorance. I did read the general guidelines before posting, but I apparently missed the ones you just linked to (going over them as we speak). \$\endgroup\$
    – Harry K.
    May 19 at 18:52
3
\$\begingroup\$

The macro is a little over-complex. If we're happy to always provide a string literal as format specifier (and we should be, for debugging), then just concatenate the prefix:

#define DBGMSG(format, ... )\
    fprintf(stderr, "*** %s(): " format, __func__, __VA_ARGS__ )

Now it's a single expression, we no longer need the do … while(0) wrapper, and can use the return value normally.


Using the sign of one argument to switch behaviours is an awkward design choice. It's too easy for an arithmetic error to cause an unintended switch of mode instead of a more visible, and thus correctable, error (either a log message or failure to allocate).

I would prefer to see an extra bool parameter (or more likely a "flags" argument where we can OR together named constants for the options - that's more readable at call sites). We could possibly abstract that behind a pair of clearly-named wrapper functions that then form the main public interface, but given the other switches, I wouldn't do that (we'd need 8 functions already to cover all the flag combinations).


    int _nchars = 128;  

Avoid identifiers with leading underscores, even if you do happen to know the tricky details of what you can legally use in every given context, and what's reserved for the implementation.

In any case, please don't name a variable so similarly to a formal parameter - it causes no end of confusion.

\$\endgroup\$
8
  • \$\begingroup\$ Thank you for taking the time Toby, I appreciate it! I already up-voted your reply and edited my post according to your suggestions, but please allow me a few more days in case anyone else shows up. I mean before accepting it! Btw, any thoughts about the 3 bullets in my post? \$\endgroup\$
    – Harry K.
    May 19 at 18:42
  • 1
    \$\begingroup\$ I didn't have time for a really in-depth review, but saw nothing else odd during a quick skim - so don't accept this answer unless you really get nothing else! No comments on your specific questions - I think your approaches to all three are at least as good as the choices I would have made. Perhaps continuing to use a multiplier of 2 is getting a bit crazy when the size gets very large - perhaps switch to linear growth at some threshold? \$\endgroup\$ May 20 at 6:00
  • \$\begingroup\$ Just saw your edit Toby, thanks! No doubt, signedness is indeed an awkward choice. I debated among several approaches (even having 2 separate funcs). Tbh I'm not very fond of bool params; I think they severely hurt readability when passed as direct values instead of named entities (toggled when needed). And there's 2 of them already. I prefer a flags bitmask instead too, but would require interfacing bit constants. Still not sure which of the 2 is easier from the user pov. If the func goes into a lib, flags makes much more sense, along with specialized negative error-codes via nchars. \$\endgroup\$
    – Harry K.
    May 20 at 11:58
  • \$\begingroup\$ perhaps switch to linear growth at some threshold?, I like this, thank you! \$\endgroup\$
    – Harry K.
    May 20 at 12:03
  • \$\begingroup\$ I would definitely go with the flags bits - it's much easier to both read and write a combination of nice named constants than a sequence of bools when you can never remember which is which. When I wrote "more likely" in the parentheses, I really meant "much much more likely"... \$\endgroup\$ May 20 at 13:35

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.