Opened 16 years ago

Closed 15 years ago

Last modified 15 years ago

#210 closed task (fixed)

The way to use execvp in the multiarch_wrapper, in order to keep control over the processes.

Reported by: manurootcpp Owned by: clfs-commits@…
Priority: minor Milestone: CLFS Standard 1.2.0
Component: BOOK Version: CLFS Standard 1.2.0
Keywords: multiarch_wrapper Cc:

Description (last modified by Joe Ciccone)

As it is explained in the glibc manual, the exec like functions don't keep any trace of the calling process: "Executing a new process image completely changes the contents of memory, copying only the argument and environment strings to new locations."

So when we use execvp without any traitment, the multiarch_wrapper exit when the "filename" program finish. As a result, we never reach the end of program (i.e. perror and free).

To use execvp, we must first create a child process with the fork() function. This child process executes the command (filename), and return normally. The parent process waits this return, and we can finish the job.

This is the modified multiarch_wrapper that i propose :

/* multiarch_wrapper.c for CLFS multilib */

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#ifndef USE_ARCH
#define USE_ARCH "64"
#endif

/* Find in libc manual, 26.9.Process creation example. Page 685. */
/* Modified to use execvp instead of execl use in the libc example.*/
/* Execute a program "command", with an array of commands "arg[]" */

int my_system (const char *command, char *const arg[])
{
	int status;
	pid_t pid;
	pid = fork ();
	if (pid == 0)
	{
		/* This is the child process. Execute command with arg[] */
		execvp ( command, arg );
		_exit (EXIT_FAILURE);
	}
	else if (pid < 0)
		/* The fork failed. Report failure. */
		status = -1;
	else
		/* This is the parent process. Wait for the child to complete. */
		if (waitpid (pid, &status, 0) != pid)	/* waitpid return the child pid, status stocks the returned code by child */
			status = -1;
	return status;
}

int main ( int argc, char **argv )
{
	char *filename = NULL;
	char *use_arch = NULL;
	
	if(!(use_arch = getenv("USE_ARCH")))
		use_arch = USE_ARCH;
	
	if (asprintf (&filename, "%s-%s", argv[0], use_arch) < 0)	/* If asprintf fails, return a value < 0, else,
	 									if ok return number of char allocated */
	{
		perror (argv[0]);
		return -1;
	}
	
	if (my_system (filename,argv) == -1)
	{
		perror (argv[0]);
		free (filename);
		return -1;
	}
	
	free (filename);
	
	return 0;
}

It's a longer source, but with this, we keep control over the processes. But in fact, it's a delicate task to execute a child process. Glibc doc precise that the waitpid function is a "cancellation point" in multithreaded programs, but i don't yet know how to solve this potential problem. Since i don't understand what is exactly a "cancellation point".

PS: I'm french, so, exuse me if my ticket isn't in a good english.

This is where can be found the pdf of The GNU C Library Reference Manual: http://www.gnu.org/software/libc/manual/pdf/libc.pdf

Attachments (1)

multiarch_wrapper.c (1.3 KB ) - added by manurootcpp 16 years ago.

Download all attachments as: .zip

Change History (9)

by manurootcpp, 16 years ago

Attachment: multiarch_wrapper.c added

comment:1 by Joe Ciccone, 16 years ago

Description: modified (diff)

comment:2 by Joe Ciccone, 16 years ago

Why would we need to fork a process to execute a single process while we dont need to do anything else? It's a waste of code. Unless I'm completely missing the point for your proposed change. It has the exact same effect.

comment:3 by manurootcpp, 15 years ago

Hello. I apologize for my late answer. The code that i have written has the same effect, but what i wanted to point is that when the multiarch_wrapper(the original one) has finished to execute the "filename" programme, this is the end. So it is also a waste of code to write :

perror(argv[0]);

free(filename);

These lines are never executed. You can see it with valgrind and gdb.

Also, when i have written "This is the modified multiarch_wrapper that i propose", it was just an expression.

comment:4 by Joe Ciccone, 15 years ago

How about this:

#define _GNU_SOURCE

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

#ifndef DEF_SUFFIX
#  define DEF_SUFFIX "64"
#endif

int main(int argc, char **argv)
{
  char *filename;
  char *suffix;

  if(!(suffix = getenv("USE_ARCH")))
    if(!(suffix = getenv("BUILDENV")))
      suffix = DEF_SUFFIX;

  if (asprintf(&filename, "%s-%s", argv[0], suffix) < 0) {
    perror(argv[0]);
    return -1;
  }

  int status = EXIT_FAILURE;
  pid_t pid = fork();

  if (pid == 0) {
    execvp(filename, argv);
    perror(filename);
    goto end;
  } else if (pid < 0) {
    goto end_error;
  } else {
    if (waitpid(pid, &status, 0) != pid) {
      status = EXIT_FAILURE;
      goto end_error;
    }
  }
  goto end;

end_error:
  perror(argv[0]);
end:
  free(filename);

  return status;
}

I noticed the version that you provided was missing a few headers and _GNU_SOURCE but I borrowed the way you used execvp in a fork. I also noticed that the memory still wasn't getting freed when the execvp failed. So I changed some of the error handling. In valgrind it now shows all memory is freed at any place the program can exit.
I also noticed that when the execvp failed to run because the file was missing it would say [blah]-config not found instead of [blah]-config-{32,n32,64}. I fixed that by changing the perror.
I also added the ability to use the BUILDENV enviornment variable to do the same thing, with USE_ARCH superseding it. In stead of a function I used some labels. It made the code a lot smaller and it'd probably be easier to type in for those people not using c&p or ssh.
I'd like to get your opinion on the program before I make the change to the book. I dont think I missed anything, but its nice to have a second set of eyes on it.

comment:5 by Joe Ciccone, 15 years ago

I just found a mistake that had to do with the return value of the child process. The status returned by waitpid can not be used directly as the return value of the process. Other bits are used to store other flags. The fixed version is as follows:

#define _GNU_SOURCE

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

#ifndef DEF_SUFFIX
#  define DEF_SUFFIX "64"
#endif

int main(int argc, char **argv)
{
  char *filename;
  char *suffix;

  if(!(suffix = getenv("USE_ARCH")))
    if(!(suffix = getenv("BUILDENV")))
      suffix = DEF_SUFFIX;

  if (asprintf(&filename, "%s-%s", argv[0], suffix) < 0) {
    perror(argv[0]);
    return -1;
  }

  int status = EXIT_FAILURE;
  pid_t pid = fork();

  if (pid == 0) {
    execvp(filename, argv);
    perror(filename);
    goto end;
  } else if (pid < 0) {
    goto end_error;
  } else {
    if (waitpid(pid, &status, 0) != pid) {
      status = EXIT_FAILURE;
      goto end_error;
    }
    status = WEXITSTATUS(status);
  }
  goto end;

end_error:
  perror(argv[0]);
end:
  free(filename);

  return status;
}

comment:6 by manurootcpp, 15 years ago

Hi! I have just seen your reply. It looks well, and it seems that you have check all the potentials problems, like memory lost. I think you can make the change to the book. I will try it tomorrow, but if there is another problem, it would probably be in a non ordinary situation.

comment:7 by Joe Ciccone, 15 years ago

Resolution: fixed
Status: newclosed

Ok, the book is updated in r5044. If you find something else just open up a new ticket.

Note: See TracTickets for help on using tickets.