Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds

[Alexander Turenko <alexander.turenko@tarantool.org>]

> > +int
> > +popen_shutdown(struct popen_handle *handle, unsigned int flags)
> > +{
> > +	assert(handle != NULL);
> > +
> > +	if ((flags & (POPEN_FLAG_FD_STDIN |
> > +		      POPEN_FLAG_FD_STDOUT |
> > +		      POPEN_FLAG_FD_STDERR)) == 0) {
> > +		diag_set(IllegalParams,
> > +			 "popen: neither stdin, stdout nor stderr is choosen");
> > +		return -1;
> > +	}
> > +
> > +	/* Verify the operation. */
> > +	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> 
> 	for (size_t i = 0; i < lengthof(pfd_map); i++)
> 
> We already do a build time check for STDIN_x proper mapping to numbers,
> lets make it shorter.

Sure, changed.

The diff at bottom of the email.

> 
> 
> > +		/* Operate only on asked fds. */
> > +		unsigned int op_mask = pfd_map[idx].mask;
> > +		if ((flags & op_mask) == 0)
> > +			continue;
> > +
> > +		if (popen_may_io(handle, idx, op_mask, true) != 0)
> > +			return -1;
> > +	}
> > +
> > +	/* Perform the operation. */
> > +	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> > +		/* Operate only on asked fds. */
> > +		unsigned int op_mask = pfd_map[idx].mask;
> > +		if ((flags & op_mask) == 0)
> > +			continue;
> > +
> > +		/* Skip already closed fds. */
> > +		if (handle->ios[idx].fd < 0)
> > +			continue;
> > +
> > +		say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
> > +			  handle->pid, stdX_str(idx), idx,
> > +			  handle->ios[idx].fd);
> > +		coio_close_io(loop(), &handle->ios[idx]);
> > +	}
> 
> I don't get why we need two for() cycles?

The first loop is to check that we able to close all fds: all checks are
made first, then all close()'s are called. But the first loop actually
may be replaced with the following check:

 | flags &= POPEN_FLAG_FD_STDIN  |
 |          POPEN_FLAG_FD_STDOUT |
 |          POPEN_FLAG_FD_STDERR;
 | if ((handle->flags & flags) != flags)
 |         /* Give an error here. */

It looks more clean, so I'll use this way. Thanks for catching this!

> Also, I don't like that we
> mangle popen_may_io(). The shutdown is special. Why not do something like

We should check for closed fd in popen_may_io(), because read / write
should give proper error in the case. I guess that here we agreed.

But I made this check conditional in popen_may_io() and I guess it is
what you complain about.

I agree that it looks a bit artificial and so I extracted setting of the
'not supported IO operation' error into a helper function and use it in
popen_shutdown() directly. This really looks better.

See the diff below.

> 
> for (size_t idx = 0; i < lengthof(pfd_map); i++) {
> 	unsigned int op_mask = pfd_map[idx].mask;
> 	if ((flags & op_mask) == 0)
> 		continue;
> 
> 	if (handle->ios[idx].fd < 0)
> 		continue;
> 
> 	...
> }
> 
> Can't we do something like that?

Not exactly, but a kind of this. See below.

WBR, Alexander Turenko.

----

Incremental diff:

--- a/src/lib/core/popen.c	2020-04-15 01:44:51.387489580 +0300
+++ b/src/lib/core/popen.c	2020-04-15 01:44:35.118490605 +0300
@@ -245,21 +245,30 @@
 }
 
 /**
+ * Generate an error about IO operation that is not supported by
+ * a popen handle.
+ */
+static inline int
+popen_set_unsupported_io_error(void)
+{
+	diag_set(IllegalParams, "popen: handle does not support the "
+		 "requested IO operation");
+	return -1;
+}
+
+/**
  * Test if the handle can run a requested IO operation.
  *
  * Returns 0 if so and -1 otherwise (and set a diag).
  */
 static inline int
 popen_may_io(struct popen_handle *handle, unsigned int idx,
-	     unsigned int io_flags, bool allow_closed)
+	     unsigned int io_flags)
 {
-	if (!(io_flags & handle->flags)) {
-		diag_set(IllegalParams, "popen: handle does not support the "
-			 "requested IO operation");
-		return -1;
-	}
+	if (!(io_flags & handle->flags))
+		return popen_set_unsupported_io_error();
 
-       if (! allow_closed && handle->ios[idx].fd < 0) {
+	if (handle->ios[idx].fd < 0) {
 		diag_set(IllegalParams, "popen: attempt to operate "
 			 "on a closed file descriptor");
 		return -1;
@@ -374,7 +383,7 @@
 
 	int idx = STDIN_FILENO;
 
-	if (popen_may_io(handle, idx, flags, false) != 0)
+	if (popen_may_io(handle, idx, flags) != 0)
 		return -1;
 
 	say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
@@ -439,7 +448,7 @@
 	int idx = flags & POPEN_FLAG_FD_STDOUT ?
 		STDOUT_FILENO : STDERR_FILENO;
 
-	if (popen_may_io(handle, idx, flags, false) != 0)
+	if (popen_may_io(handle, idx, flags) != 0)
 		return -1;
 
 	say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
@@ -486,27 +495,29 @@
 {
 	assert(handle != NULL);
 
-	if ((flags & (POPEN_FLAG_FD_STDIN |
-		      POPEN_FLAG_FD_STDOUT |
-		      POPEN_FLAG_FD_STDERR)) == 0) {
+	/* Ignore irrelevant flags. */
+	flags &= POPEN_FLAG_FD_STDIN	|
+		 POPEN_FLAG_FD_STDOUT	|
+		 POPEN_FLAG_FD_STDERR;
+
+	/* At least one ..._FD_STDx flag should be set. */
+	if (flags == 0) {
 		diag_set(IllegalParams,
 			 "popen: neither stdin, stdout nor stderr is choosen");
 		return -1;
 	}
 
-	/* Verify the operation. */
-	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
-		/* Operate only on asked fds. */
-		unsigned int op_mask = pfd_map[idx].mask;
-		if ((flags & op_mask) == 0)
-			continue;
-
-		if (popen_may_io(handle, idx, op_mask, true) != 0)
-			return -1;
-	}
+	/*
+	 * The handle should have all std*, which are asked to
+	 * close, be piped.
+	 *
+	 * Otherwise the operation has no sense: we should close
+	 * parent's end of a pipe, but it was not created at all.
+	 */
+	if ((handle->flags & flags) != flags)
+		return popen_set_unsupported_io_error();
 
-	/* Perform the operation. */
-	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+	for (size_t idx = 0; idx < lengthof(pfd_map); ++idx) {
 		/* Operate only on asked fds. */
 		unsigned int op_mask = pfd_map[idx].mask;
 		if ((flags & op_mask) == 0)