On 18/02/12 12:15, Albrecht Schlosser wrote:
> I see a few issues with the proposed code. Posting for discussion...
Thanks; this is precisely the sort of constructive dialogue I hoped
to encourage. Comments attached directly to the ticket might have been
preferred, but no big deal; we can easily add a cross reference to this
mail thread.
> The stream argument shouldn't be checked for NULL (and return EINVAL),
You're right. I added the stream argument to the original checks on
linebuf and len, as an afterthought. Blessed with hindsight, I realise
that I should have checked it separately...
> but leave this up to fgetc() later
...but not like this, I think...
> or return EBADF instead,
...but rather this, explicitly.
> which fgetc() would do.
Unfortunately, I don't think it would. At least, running under wine,
(and with native code compiled for Linux):
int x = fgetc( (FILE *)(NULL) );
doesn't return at all -- it aborts with a segmentation fault. Sure, one
might argue that, if it's okay for fgetc() to abort in this ugly manner,
when handed a NULL stream reference,it should be good enough for
getline() and getdelim() too, but to me, it just seems to smack of
laziness and carelessness.
> The standard says "For the conditions under which the getdelim() and
> getline() functions shall fail and may fail, refer to fgetc", and
> this would be one of them.
It would, if fgetc() did actually fail gracefully, with an appropriate
assignment to errno, in this circumstance.
> The way how realloc() is called will lead to a memory leak if realloc()
> returns NULL.
Indeed it will; thanks for pointing this out.
> This will probably only happen if realloc has been called
> many times before, and the buffer is already very long. The previous
> linebuf pointer would be lost, and *len would have been incremented
> already. A cleaner solution would be to keep the previous pointer and
> the buffer size intact (so the buffer could at least be free()'d).
>
> The standard doesn't say anything about what should be done in this
> case, but I'd expect the buffer pointer (and size) to be valid as of
> the time when realloc() fails.
I've adjusted it, so the original pointer and length will be retained,
while the function will immediately return -1; caller will still have
access to the abandoned buffer, and its length, and will be expected to
take responsibility for clean up.
> Besides that there would be no way to retrieve the data that has
> already been read. IMHO it would be useful to zero-terminate the
> buffer also in case of failure. However, this would still not
> guarantee that the data can be correctly retrieved, since zero bytes
> within the data are allowed. Again, the standard doesn't say much
> about this case, maybe this should be regarded as undefined, but...
I didn't bother to NUL terminate it, but with the second patch the
abandoned buffer remains accessible to the caller, which could supply
the terminator if desired; I don't know how useful that might be.
--
Regards,
Keith.
|
