[PATCH v5] stdio: implement fopencookie(3)

[PATCH v5] stdio: implement fopencookie(3)

[William Pitcock]

The fopencookie(3) function allows the programmer to create a custom
stdio implementation, using four hook functions which operate on a
"cookie" data type.

Changelog:

v5:
- implement stdio readahead buffering support

v4:
- remove parameter names from header function declarations

v3:
- remove spurious `struct winsize`
- make f->lock unconditionally 0

v2:
- properly implement stdio buffering

v1:
- initial proof of concept
---
 include/stdio.h         |   9 ++++
 src/stdio/fopencookie.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 src/stdio/fopencookie.c

diff --git a/include/stdio.h b/include/stdio.h
index 884d2e6a..da0563f6 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -182,6 +182,15 @@ int vasprintf(char **, const char *, __isoc_va_list);
 #ifdef _GNU_SOURCE
 char *fgets_unlocked(char *, int, FILE *);
 int fputs_unlocked(const char *, FILE *);
+
+typedef struct {
+	ssize_t (*read)(void *, char *, size_t);
+	ssize_t (*write)(void *, const char *, size_t);
+	int (*seek)(void *, off_t *, int);
+	int (*close)(void *);
+} cookie_io_functions_t;
+
+FILE *fopencookie(void *, const char *, cookie_io_functions_t);
 #endif
 
 #if defined(_LARGEFILE64_SOURCE) || defined(_GNU_SOURCE)
diff --git a/src/stdio/fopencookie.c b/src/stdio/fopencookie.c
new file mode 100644
index 00000000..fa15be00
--- /dev/null
+++ b/src/stdio/fopencookie.c
@@ -0,0 +1,115 @@
+#define _GNU_SOURCE
+#include "stdio_impl.h"
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+
+struct fcookie {
+	void *cookie;
+	cookie_io_functions_t iofuncs;
+};
+
+static size_t cookieread(FILE *f, unsigned char *buf, size_t len)
+{
+	struct fcookie *fc = f->cookie;
+	size_t ret, remain = len, readlen = 0;
+
+	if (fc->iofuncs.read == NULL) return 0;
+
+	ret = fc->iofuncs.read(fc->cookie, (char *) buf, len - 1);
+	if (ret == 0) goto bail_eof;
+
+	readlen += ret;
+	remain -= ret;
+
+	f->rpos = f->buf;
+	ret = fc->iofuncs.read(fc->cookie, (char *) f->rpos, f->buf_size);
+	if (ret == 0) goto bail_eof;
+	f->rend = f->rpos + ret;
+
+	if (remain > f->buf_size) remain = f->buf_size;
+	memcpy(buf + readlen, f->rpos, remain);
+	readlen += remain;
+	f->rpos += remain;
+
+	return readlen;
+
+bail_eof:
+	f->flags |= F_EOF;
+	f->rpos = f->rend = f->buf;
+	return readlen;
+}
+
+static size_t cookiewrite(FILE *f, const unsigned char *buf, size_t len)
+{
+	struct fcookie *fc = f->cookie;
+	size_t ret;
+	size_t len2 = f->wpos - f->wbase;
+	if (fc->iofuncs.write == NULL) return 0;
+	if (len2) {
+		f->wpos = f->wbase;
+		if (cookiewrite(f, f->wpos, len2) < len2) return 0;
+	}
+	return fc->iofuncs.write(fc->cookie, (const char *) buf, len);
+}
+
+static off_t cookieseek(FILE *f, off_t off, int whence)
+{
+	struct fcookie *fc = f->cookie;
+	if (fc->iofuncs.seek) return fc->iofuncs.seek(fc->cookie, &off, whence);
+	return -1;
+}
+
+static int cookieclose(FILE *f)
+{
+	struct fcookie *fc = f->cookie;
+	if (fc->iofuncs.close) return fc->iofuncs.close(fc->cookie);
+	return 0;
+}
+
+FILE *fopencookie(void *cookie, const char *mode, cookie_io_functions_t iofuncs)
+{
+	FILE *f;
+	struct fcookie *fc;
+
+	/* Check for valid initial mode character */
+	if (!strchr("rwa", *mode)) {
+		errno = EINVAL;
+		return 0;
+	}
+
+	/* Allocate FILE+fcookie+buffer or fail */
+	if (!(f=malloc(sizeof *f + sizeof *fc + UNGET + BUFSIZ))) return 0;
+
+	/* Zero-fill only the struct, not the buffer */
+	memset(f, 0, sizeof *f);
+
+	/* Impose mode restrictions */
+	if (!strchr(mode, '+')) f->flags = (*mode == 'r') ? F_NOWR : F_NORD;
+
+	/* Set up our fcookie */
+	fc = (void *)(f + 1);
+	fc->cookie = cookie;
+	fc->iofuncs.read = iofuncs.read;
+	fc->iofuncs.write = iofuncs.write;
+	fc->iofuncs.seek = iofuncs.seek;
+	fc->iofuncs.close = iofuncs.close;
+
+	f->fd = -1;
+	f->cookie = fc;
+	f->buf = (unsigned char *)f + sizeof *f + sizeof *fc + UNGET;
+	f->buf_size = BUFSIZ;
+	f->lbf = EOF;
+	f->lock = 0;
+
+	/* Initialize op ptrs. No problem if some are unneeded. */
+	f->read = cookieread;
+	f->write = cookiewrite;
+	f->seek = cookieseek;
+	f->close = cookieclose;
+
+	/* Add new FILE to open file list */
+	return __ofl_add(f);
+}
-- 
2.13.3

Re: [PATCH v5] stdio: implement fopencookie(3)

[Rich Felker]

On Wed, Oct 11, 2017 at 04:52:54AM +0000, William Pitcock wrote:
> +static size_t cookieread(FILE *f, unsigned char *buf, size_t len)
> +{
> +	struct fcookie *fc = f->cookie;
> +	size_t ret, remain = len, readlen = 0;
> +
> +	if (fc->iofuncs.read == NULL) return 0;

This should probably set F_EOF (per the man page). Also as a matter of
style, musl usually uses !p rather than p==0 or p==NULL, and avoids
NULL entirely. I wouldn't insist on changing this if there were
nothing else wrong with the code, but if you have to change it for EOF
anyway it'd be nice to make it consistent with style too.

> +	ret = fc->iofuncs.read(fc->cookie, (char *) buf, len - 1);

Except when called by fread, len is always 1, so you're making a call
to iofuncs.read with a length of 0 in all these cases. I doubt
iofuncs.read has accepting lenth 0 as part of its (undocumented)
contract; it seems like a really bad idea to do it even if it is
valid.

> +	if (ret == 0) goto bail_eof;

And in this case it's going to have to return 0, which makes goto
bail_eof clearly wrong. I don't think this code could have passed even
minimal testing. Have you only tried it with fread() and not other
functions like fgetc(), fgets(), etc.?

This can probably be fixed by making the above code (both the
iofuncs.read call and the goto check) conditional on len>1.

If len>1, you are servicing an fread() that wasn't satisfied fully by
existing buffer contents. Last night on IRC I suggested that it might
make sense to avoid filling the buffer at all in this case (since it's
a bit costly -- it requires 2 calls to iofuncs.read as you've done),
but I think I was wrong. The interesting case is a number of small
fread calls, each reading a length much smaller than the buffer size.
In that case, if you don't fill the buffer, you end up with lots of
calls to iofuncs.read. So I think you need to fill the buffer.

I still do think there might be good ways to make it so iofuncs.read
is always called just once, but making it work that way right now is
not essential as long as it just works, and might be better done as
part of a larger overhaul of how the stdio readv-like stuff works. As
noted before, the current way read/write backends for stdio FILEs have
to work is really klunky and designed around readv/writev for ordinary
files and around the needs of fread/fwrite without much consideration
for other functions. It could probably be improved considerably at
some point, but doing so will be delicate and probably should be done
alongside properly documenting all the internals.

> +	readlen += ret;
> +	remain -= ret;
> +
> +	f->rpos = f->buf;
> +	ret = fc->iofuncs.read(fc->cookie, (char *) f->rpos, f->buf_size);
> +	if (ret == 0) goto bail_eof;
> +	f->rend = f->rpos + ret;
> +
> +	if (remain > f->buf_size) remain = f->buf_size;
> +	memcpy(buf + readlen, f->rpos, remain);
> +	readlen += remain;
> +	f->rpos += remain;
> +
> +	return readlen;
> +
> +bail_eof:
> +	f->flags |= F_EOF;
> +	f->rpos = f->rend = f->buf;
> +	return readlen;
> +}

The rest of this looks like it mostly works, but seems to be missing
error handling. The man page documents that the read function can
return -1 on error, so I think you need to check for this and set the
stream error state.

> +static size_t cookiewrite(FILE *f, const unsigned char *buf, size_t len)
> +{
> +	struct fcookie *fc = f->cookie;
> +	size_t ret;
> +	size_t len2 = f->wpos - f->wbase;
> +	if (fc->iofuncs.write == NULL) return 0;
> +	if (len2) {
> +		f->wpos = f->wbase;
> +		if (cookiewrite(f, f->wpos, len2) < len2) return 0;
> +	}
> +	return fc->iofuncs.write(fc->cookie, (const char *) buf, len);
> +}

Likewise this is missing error handling. If iofuncs.write returns -1,
it will wrongly return SIZE_MAX, probably blowing up pointer
arithmetic in the caller. Instead it needs to return some value <len,
the actual amount of len written, on error, and must set the stream
error state if this happens. See __stdio_write.c and its handling of
cnt<0.

> +static off_t cookieseek(FILE *f, off_t off, int whence)
> +{
> +	struct fcookie *fc = f->cookie;
> +	if (fc->iofuncs.seek) return fc->iofuncs.seek(fc->cookie, &off, whence);
> +	return -1;
> +}
> +
> +static int cookieclose(FILE *f)
> +{
> +	struct fcookie *fc = f->cookie;
> +	if (fc->iofuncs.close) return fc->iofuncs.close(fc->cookie);
> +	return 0;
> +}
> +
> +FILE *fopencookie(void *cookie, const char *mode, cookie_io_functions_t iofuncs)
> +{
> +	FILE *f;
> +	struct fcookie *fc;
> +
> +	/* Check for valid initial mode character */
> +	if (!strchr("rwa", *mode)) {
> +		errno = EINVAL;
> +		return 0;
> +	}
> +
> +	/* Allocate FILE+fcookie+buffer or fail */
> +	if (!(f=malloc(sizeof *f + sizeof *fc + UNGET + BUFSIZ))) return 0;

In practice I think the pointer arithmetic works out okay, but it
might be better to use something like:

struct cookie_FILE {
	FILE f;
	struct fcookie fc;
	char buf[UNGET+BUFSIZ];
};

so that you don't have to rely on it being right.

> +	/* Zero-fill only the struct, not the buffer */
> +	memset(f, 0, sizeof *f);
> +
> +	/* Impose mode restrictions */
> +	if (!strchr(mode, '+')) f->flags = (*mode == 'r') ? F_NOWR : F_NORD;
> +
> +	/* Set up our fcookie */
> +	fc = (void *)(f + 1);
> +	fc->cookie = cookie;
> +	fc->iofuncs.read = iofuncs.read;
> +	fc->iofuncs.write = iofuncs.write;
> +	fc->iofuncs.seek = iofuncs.seek;
> +	fc->iofuncs.close = iofuncs.close;
> +
> +	f->fd = -1;
> +	f->cookie = fc;
> +	f->buf = (unsigned char *)f + sizeof *f + sizeof *fc + UNGET;
> +	f->buf_size = BUFSIZ;
> +	f->lbf = EOF;
> +	f->lock = 0;

The reason f->lock is 0 and not -1 might be worth a comment.

Rich