From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 970F44696C3 for ; Wed, 15 Apr 2020 07:21:17 +0300 (MSK) Date: Wed, 15 Apr 2020 07:21:12 +0300 From: Alexander Turenko Message-ID: <20200415042112.ur5anngvrhn7mmrl@tkn_work_nb> References: <6873a6070a181c7c47f2e94295539c483fd6e7b7.1586862436.git.alexander.turenko@tarantool.org> <20200414130507.GH3072@uranus> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200414130507.GH3072@uranus> Subject: Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.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)