Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stefan Beller <sbeller@xxxxxxxxxx> writes:

> On Thu, Nov 5, 2015 at 10:45 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>>
>>> strbuf_read_once can also operate on blocking file descriptors if we are
>>> sure they are ready. The poll (2) command however makes sure this is the
>>> case.
>>>
>>> Reading the manual for poll (2), there may be spurious returns indicating
>>> readiness but that is for network sockets only. Pipes should be unaffected.
>>
>> Given the presence of "for example" in that bug section, I wouldn't
>> say "only" or "should be unaffected".
>
> Reading the documentation we are in agreement, that we expect
> no spurious returns, no?

Given the presence of "for example" in that bug section, I wouldn't
say "only" or "should be unaffected".  I cannot say "we expect no
spurious returns".

>> Thanks.  Let's apply these fixes on sb/submodule-parallel-fetch,
>> merge the result to 'next' and have people play with it.
>
> Maybe the commit message was weakly crafted. Do you want me to resend?

I somehow feel that it is prudent to let this cook just above 'next'
for a few days (not just for the log message but to verify the
strategy and wait for others to come up with even better ideas), but
then I'll be offline starting next week, so I expect that merging
the final version to 'next' will be done by our interim maintainer,
which means we still have time to polish ;-)

Here is what I queued for now.

-- >8 --
From: Stefan Beller <sbeller@xxxxxxxxxx>
Date: Thu, 5 Nov 2015 10:17:18 -0800
Subject: [PATCH] run-command: remove set_nonblocking()

strbuf_read_once can also operate on blocking file descriptors if we
are sure they are ready.  And the poll(2) we call before calling
this ensures that this is the case.

Reading the manual for poll(2), there may be spurious returns
indicating readiness but that is for network sockets only and pipes
should be unaffected.

With this change, we rely on

 - poll(2) returns only non-empty pipes; and
 - read(2) on a non-empty pipe does not block.

This should fix compilation on Windows.

Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
 run-command.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1fbd286..07424e9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -996,17 +996,6 @@ static void pp_cleanup(struct parallel_processes *pp)
 	sigchain_pop_common();
 }
 
-static void set_nonblocking(int fd)
-{
-	int flags = fcntl(fd, F_GETFL);
-	if (flags < 0)
-		warning("Could not get file status flags, "
-			"output will be degraded");
-	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-		warning("Could not set file status flags, "
-			"output will be degraded");
-}
-
 /* returns
  *  0 if a new task was started.
  *  1 if no new jobs was started (get_next_task ran out of work, non critical
@@ -1042,8 +1031,6 @@ static int pp_start_one(struct parallel_processes *pp)
 		return code ? -1 : 1;
 	}
 
-	set_nonblocking(pp->children[i].process.err);
-
 	pp->nr_processes++;
 	pp->children[i].in_use = 1;
 	pp->pfd[i].fd = pp->children[i].process.err;
-- 
2.6.2-539-g1c5cd50

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]