From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2AB6346970E for ; Thu, 26 Dec 2019 07:33:57 +0300 (MSK) Received: by mail-lj1-f193.google.com with SMTP id j26so23791223ljc.12 for ; Wed, 25 Dec 2019 20:33:57 -0800 (PST) Date: Thu, 26 Dec 2019 07:33:54 +0300 From: Konstantin Osipov Message-ID: <20191226043354.GA1337@atlas> References: <20191210094855.24953-1-gorcunov@gmail.com> <20191210094855.24953-2-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191210094855.24953-2-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml > +/** > + * command_new - allocates a command string from argv array Would be nice to say why you need to linearise the command at all - is it for logging, or error messages, or what? > + * @argv: an array with pointers to argv strings > + * @nr_argv: number of elements in the @argv > + * > + * Returns a new string or NULL on error. > + */ > +static inline char * > +command_new(char **argv, size_t nr_argv) _new/_delete are usually used for classes/objects. command is not a standalone class, so a better name for the function is alloc_argv or similar. having a separate command_free(0) IMO is over-engineering, as well as separate handle_free and popen_delete(). I would inline handle_free() and popen_delete() into popen, as well as handle_new(). If not, I would at least move all free/destroy functions close together, so that the code is easier to make ends of - right now popen_delete() as at the end of a long file, while command_new/handle_new - at the beginnign. > +ssize_t > +popen_write(struct popen_handle *handle, void *buf, > + size_t count, unsigned int flags) > +{ > + if (!popen_may_io(handle, STDIN_FILENO, flags)) > + return -1; > + > + if (count > (size_t)SSIZE_MAX) { > + errno = E2BIG; > + return -1; > + } > + > + say_debug("popen: %d: write idx [%s:%d] buf %p count %zu", > + handle->pid, stdX_str(STDIN_FILENO), > + STDIN_FILENO, buf, count); > + > + return write(handle->fds[STDIN_FILENO], buf, count); > +} I think popen_write() should work like follows: while (not error and not written the full contents of the buffer) { rc = write() // handle errors // advance write position // if written_size != buf_size coio_fiber_yield_timeout() until the descriptor // becomes ready. } For that to work, the descriptor should be set to non-blocking on parent side right after fork. Why are you allowing a partial write here? Why are you not accepting an optional timeout? > + */ > +static int > +popen_wait_read(struct popen_handle *handle, int idx, int timeout_msecs) > +{ > + struct pollfd pollfd = { > + .fd = handle->fds[idx], > + .events = POLLIN, > + }; > + int ret; > + > + ret = poll(&pollfd, 1, timeout_msecs); Here you block the event loop for timeout_msecs. Why aren't you using coio_fiber_yield_timeout()? The timeout should be in ev_tstamp format, not integer. popen_read(), similar to popen_write() should be reading the requested amount or error, not return a partial read. > +#else > + /* FIXME: What about FreeBSD/MachO? freebsd has fdsecfs mac has proc_pidinfo() > -- Konstantin Osipov, Moscow, Russia