Re: orangefs: NAK until the ABI is documented (was Re: updated orangefs tree at kernel.org)

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Oct 13, 2015 at 01:46:54PM -0400, Mike Marshall wrote:
>  > *shrug*
> 
> Al, I guess what I wrote sounded pi**y, but I promise my attitude
> is 180 degrees away from that... I'm just trying to focus on what is
> most important... you've identified numerous issues, all of which
> need to be addressed, but the state of the API used by userspace
> is a show stopper: it needs documented, and then probably improved...
> 
> When we have something coherent started, we'll send it your way
> so you can help make sure we're on the right track...

No problem...  BTW, one note regarding the iov_iter - unlike an array of
iovec, things like "copy <that many> bytes" are easy; you don't need the
thing to start on the iovec boundary or anything like that.  Simple
copy_from_iter(dest, size, iter) will take care of everything.

Something like
	struct {
		__u32 version;
		__u32 magic;
		__u64 tag;
	} head;

	total = iov_iter_count(iter);
	if (total < 24)
		short packet
	n = copy_from_iter(&head, 16, iter);
	if (n != 16)
		failed copy
	<check head.magic, find the matching upcall by head.tag>
	if (not found)
		stray response

	/* get the beginning of response proper */
	memset(op->downcall, 0, 16);
	n = copy_from_iter(&op->downcall, 16, iter);
	if (n < 16) {
		/* it might be really short... */
		if (iov_iter_count())
			failed copy
		/* yes, and that's too short to have a trailer */
		/* just zero-pad and that's it */
		memset((char *)&op->downcall + n, 0,
			sizeof(op->downcall) - n);
		goto done;
	}

	/* will there be a trailer? */
	trailer_count = 0;
	if (op->downcall.status == 0 && op->downcall.trailer_count) {
		trailer_count = op->downcall.trailer_count;
		if (total - 32 < trailer_count)
			packet too short
		buf = vmalloc(trailer_count)
		if (!buf)
			op->downcall.status = -ENOMEM;
	}

	n = total - 32 - trailer_count;
	if (sizeof(op->downcall) - 16 < n)
		packet too long
	if (copy_from_iter((char *)op->downcall + 16, 0, n) != n)
		failed copy
	memset((char *)op->downcall + 16 + n, 0,
		sizeof(op->downcall) - n - 16);

	if (trailer_count) {
		/* read or skip the trailer */
		if (!buf) {
			iov_iter_advance(trailer_count, iter);
		} else {
			n = copy_from_iter(buf, trailer_count, iter);
			if (n < trailer_count)
				failed copy
		}
	}
done:
	...
	return total;

would do marshalling and accept all cases where your current code doesn't
produce an obvious breakage.  It's a lot more flexible than your current
*userland* code needs and probably more flexible than it would make sense
to have; e.g. if packets are not allowed to be just 24 bytes long, we
could turn that if (n < 16) {...} into if (n != 16) failed copy,
if we can always assume that packet is at least 16 + sizeof(op->downcall),
if could be simplified even more, etc.

There's no real benefit in using the boundary of the magic 5th segment to
tell where the trailer starts.  The above is just a demo, but hopefully it
does demonstrate how to use those primitives.  Basically, use copy_from_iter()
as you would use read() on stdin when writing a userland filter, with
iov_iter_advance() used to skip a given amount and iov_iter_count() telling
how much input is left.