Re: [Nano-devel] [PATCH] Replace fork with clone to share file descriptors between parent and child.

[Benno Schulenberg]

Op 17-08-18 om 00:19 schreef Marco Diego Aurélio Mesquita:
> On Thu, Aug 16, 2018 at 07:40:16PM +0200, Benno Schulenberg wrote:
>> It does fix the problem.  But do you have any
>> idea why the fork() created a problem?  What was it doing wrong?
>>
> I have no idea.

But it is puzzling.  I'd like to know what the filtering fork()
is doing wrong.

> Anyway, I fixed it in a better way: I ported it from clont to
> pthreads. Now, it should be supported on OpenBSD and even MacOS.

Cool.  But... it adds some twenty five lines of code.  :|

> Date: Sun, 12 Aug 2018 13:42:01 -0300
> Subject: [PATCH] filter: replace fork with pthread
> 
> So that file descriptors can be adequately shared.

Doesn't explain anything to me.

> Outputting the buffer (or marked region) should share as much context
> with nano main process as possible,

Why?

Your commit message should explain why a copy of the buffer
needs to be made, before a second thread pushes it out toward
the filter program, while the main thread continues to delete
the original buffer in order to replace it with the output of
the filter program.  Right?

> +dnl AX_PTHREADS leaves PTHREAD_LIBS empty for gcc and sets PTHREAD_CFLAGS
> +dnl to -pthread, which causes problems if we need -lpthread to appear in
> +dnl pkgconfig files.
> +
> +test -z "$PTHREAD_LIBS" && PTHREAD_LIBS="-lpthread"

Where did you get this from?

> -/* Send the text that starts at the given line to file descriptor fd. */
> -void send_data(const filestruct *line, int fd)
> +/* Sends the current buffer or cutbuffer to file descriptor. */
> +void *send_data(void *fds)

Please have a look at the 'cloning' branch in git, and improve
your naming -- "fds" sounds like a plural.

> +     /* Send each line (except a final empty line) and free output buffer. */
> +     while (line != NULL) {

Please don't combine outputting and freeing.  Change as little code
as possible.  Free the buffer afterward, with a single call to
free_filestruct().

> +             /* Make a copy of current buffer or marked region */

As said: why?  Anyway, there is a function for copying a buffer:
copy_filestruct().  Why not use that?  It copies line numbers and
multidata unnecessarily, but I don't care about that.  This filter
feature will be rarely used, so economy of code is more important.

Benno

signature.asc
Description: OpenPGP digital signature