Re: [Tarantool-patches] [PATCH v6 2/4] popen: Introduce a backend engine

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/12/17 15:57]:
> +/* Children pids map popen_handle map */

map popen_handle map?
Even if I remove double "map" I don't undesrtand the ocmment.

Is it pid -> popen_handle() map? How is it used? For waitpid?
Please explain in the comment.

> +static struct mh_i32ptr_t *popen_pids_map = NULL;
> +
> +/* All popen handles to be able to cleanup them on exit */
at exit

> +static RLIST_HEAD(popen_head);
> +
> +/* /dev/null to be used inside children if requested */

This is a bit too sketchy. Please explain: how it is requested,
what it is used for, etc. I can of course guess that it's used
as children stdin/stdout, if there is an option to popen(), but
that's it.

> +static int dev_null_fd_ro = -1;
> +static int dev_null_fd_wr = -1;

> +/**
> + * handle_free - free memory allocated for a handle
> + * @handle:	popen handle to free
> + *
> + * Just to match handle_new().
> + */
> +static void
> +handle_free(struct popen_handle *handle)
> +{
> +	say_debug("popen: handle %p free %p", handle);
> +	free(handle);
> +}

I don't understand who owns char *command and why 
it's not freed here. Could you please clarify?

> +	return coio_write_fd_timeout(handle->fds[idx],
> +				     buf, count, timeout);

You could store struct coio for each child stream in the handle,
it's pretty cheap. I don't have a strong opinion on this,
your implementation looks tidy (although has some overhead).

> +/**
> + * popen_notify_sigchld - notify popen subsistem about SIGCHLD event
subsystem

> +	if (WIFEXITED(wstatus) || WIFSIGNALED(wstatus)) {
> +		assert(handle->pid == pid);
> +		/*
> +		 * libev calls for waitpid by self so
> +		 * we don't have to wait here.
> +		 */
> +		popen_unregister(handle);
> +		/*
> +		 * Since SIGCHLD may come to us not
> +		 * due to exit/kill reason (consider
> +		 * a case when someone stopped a child
> +		 * process) we should continue wathcing
> +		 * state changes, thus we stop monitoring
> +		 * dead children only.
> +		 */
> +		say_debug("popen: ev_child_stop %d", handle->pid);
> +		ev_child_stop(EV_DEFAULT_ &handle->ev_sigchld);
> +		handle->pid = -1;
> +	}

I don't understand how the code matches the comment.

You write "we should continue watching state changes" inside
a branch that handles termination. How is this comment relevant to
what you are doing in this branch?

Why do you have to cast handle->ev_sigchld to EV_DEFAULT_?


> +}
> +
> +/**
> + * ev_sigchld_cb - handle SIGCHLD from a child process
> + * @w:		a child exited
> + * @revents:	unused
> + */
> +static void
> +ev_sigchld_cb(EV_P_ ev_child *w, int revents)
> +{
> +	(void)revents;
> +	/*
> +	 * Stop watching this child, libev will
> +	 * remove it from own hashtable.
> +	 */
> +	ev_child_stop(EV_A_ w);
> +
> +	/*
> +	 * The reason for a separate helper is that
> +	 * we might need to notify more subsystems
> +	 * in future.
> +	 */
> +	popen_notify_sigchld(w->rpid, w->rstatus);

I think ev_sigchld_cb is a too general name. Besides, why do you
need two functions, and call one from another?

> +	if (handle->flags & POPEN_FLAGS_SETSID) {
> +		if (signo == SIGKILL || signo == SIGTERM) {
> +			say_debug("popen: killpg %d signo %d",
> +				  handle->pid, signo);
> +			ret = killpg(handle->pid, signo);

I don't understand why you need to use killpg() here.

Could you please explain?
I suspect that you don't, you're just trying to be safe,
and there is no need to.

> +		if (handle->fds[i] != -1) {
> +			/*
> +			 * We might did some i/o on
> +			 * this fd, so make sure
> +			 * libev removed it from
> +			 * watching before close.
> +			 *
> +			 * Calling coio_close on
> +			 * fd which never been watched
> +			 * if safe.
> +			 */
> +			coio_close(handle->fds[i]);

Ugh. This definitely suggests you need a coio object 
in popen_handle(), this coio_close() looks like a hack.

-- 
Konstantin Osipov, Moscow, Russia