>tear it apart
As ordered. I'll even ignore the non-portable stuff that you probably have already been flamed about.
>#include <malloc.h>
This is an archaic header that was supplanted by stdlib.h.
>char *sgets(char *buf, int bufsize);
This is the first scary part. You expect to pass in a primary buffer and buffer size, yet the function is supposed to return a pointer to dynamic memory. What happens to buf?
>size_t strlcpy(char *d, const char *s, size_t bufsize);
>size_t strlcat(char *d, const char *s, size_t bufsize);
Both of these invade the implementation's namespace by using str followed by a lower case letter. Even worse, they're clones of two common extension functions, so the chances of a name conflict with the imlementation are pretty good.
Code:
if(fgets(buf, bufsize, stdin) == NULL)
{
return(NULL);
}
All is well so far. If fgets fails then sgets fails. That's a reasonable expectation.
Code:
if( strchr(buf, '\n') == NULL )
{
newbuf = malloc(bufsize);
strlcpy(newbuf, buf, bufsize);
newsize = bufsize;
}
Everything is still okay, but now I begin to wonder why you only allocate memory if a newline wasn't in the buffer, and whether the lack of + 1 for a null character in malloc is an error or you already accounted for it.
Code:
else
{
return(buf);
}
Bzzt! Your code will be the source of endless confusion because the client will expect a pointer to dynamic memory to be returned. Because buf is actually an array, when the client tries to do the right thing and free their memory when they're done, the program will likely crash. Either return the buffer you're given or a pointer to memory allocated within the function, but not both.
>newsize += (BUFSIZ * sizeof(char));
sizeof(char) is guaranteed to be 1, so you can omit that part of the expression.
>newbuf = realloc(newbuf, newsize);
Once again, how do I know that you've left room for a null character at the end? People expect to see this, so if you don't include it then a comment explaining why is a good idea.
Code:
if( newbuf == NULL )
{
return(NULL);
}
This causes a memory leak. No problem if your system reclaims memory when the short running program terminates, not okay if the function is used in a long running program or if the system doesn't reclaim memory properly.
>return(newbuf);
This is where you return freeable memory, but if clients crash half the time they use your function, they'll probably break the code even more by not calling free. Or they'll use fgets, which is clearly easier and more predictable.
Let's try again, this time with the following goals in mind:
- Handle empty lines
- Handle EOF
- Handle input errors
- Work with any input stream
- Always return freeable memory
- Be conservative with the returned memory if possible
- Remove the trailing newline character
- Return NULL only if no input could be processed, otherwise return what we could get
- Avoid leaking memory
The simplest possible declaration for such a function would be:
Code:
char *readline(FILE *in);
It's equally simple to use, and a test program is immediately obvious:
Code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
char *readline(FILE *in);
int main(void)
{
char *p = readline(stdin);
if (p != NULL) {
printf("[%s]\n", p);
free(p);
}
return 0;
}
/* Definition for readline here */
As with your sgets, my readline will use a temporary buffer and fgets, but I'll be liberal with the memory allocation to be on the safe side (I whipped this out pretty quickly, so I'd rather err on the side of having too much). Because of this, and one of the goals, I'll attempt to shrink wrap the memory to fit perfectly just before returning.
The outline might look like this if we know that a pointer to char is needed for the final string, an array of char for the buffer, a save pointer for safe realloc calls, and the current allocation size:
Code:
char *readline(FILE *in)
{
char *ret = NULL, *save;
char buf[BUFSIZ];
int ret_size = 0;
while (fgets(buf, sizeof buf, in) != NULL) {
/* Resize */
/* Copy */
/* Are we done? */
}
/* Shrink wrap if possible */
if (ret != NULL) {
save = realloc(ret, strlen(ret) + 1);
if (save != NULL)
ret = save;
}
return ret;
}
The resize part is easy, but I do make use of an obscure feature of realloc. If the pointer argument is NULL then realloc acts like malloc, so there's no need for a preliminary malloc call.
Code:
save = realloc(ret, ret_size + BUFSIZ + 1);
if (save == NULL)
return ret;
ret = save;
The copy part is equally easy once you figure out how to copy on the first go around and concatenate every time after. The quick and dirty solution is just to test and call either strcpy or strcat. ret_size can't be modified until after this test though, so the increment is a part of the copy code.
Code:
ret_size == 0 ? strcpy(ret, buf) : strcat(ret, buf);
ret_size += BUFSIZ;
Lastly, check to see if the buffer had a newline. If it did then to meet one of the requirements we need to turn it into a null character before breaking from the loop and trying to shrink wrap. There's a slight performance penalty here because strchr returns a pointer to within the string. If we used strchr and buf then the newline wouldn't be removed in ret, so ret must be used with strchr and ret might be long.
Code:
save = strchr(ret, '\n');
if (save != NULL) {
*save = '\0';
break;
}
Here's the finished code:
Code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
char *readline(FILE *in);
int main(void)
{
char *p = readline(stdin);
if (p != NULL) {
printf("[%s]\n", p);
free(p);
}
return 0;
}
char *readline(FILE *in)
{
char *ret = NULL, *save;
char buf[BUFSIZ];
int ret_size = 0;
while (fgets(buf, sizeof buf, in) != NULL) {
/* Resize */
save = realloc(ret, ret_size + BUFSIZ + 1);
if (save == NULL)
return ret;
ret = save;
/* Copy */
ret_size == 0 ? strcpy(ret, buf) : strcat(ret, buf);
ret_size += BUFSIZ;
/* Are we done? */
save = strchr(ret, '\n');
if (save != NULL) {
*save = '\0';
break;
}
}
/* Shrink wrap if possible */
if (ret != NULL) {
save = realloc(ret, strlen(ret) + 1);
if (save != NULL)
ret = save;
}
return ret;
}
Compare and contrast.