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

[Marco Diego Aurélio Mesquita]

On Thu, Aug 16, 2018 at 07:40:16PM +0200, Benno Schulenberg wrote:
> 
> Op 12-08-18 om 18:51 schreef Marco Diego Aurélio Mesquita:
> > The attached patch replaces the call to fork in execute_command with clone.
> > This way, file descriptors can be shared between parent and child 
> > effectively
> > fixing https://savannah.gnu.org/bugs/?54499 .
> 
> Thanks for the patch.  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. 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.

> > A dup2 was removed since it was not having any effect.
> 
> Is the removal necessary to make your patch work?  If not, then
> it should be a separate change.
> 

Re-added on the attached patch.

> Is the clone() function POSIX?  Because it does not seem to exist
> on OpenBSD:
> 
>   text.c: In function 'execute_command':
>   text.c:1206: warning: implicit declaration of function 'clone'
>   text.c:1206: error: 'CLONE_FILES' undeclared (first use in this function)
> 
> On NetBSD and FreeBSD it compiles fine, though.  And it works too
> on NetBSD.  (On FreeBSD I can't test, because I haven't figured out
> how to get around the failure of linking against gettext.)
> 

The clone syscall is linux specific. Attached uses pthread instead.

> > +#define STACK_SIZE 65536
> > +   /* Stack size for cloned process to pipe out the buffer. */
> 
> Why such a big stack?  How much space does send_data() need, do
> you think?
> 

The example suggest 1024*1024. I saw the value 65536 on another example.

Anyway, current implementation doesn't care about it. So, I think it is
irrelevant now.

> > -   /* Send each line, except a final empty line. */
> 
> Why remove this comment?  It should still be valid.
> 

Improved on the attached patch.

> > +   int *to_fd = ((int*)fds);
> > +   const filestruct *line = cutbuffer != NULL ? cutbuffer : 
> > openfile->fileage;
> >     while (line != NULL && (line->next != NULL || line->data[0] != '\0')) {
> 
> Between declarations and code should be a blank line.
> 

Fixed on the attached patch.

> Apart from those cosmetic issues, your patch looks fine.
> The commit message should explain more why the change is
> needed, though, why the descriptors need to be shared.
> 

Fixed on the attached patch.

0001-filter-replace-fork-with-pthread.patch
Description: Text Data