[Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds
Alexander Turenko
alexander.turenko at tarantool.org
Wed Apr 15 07:21:12 MSK 2020
> > +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)
More information about the Tarantool-patches
mailing list