[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: Thu, 16 Aug 2018 19:19:41 -0300
User-agent: NeoMutt/20170113 (1.7.2)

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

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

reply via email to

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