[Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine
Cyrill Gorcunov
gorcunov at gmail.com
Thu Dec 26 10:04:45 MSK 2019
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.
More information about the Tarantool-patches
mailing list