All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Sixt <j6t@kdbg.org>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>, Aaron Schrab <aaron@schrab.com>,
	Clemens Buchacher <drizzd@aon.at>,
	David Michael Barr <b@rr-dav.id.au>,
	Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
Subject: Re: [PATCH v2 1/4] run-command: add new check_command helper
Date: Tue, 02 Apr 2013 21:16:59 +0200	[thread overview]
Message-ID: <515B2EAB.10402@kdbg.org> (raw)
In-Reply-To: <1364898709-21583-2-git-send-email-felipe.contreras@gmail.com>

Am 02.04.2013 12:31, schrieb Felipe Contreras:
> And persistent_waitpid() to recover the information from the last run.

I'm not a fan of this new API, because it looks like a workaround
for a problem that should have been solved in a cleaner way. But if
we can't avoid it, please also add a paragraph to
Documentation/technical/api-run-command.txt

> +int check_command(struct child_process *cmd)
> +{
> +	int status;
> +	pid_t waiting;
> +
> +	if (cmd->last_status.valid)
> +		return 0;
> +
> +	while ((waiting = waitpid(cmd->pid, &status, WNOHANG)) < 0 && errno == EINTR)
> +		; /* nothing */
> +
> +	if (!waiting)
> +		return 1;
> +
> +	if (waiting == cmd->pid) {
> +		cmd->last_status.valid = 1;
> +		cmd->last_status.status = status;
> +		return 0;
> +	}
> +
> +	if (waiting > 0)
> +		die("BUG: waitpid reported a random pid?");
> +
> +	return 0;
> +}
> +
>  static void prepare_run_command_v_opt(struct child_process *cmd,
>  				      const char **argv,
>  				      int opt)
> @@ -729,7 +770,7 @@ error:
>  int finish_async(struct async *async)
>  {
>  #ifdef NO_PTHREADS
> -	return wait_or_whine(async->pid, "child process");
> +	return wait_or_whine(cmd, async->pid, "child process");

This breaks the NO_PTHREADS build because cmd is undeclared. Perhaps
this on top:

diff --git a/run-command.c b/run-command.c
index a9fa779..a02ef62 100644
--- a/run-command.c
+++ b/run-command.c
@@ -230,7 +230,7 @@ static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *statu
 {
 	pid_t waiting;
 
-	if (cmd->last_status.valid) {
+	if (cmd && cmd->last_status.valid) {
 		*status = cmd->last_status.status;
 		return pid;
 	}
@@ -771,7 +771,7 @@ int start_async(struct async *async)
 int finish_async(struct async *async)
 {
 #ifdef NO_PTHREADS
-	return wait_or_whine(cmd, async->pid, "child process");
+	return wait_or_whine(NULL, async->pid, "child process");
 #else
 	void *ret = (void *)(intptr_t)(-1);
 

  reply	other threads:[~2013-04-02 19:17 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-02 10:31 [PATCH v2 0/4] run-command: new check_command helper Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 1/4] run-command: add " Felipe Contreras
2013-04-02 19:16   ` Johannes Sixt [this message]
2013-04-02 10:31 ` [PATCH v2 2/4] transport-helper: check if remote helper is alive Felipe Contreras
2013-04-03  0:35   ` Junio C Hamano
2013-04-02 10:31 ` [PATCH v2 3/4] tmp: remote-helper: add timers to catch errors Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 4/4] tmp: run-command: code to exercise check_command Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=515B2EAB.10402@kdbg.org \
    --to=j6t@kdbg.org \
    --cc=aaron@schrab.com \
    --cc=b@rr-dav.id.au \
    --cc=drizzd@aon.at \
    --cc=felipe.contreras@gmail.com \
    --cc=florian.achleitner.2.6.31@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.