[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Nano-devel] [PATCH] Replace fork with clone to share file descripto

From: Marco Diego Aurélio Mesquita
Subject: Re: [Nano-devel] [PATCH] Replace fork with clone to share file descriptors between parent and child.
Date: Sat, 18 Aug 2018 20:17:04 -0300
User-agent: NeoMutt/20170113 (1.7.2)

On Fri, Aug 17, 2018 at 08:27:36PM +0200, Benno Schulenberg wrote:
> 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.  :|

I still don't know what causes it. Anyway, attached patch fixes it with
addition of only 3 lines.

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

Actually sharing file descriptors would be enough.

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

>From the example I copied. Don't remeber exactly, but not uncommon:

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

To finally answer your question: because this is a thread and threads
share the same memory with its parent process. So, while nano main thread
changed the current buffer, if we didn't use a copy of it, the thread
that outputs the buffer would read data that is been modified.

Now, the attached patch uses fork again, so we have the same portability
as before and no need to explicitly copy data.

Explaining current approach: now, the process that outputs the data
is forked from the same process that filters it. For some (still
unknow) reason, this is enough to avoid the problem we had with file
descriptors. We now have 4 fixes for this problem: the one removing fork,
the one replacing it with a clone, the one replacing it with pthreads and
this one nesting forks. I think this latter should be the preferred one:
it makes fewer changes (just 3 added lines of code) is as portable as
before and fixes the problem we needed.

If there's any need to improve it, please let me know.

Attachment: 0001-filter-reshuffle-some-code-around.patch
Description: Text Data

reply via email to

[Prev in Thread] Current Thread [Next in Thread]