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

[Benno Schulenberg]

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?

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

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

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

> -     /* Send each line, except a final empty line. */

Why remove this comment?  It should still be valid.

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

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.

Benno

signature.asc
Description: OpenPGP digital signature