Re: [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine

[Cyrill Gorcunov <gorcunov@gmail.com>]

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.