lib: posix: syscall for pipe(2)

[cfriedt]

This is a WIP. Just putting it out there to see if there are any pointers.

I don't feel like it will be difficult to implement the read, write or ioctl callbacks.

FWIU, close(2) and fcntl(2) are implemented using ioctl.

I did add the st_mode field for struct stat as well as a few related macros.

The read end of the file descriptor will have the S_IRUSR bit set, while the write end of the pipe will have the S_IWUSR bit set.

Functions would be able to easily tell it was a pipe (or socket or regular file) if we also had fstat(2).

Also, this is a requirement for #24367 .

[cfriedt]

Does anyone have a pointer on how to quickly test code either natively or virtually (with Qemu)? Is there a BOARD that just runs an image inside of Qemu that allows you to step debug?

[zephyrbot]

Some checks failed. Please fix and resubmit.

checkpatch issues

-:99: WARNING:LONG_LINE: line over 80 characters
#99: FILE: lib/posix/pipe.c:20:
+#define D(fmt, args...) printk("D: %s(): %d: " fmt "\n", __func__, __LINE__, ##args)

-:139: ERROR:SPACING: space prohibited before that close parenthesis ')'
#139: FILE: lib/posix/pipe.c:60:
+	D("obj: %p buf: %p: sz: %u", obj, buf, sz );

-:157: ERROR:C99_COMMENTS: do not use C99 // comments
#157: FILE: lib/posix/pipe.c:78:
+// Need to learn a bit more about Zephyr kernel vs user space.

-:158: ERROR:C99_COMMENTS: do not use C99 // comments
#158: FILE: lib/posix/pipe.c:79:
+// This causes a compile error, even when I include syscall_handler.h

-:160: WARNING:BLOCK_COMMENT_STYLE: Block comments use * on subsequent lines
#160: FILE: lib/posix/pipe.c:81:
+/*
+	if (Z_SYSCALL_MEMORY_WRITE(buf,sz)) {

-:180: ERROR:C99_COMMENTS: do not use C99 // comments
#180: FILE: lib/posix/pipe.c:101:
+		// N.B. K_FOREVER does not work here!!

-:187: WARNING:LONG_LINE: line over 80 characters
#187: FILE: lib/posix/pipe.c:108:
+	D("k_pipe_get(data: %p bytes_to_read: %u min_xfer: %u timeout: %d", buf, sz, min_xfer, timeout);

-:229: WARNING:LONG_LINE: line over 80 characters
#229: FILE: lib/posix/pipe.c:150:
+	D("obj: %p buf: '%s': sz: %u", obj, (NULL == buf) ? "(null)" : (char *)buf, sz );

-:229: ERROR:SPACING: space prohibited before that close parenthesis ')'
#229: FILE: lib/posix/pipe.c:150:
+	D("obj: %p buf: '%s': sz: %u", obj, (NULL == buf) ? "(null)" : (char *)buf, sz );

-:229: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#229: FILE: lib/posix/pipe.c:150:
+	D("obj: %p buf: '%s': sz: %u", obj, (NULL == buf) ? "(null)" : (char *)buf, sz );

-:247: ERROR:C99_COMMENTS: do not use C99 // comments
#247: FILE: lib/posix/pipe.c:168:
+// Need to learn a bit more about Zephyr kernel vs user space.

-:248: ERROR:C99_COMMENTS: do not use C99 // comments
#248: FILE: lib/posix/pipe.c:169:
+// This causes a compile error, even when I include syscall_handler.h

-:250: WARNING:BLOCK_COMMENT_STYLE: Block comments use * on subsequent lines
#250: FILE: lib/posix/pipe.c:171:
+/*
+	if (Z_SYSCALL_MEMORY_READ(buf,sz)) {

-:265: ERROR:C99_COMMENTS: do not use C99 // comments
#265: FILE: lib/posix/pipe.c:186:
+		// N.B. K_FOREVER does not work here!!

-:275: WARNING:LONG_LINE: line over 80 characters
#275: FILE: lib/posix/pipe.c:196:
+	D("k_pipe_put(data: %p bytes_to_write: %u min_xfer: %u timeout: %d", buf, sz, min_xfer, timeout);

-:276: WARNING:LONG_LINE: line over 80 characters
#276: FILE: lib/posix/pipe.c:197:
+	res = k_pipe_put(&wobj->pipe, (void *)buf, sz, &bytes_written, min_xfer, timeout);

-:305: ERROR:OPEN_BRACE: open brace '{' following function declarations go on the next line
#305: FILE: lib/posix/pipe.c:226:
+static int pipe_ioctl_close(void *_obj, va_list args) {

-:351: ERROR:SPACING: space required before the open parenthesis '('
#351: FILE: lib/posix/pipe.c:272:
+	switch(request) {

-:357: WARNING:ENOSYS: ENOSYS means 'invalid syscall nr' and nothing else
#357: FILE: lib/posix/pipe.c:278:
+		errno = ENOSYS;

-:371: WARNING:LONG_LINE: line over 80 characters
#371: FILE: lib/posix/pipe.c:292:
+	const size_t size = is_read_end ? sizeof(struct pipe_fd_robject) : sizeof(struct pipe_fd_wobject);

-:402: ERROR:SPACING: space prohibited after that '&' (ctx:WxW)
#402: FILE: lib/posix/pipe.c:323:
+		*obj = & ((struct pipe_fd_robject *)o)->obj;
 		       ^

-:405: ERROR:SPACING: space prohibited after that '&' (ctx:WxW)
#405: FILE: lib/posix/pipe.c:326:
+		*obj = & wobj->obj;
 		       ^

-:406: WARNING:LONG_LINE: line over 80 characters
#406: FILE: lib/posix/pipe.c:327:
+		D("initializing Zephyr pipe of size %u at %p", sizeof(wobj->buf), wobj->buf);

-:546: WARNING:LONG_LINE_STRING: line over 80 characters
#546: FILE: tests/posix/common/src/pipe.c:25:
+	zassert_true(res == -1 || res == 0, "pipe returned an unspecified value");

-:550: WARNING:LONG_LINE_STRING: line over 80 characters
#550: FILE: tests/posix/common/src/pipe.c:29:
+	zassert_equal(res, strlen(expected_msg), "did not write entire message");

-:563: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#563: FILE: tests/posix/common/src/pipe.c:42:
+	zassert_true(0 == strncmp(expected_msg, actual_msg,

-:569: WARNING:LONG_LINE_STRING: line over 80 characters
#569: FILE: tests/posix/common/src/pipe.c:48:
+	zassert_equal(res, -1, "pipe should fail when passed an invalid pointer");

-:570: WARNING:LONG_LINE_STRING: line over 80 characters
#570: FILE: tests/posix/common/src/pipe.c:49:
+	zassert_equal(errno, EFAULT, "errno should be EFAULT with invalid pointer");

- total: 12 errors, 16 warnings, 536 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[pfalcon]

Does anyone have a pointer on how to quickly test code either natively or virtually (with Qemu)? Is there a BOARD that just runs an image inside of Qemu that allows you to step debug?

Yes, standard BOARD=qemu_x86 allows debugging using gdb, by following instructions in the Zephyr docs: https://docs.zephyrproject.org/latest/application/index.html?highlight=gdb#id1 . (Disclaimer: I don't do gdb debugging too frequently, and didn't do it for a while, but any time I did, following instructions worked for me. If you face any issues, let us know.)

[pfalcon]

FWIU, close(2) and fcntl(2) are implemented using ioctl.

Ack. More specifically, "via private (Zephyr kernel-only) request codes using ioctl". That's done to not bloat vmethod table for fd types (and thus save some ROM space (including function prologs/epilogs)): only performance-bottleneck operations like read/write are worth separate vmethod, less frequent operation can be multiplexed via a single vmethod (ioctl).

[pfalcon]

I did add the st_mode field for struct stat as well as a few related macros.

That sounds good, but I'd suggest to split stat-related changes into a separate commit, as they're not directly related to pipe().

[pfalcon]

Also I see that you make changes to libc/minimal/include/sys/stat.h, that's again good, but please don't forget that this code should work also with CONFIG_NEWLIB_LIBC=y. In that regard, I'd suggest to follow Newlib's way of things as close as possible, up to e.g. insert st_mode at the same relative place of struct stat (the less discrepancies, the less confusion; of course, if there're truly good reasons to stray away, yes we can (e.g., we didn't have st_mode in minimal libc because nobody needed it)).

[pfalcon]

Do we really need a level of indirection here? Why don't store struct pipe_fd_object* directly here?

[cfriedt]

My initial thought was that I should be able to look up the other end of the pipe with some reliability other than just dereferencing a (possibly invalid) pointer, because closing a pipe from the write end does not automatically close the read end, and vice-versa.

From http://man7.org/linux/man-pages/man7/pipe.7.html :

   If all file descriptors referring to the write end of a pipe have
   been closed, then an attempt to read(2) from the pipe will see end-
   of-file (read(2) will return 0).  If all file descriptors referring
   to the read end of a pipe have been closed, then a write(2) will
   cause a SIGPIPE signal to be generated for the calling process.  If
   the calling process is ignoring this signal, then write(2) fails with
   the error EPIPE. 

There are possible race conditions though.

[pfalcon]

Let me ask first - does k_pipe have a semaphore or mutex?

I'm not author of k_pipe, and don't keep entire picture of Zephyr kernel in mind, so to answer that, would need to consult docs and/or source, which hopefully you can get to earlier than me (working on sth else now).

Is it possible to notify a thread that is waiting on it? I haven't gotten there yet, so just thought it would be useful to consider a situation where I need to check whether a fd is valid.

Not sure how that would be related to check whether a fd is valid. You'd definitely need to block on an empty k_pipe in POSIX read() implementation. And for robust impl, would apparently need to cancel such a read in case write end is closed. Both are possible with k_fifo (which socket code uses), not sure about k_pipe, would need to check docs.

[cfriedt]

Hmm... I might need to look into k_fifo then. k_pipe definitely is a little wonky in its API. E.g. K_FOREVER and min_xfer == 1 block indefinitely, even when min_xfer can be satisfied.

[cfriedt]

Also I see that you make changes to libc/minimal/include/sys/stat.h, that's again good, but please don't forget that this code should work also with CONFIG_NEWLIB_LIBC=y. In that regard, I'd suggest to follow Newlib's way of things as close as possible,

Those macros were copied from newlib ;-)

[cfriedt]

Couldn't get tests running with qemu_x86 because it was complaining about toolchain variables, so I just went with qemu_cortex_m3.

The pipe(), write(), read(), close() tests are working (see below). However, I noticed a some weird things.

1. with the k_pipe API, K_FOREVER literally blocks forever, even when xfer_min == 1. That seems to contradict the docs, so I chose the number 42 for my "blocking" timeout. It's not arbitrarily chosen.
2. I'm certain that I should be checking memory locations, but even more certain I'm messing up the whole kernel-space / user-space thing in Zephyr. I didn't actually create a syscall for this.. is it all in userspace? How should I go about checking memory locations?

$ ninja -C build run
ninja: Entering directory `build'
[0/1] To exit from QEMU enter: 'CTRL+a, x'[QEMU] CPU: cortex-m3
qemu-system-arm: warning: nic stellaris_enet.0 has no peer
*** Booting Zephyr OS build zephyr-v2.2.0-1468-g335cd21f706e  ***
Running test suite posix_apis
===================================================================
starting test - test_posix_pipe_read_write
fildes: {3,4}
res: 21 errno: 0
PASS - test_posix_pipe_read_write
===================================================================
starting test - test_posix_pipe_select
PASS - test_posix_pipe_select
===================================================================
starting test - test_posix_pipe_poll
PASS - test_posix_pipe_poll
===================================================================
Test suite posix_apis succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL

[cfriedt]

the read and write functions are so similar that I should really try to combine them.

[pfalcon]

@cfriedt: Thanks for the patch. I would like to check that you're still interested to proceed with this patch given the discussion at #24426 (comment) .

If you're, first thing please fix review comments from our codestyle bot. (It's pretty much a default process that human reviewers chime into review process after the codestyle and CI checks pass.)

[cfriedt]

Going to close this for the time being since I opted to implement socketpair(2) without using pipe(2).