From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f194.google.com (mail-lj1-f194.google.com [209.85.208.194]) (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 6CA3E46970E for ; Fri, 20 Dec 2019 11:12:01 +0300 (MSK) Received: by mail-lj1-f194.google.com with SMTP id z22so4254943ljg.1 for ; Fri, 20 Dec 2019 00:12:01 -0800 (PST) Date: Fri, 20 Dec 2019 11:11:59 +0300 From: Konstantin Osipov Message-ID: <20191220081159.GG30445@atlas> References: <20191217125420.20881-1-gorcunov@gmail.com> <20191217125420.20881-3-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191217125420.20881-3-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v6 2/4] 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 * Cyrill Gorcunov [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