int socket_read_line_alloc(int sock, char** out) {
/* current buffer size */
unsigned int size = 128;
/* current location in buffer */
int i = 0;
/* whether the previous char was a \r */
int cr = 0;
char ch;
/* result, grows as we need it to */
*out = malloc(sizeof(char) * size);
for ( ; ; ) {
if (recv(sock, &ch, 1, 0) == -1) {
return -1;
}
if (i >= size) {
/* grow buffer */
size *= 2;
*out = realloc(*out, size);
}
if (ch == '\n') {
/* if preceded by a \r, we overwrite it */
if (cr) {
assert(i > 0);
i--;
}
(*out)[i] = '\0';
break;
} else {
cr = (ch == '\r' ? 1 : 0);
(*out)[i] = ch;
}
i++;
}
return i + 1;
}
Refactorings
No refactoring yet !
Casper
December 6, 2007, December 06, 2007 23:15, permalink
Several comments.
1: What do you do with the string that is returned from the socket? In many cases when you use sockets you process some data or commands that you receive, and then discard the string you read. If you do something similar here it's innefficient and unnecessary to every time realloc a new string in memory for every call to your function.
Look at how standard C-library read functions are coded (read() or fgets() for example). In most cases it's the callee's responsibility to preallocate the string. Since in most cases the callee (i.e. you) will know what the maximum line length or string length that is about to be read is.
It's much more efficient, and less memory fragmenting, to code it that way, and less error prone. You preallocate one buffer. Say char my_buffer[MAX_LINE_LENGTH]. This you then give to your method:
while (i_want_to_read_some_stuff) {
socket_read_line(sock, my_buffer, MAX_LINE_LENGHT);
/* process the data you received... */
}
2: If you're reading lines anyway you can convert the socket description into a standard C FILE stream and use fgets() instead: FILE *socket_stream_in = fdopen(sock, "r"); fgets(my_buffer, MAX_LINE_LENGTH, socket_stream_in); Why reinvent the wheel? :)
3: The line cr = (ch == '\r' ? 1 : 0); can be refactored as simply cr = (ch == '\r'); I would use a more descriptive variable name though. I like those to make the code more readable. So rename it to something like int got_cr = 0; got_cr = (ch == '\r'); But I'm kind of nitpicky on these..but it still makes if statements read more fluently: if (got_cr) { ...do stuff... }..
Steve
December 11, 2007, December 11, 2007 18:39, permalink
You might consider using a static buffer inside the function like strtok().
I also extended the idea of binding the socket to a FILE and then calling fgets() mentioned by Casper.
I was lazy on checking malloc returns, you might want to make that better.
This may not be exactly what you want for sockets, but you might be able to adapt it to your needs.
const char *fgetLine( FILE* pFile )
{
static size_t bufSize = 128;
static char *pLine = (char*)malloc( 1, bufSize );
size_t index;
bool success = false;
assert( pLine );
pData[0] = '\0';
/* If the file isn't there or at the end, can't do anything */
if ( ( NULL == pFile ) || feof( pFile ) || ferror( pFile ) )
{
}
else
{
index = 0;
while ( !( feof( pFile ) || ferror( pFile ) ) )
{
fgets( &pLine[index], (int)( bufSize - index ), pFile );
index += strlen( &pLine[index] );
/* If we filled the buffer w/o finding newline */
if ( ( index == ( bufSize - 1 ) ) && ( '\n' != pLine[index-1] ) )
{
/* End of file doesn't necessarily end in newline */
if ( feof( pFile ) || ferror( pFile ) )
{
success = ( index > 0 );
break;
}
/* Extend the line length */
else
{
bufSize <<= 1;
pLine = realloc( pLine, bufSize );
assert( pLine );
}
}
/* Finished */
else
{
success = true;
break;
}
}
}
return( ( success ) ? pLine : NULL );
}
I don't like the char** out, it just feels ugly to me. But seeing as I have to return a -1 for a socket error, else the string length, it seems to be the only way. Unless I switch the result and out so I return the char* and write the size/status to an output argument. Thoughts? Also, comments on the actual algorithm are appreciated.