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 E72CD46970E for ; Thu, 26 Dec 2019 10:04:47 +0300 (MSK) Received: by mail-lj1-f193.google.com with SMTP id z22so19195383ljg.1 for ; Wed, 25 Dec 2019 23:04:47 -0800 (PST) Date: Thu, 26 Dec 2019 10:04:45 +0300 From: Cyrill Gorcunov Message-ID: <20191226070445.GC2463@uranus> References: <20191210094855.24953-1-gorcunov@gmail.com> <20191210094855.24953-2-gorcunov@gmail.com> <20191226043354.GA1337@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191226043354.GA1337@atlas> 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: Konstantin Osipov Cc: tml On Thu, Dec 26, 2019 at 07:33:54AM +0300, Konstantin Osipov wrote: > > +/** > > + * 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? ok > > + * @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. sure, will do > 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. ok, will do > > +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? Because it is v2 of the series, which is obsolete. In v6 we already process timeouts. > > > + */ > > +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. Already addressed in v6 > 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() Thanks a lot for review and this info about freebsd/macho, Kostya! As to fdsecfs/proc_pidinfo -- I simply don't have these machines to test on. I think we will address these platforms on top of the series.