Re: [PATCH 3/5] run-command: Elaborate execvp error checking

From: Johannes Sixt <j6t@kdbg.org>
Date: Jan 25, 2012, 19:03 UTC
Message-ID: <4F205212.5080007@kdbg.org>
In-Reply-To: <1327444346-6243-4-git-send-email-fransklaver@gmail.com>

Am 24.01.2012 23:32, schrieb Frans Klaver:

Show 13 quoted lines

> +static void inspect_failure(const char *argv0, int silent_exec_failure)
> +{
> +	int err = errno;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	/* errors not related to path */
> +	if (errno == E2BIG || errno == ENOMEM)
> +		die_file_error(argv0, err);
> +
> +	if (strchr(argv0, '/')) {
> +		if (file_exists(argv0)) {
> +			strbuf_add(&sb, argv0, strlen(argv0));
> +			inspect_file(&sb, err, argv0);

Can we end up here if errno == ENOENT? If so, silent_exec_failure must be checked. (inspect_file does not return.)

Show 59 quoted lines

> +		}
> +	} else {
> +		char *path, *next;
> +		path = getenv("PATH");
> +		while (path) {
> +			next = strchrnul(path, ':');
> +			if (path < next)
> +				strbuf_add(&sb, path, next - path);
> +			else
> +				strbuf_addch(&sb, '.');
> +
> +			if (!*next)
> +				path = NULL;
> +			else
> +				path = next + 1;
> +
> +			if (!is_searchable(sb.buf)) {
> +				strbuf_release(&sb);
> +				continue;
> +			}
> +
> +			if (sb.len && sb.buf[sb.len - 1] != '/')
> +				strbuf_addch(&sb, '/');
> +			strbuf_addstr(&sb, argv0);
> +
> +			if (file_exists(sb.buf))
> +				inspect_file(&sb, err, argv0);
> +
> +			strbuf_release(&sb);
> +		}
> +	}
> +
> +	if (err == ENOENT) {
> +		if (!silent_exec_failure)
> +			error("cannot exec '%s': %s", argv0,
> +					strerror(ENOENT));
> +		exit(127);
> +	} else {
> +		die_file_error(argv0, err);
> +	}
> +}
> +#endif
> +
>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
> @@ -280,14 +415,7 @@ fail_pipe:
>  		} else {
>  			execvp(cmd->argv[0], (char *const*) cmd->argv);
>  		}
> -		if (errno == ENOENT) {
> -			if (!cmd->silent_exec_failure)
> -				error("cannot run %s: %s", cmd->argv[0],
> -					strerror(ENOENT));
> -			exit(127);
> -		} else {
> -			die_errno("cannot exec '%s'", cmd->argv[0]);
> -		}
> +		inspect_failure(cmd->argv[0], cmd->silent_exec_failure);

Isn't it important that this function calls exit(127) if we want silent_exec_failure and errno == ENOENT? But I don't see that this guaranteed by inspect_failure; see above.

>  	}
>  	if (cmd->pid < 0)
>  		error("cannot fork() for %s: %s", cmd->argv[0],

-- Hannes