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

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Fri, Dec 20, 2019 at 11:11:59AM +0300, Konstantin Osipov wrote:
> * 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.

Yes, it was s typo, should be "Mapping of children pids to popen_handle".
The map may be used for several reasons: 1) to waitpid 2) to fetch
information about specific pid (lua interface). I don't think we should
point every reason for this pid <=> handle mapping here in comment,
but instead the comment (probably) should be "Mapping of children pids
to popen_handle for fast lookup"?

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

Both variants are correct, but I'll update to match atexit
libc helper in a meaning.

> > +/* /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.

Yes, exactly, it is used as stdX. When first attempt to implement popen
hit the mailing list there were comments that we should provide /dev/null
as stdX (I presume because some of programs might require stdX presence
even if they're not writting anything). I'll update the comment to
point the reason. Thanks!

> > +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?

Look, the command is compiled from argv array into
a single string dynamically allocated. The handle
just carries a reference to it. If you think it will
be more suitable to allocate command together with
handle itself in one place then sure I'll update.

> 
> > +	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).

I'll take a look. Thanks!

> > +	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?

You know, internally every child inside libev is kept in own hashmap.
When SIGCHLD happens libev walks over the hash and findis a child
process. When I said "we should continue watching state changes"
I mean that we should not affect any other watcher libev carries
and should stop watching only our pid (remember that in future
we might need to create other child process for some reason,
unrelated to popen). Thus we should call libev's "stop" on the
pid we're interested in, and in result libev will drop own
hash entry matching to this pid. I'll rework the comment.

At first I tried to make libev to watch every children
pid but this caused problems with compilation. Maybe I should
look into this area again.

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

Otherwise it doesn't compile.

> > +}
> > +
> > +/**
> > + * 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?

Would handle_child_exit() sound better? As to nesting -- this is
just in case if we need to send more notifiers in future. I can
inline popen_notify_sigchld here if you prefer.

Or even rename ev_sigchld_cb to popen_notify_sigchld and use
it instead.

> 
> > +	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.

After thinking more I think you're right, plain kill should be enough.

> 
> > +		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.

Will look into this. Thanks a huge for review, Kostya!

	Cyrill