| Message ID | xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | avoid running "git-subcmd" in the dashed form | expand |
On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote: > diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c > index d0fafdeb9e..195335a783 100644 > --- a/builtin/credential-cache.c > +++ b/builtin/credential-cache.c > @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out) > static void spawn_daemon(const char *socket) > { > struct child_process daemon = CHILD_PROCESS_INIT; > - const char *argv[] = { NULL, NULL, NULL }; > char buf[128]; > int r; > > - argv[0] = "git-credential-cache--daemon"; > - argv[1] = socket; > - daemon.argv = argv; > + strvec_pushl(&daemon.args, > + "credential-cache--daemon", socket, > + NULL); > + daemon.git_cmd = 1; Yep, this makes sense. I don't recall any reason to use the dashed form back then, but probably it was just that I knew it was a separate program. Doing it this way will mean an extra parent "git" process hanging around, but I don't think it's that big a deal. We never try to kill it by PID, etc (instead we connect to the socket and ask it to exit). And anyway, it is becoming a builtin in a parallel topic, so that extra process will go away. :) -Peff
On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote: > As child_process structure has an embedded strvec args for > formulating the command line, let's use it instead of using > an out-of-line argv[] whose length needs to be maintained > correctly. I forgot to mention in my other reply: I think this fails to mention the actual functional change, which is switching from the dashed form to using the "git" wrapper. -Peff
On Thu, Aug 27, 2020 at 12:13:28AM -0400, Jeff King wrote: > On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote: > > > diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c > > index d0fafdeb9e..195335a783 100644 > > --- a/builtin/credential-cache.c > > +++ b/builtin/credential-cache.c > > @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out) > > static void spawn_daemon(const char *socket) > > { > > struct child_process daemon = CHILD_PROCESS_INIT; > > - const char *argv[] = { NULL, NULL, NULL }; > > char buf[128]; > > int r; > > > > - argv[0] = "git-credential-cache--daemon"; > > - argv[1] = socket; > > - daemon.argv = argv; > > + strvec_pushl(&daemon.args, > > + "credential-cache--daemon", socket, > > + NULL); > > + daemon.git_cmd = 1; > > Yep, this makes sense. I don't recall any reason to use the dashed form > back then, but probably it was just that I knew it was a separate > program. Doing it this way will mean an extra parent "git" process > hanging around, but I don't think it's that big a deal. We never try to > kill it by PID, etc (instead we connect to the socket and ask it to > exit). And anyway, it is becoming a builtin in a parallel topic, so that > extra process will go away. :) ...and if I had read the diff header more carefully, I'd see that you did this on top of that topic. :) -Peff
Jeff King <peff@peff.net> writes: > Yep, this makes sense. I don't recall any reason to use the dashed form > back then, but probably it was just that I knew it was a separate > program. Doing it this way will mean an extra parent "git" process > hanging around, but I don't think it's that big a deal. We never try to > kill it by PID, etc (instead we connect to the socket and ask it to > exit). And anyway, it is becoming a builtin in a parallel topic, so that > extra process will go away. :) Yeah, this was discovered when I tentatively merged that "don't run built-in as git-foo" to the tip of seen. Without your slimmed-down topic, it wouldn't have been caught. Likewise for remote-ext.
Jeff King <peff@peff.net> writes: > On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote: > >> As child_process structure has an embedded strvec args for >> formulating the command line, let's use it instead of using >> an out-of-line argv[] whose length needs to be maintained >> correctly. > > I forgot to mention in my other reply: I think this fails to mention the > actual functional change, which is switching from the dashed form to > using the "git" wrapper. True. With an extra paragraph. -- >8 -- Subject: [PATCH] credential-cache: use child_process.args As child_process structure has an embedded strvec args for formulating the command line, let's use it instead of using an out-of-line argv[] whose length needs to be maintained correctly. Also, when spawning a git subcommand, omit it from the command list and instead use the .git_cmd bit in the child_process structure. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- credential-cache.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/credential-cache.c b/credential-cache.c index 1cccc3a0b9..04df61cf02 100644 --- a/credential-cache.c +++ b/credential-cache.c @@ -39,13 +39,13 @@ static int send_request(const char *socket, const struct strbuf *out) static void spawn_daemon(const char *socket) { struct child_process daemon = CHILD_PROCESS_INIT; - const char *argv[] = { NULL, NULL, NULL }; char buf[128]; int r; - argv[0] = "git-credential-cache--daemon"; - argv[1] = socket; - daemon.argv = argv; + strvec_pushl(&daemon.args, + "credential-cache--daemon", socket, + NULL); + daemon.git_cmd = 1; daemon.no_stdin = 1; daemon.out = -1;
Junio C Hamano <gitster@pobox.com> writes: > As child_process structure has an embedded strvec args for > formulating the command line, let's use it instead of using > an out-of-line argv[] whose length needs to be maintained > correctly. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > builtin/credential-cache.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c > index d0fafdeb9e..195335a783 100644 > --- a/builtin/credential-cache.c > +++ b/builtin/credential-cache.c > @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out) > static void spawn_daemon(const char *socket) > { > struct child_process daemon = CHILD_PROCESS_INIT; > - const char *argv[] = { NULL, NULL, NULL }; > char buf[128]; > int r; > > - argv[0] = "git-credential-cache--daemon"; > - argv[1] = socket; > - daemon.argv = argv; > + strvec_pushl(&daemon.args, > + "credential-cache--daemon", socket, > + NULL); > + daemon.git_cmd = 1; > daemon.no_stdin = 1; > daemon.out = -1; > By the way, an interesting fact is that this cannot graduate UNTIL credential-cache becomes a built-in. Having an intermediate level process seems to break t0301.
On Mon, Aug 31, 2020 at 03:56:00PM -0700, Junio C Hamano wrote: > > - argv[0] = "git-credential-cache--daemon"; > > - argv[1] = socket; > > - daemon.argv = argv; > > + strvec_pushl(&daemon.args, > > + "credential-cache--daemon", socket, > > + NULL); > > + daemon.git_cmd = 1; > > daemon.no_stdin = 1; > > daemon.out = -1; > > > > By the way, an interesting fact is that this cannot graduate UNTIL > credential-cache becomes a built-in. Having an intermediate level > process seems to break t0301. Hmm, that is interesting. I thought it would work OK because we don't rely on any process-id magic for finding the daemon, etc, and instead talk over pipe descriptors. But that proves to be our undoing. What happens usually is this: - credential-cache spawns credential-cache--daemon with its stdout connected to a pipe - the client calls read_in_full() waiting for the daemon to tell us that it started up - the daemon opens the socket, then writes "ok\n" to stdout and closes the pipe - the client gets EOF on the pipe, then compares what it read to "ok\n", and continues (or relays an error) But when we add the extra "git" wrapper process into the mix, we never see that EOF. The wrapper's stdout also points to that pipe, so closing it in the daemon process isn't enough for the client to get EOF. And the wrapper doesn't exit, because it's waiting on the daemon. So one hacky fix is: diff --git a/credential-cache.c b/credential-cache.c index 04df61cf02..9bfddaf050 100644 --- a/credential-cache.c +++ b/credential-cache.c @@ -51,7 +51,7 @@ static void spawn_daemon(const char *socket) if (start_command(&daemon)) die_errno("unable to start cache daemon"); - r = read_in_full(daemon.out, buf, sizeof(buf)); + r = read_in_full(daemon.out, buf, 3); if (r < 0) die_errno("unable to read result code from cache daemon"); if (r != 3 || memcmp(buf, "ok\n", 3)) which makes t0301 pass. A less-hacky solution might be to loosen its expectations not to require EOF at all (and just accept anything starting with "ok\n". But I don't think it's worth doing either, if we know we're going to switch it to a builtin anyway (and that also makes me feel slightly better about the plan to do so). I do wonder if it points to a problem in the git.c wrapper. It's duplicating all of the descriptors that the child external commands will see, so callers can't rely on EOF (or EPIPE for its stdin) to know when the external program has closed them. For the most part that's OK, because we expect them to close when the external program exits, at which point the wrapper will exit, too. But things get weird around half-duplex shutdowns, or programs that try to daemonize. Perhaps git.c should be closing all descriptors after spawning the child. Of course that gets weird if it wants to write an error to stderr about spawning the child. I dunno. It seems as likely to introduce problems as solve them, so if nothing is broken beyond this cache-daemon thing, I'd just as soon leave it. I wouldn't be surprised if older pre-builtin "upload-pack" could run into problems. But we always called it as "git-upload-pack" back then anyway. Possibly modern "git daemon" could suffer weirdness, as it's still a separate program (I shied away from including it in my recent "slimming" series exactly because I was afraid of these kinds of issues; but it sounds like being a builtin probably has less-surprising implications overall). All of which is to say I'm happy to do nothing, but that turned out to be a very interesting data point. Thanks for mentioning it. :) -Peff
Jeff King <peff@peff.net> writes: > Hmm, that is interesting. I thought it would work OK because we don't > rely on any process-id magic for finding the daemon, etc, and instead > talk over pipe descriptors. But that proves to be our undoing. Yup. I also do suspect that closing all excess fds in the git process in the middle may not be a bad idea, but we may not know what the upper bound is. > Perhaps git.c should be closing all descriptors after spawning the > child. Of course that gets weird if it wants to write an error to stderr > about spawning the child. I dunno. It seems as likely to introduce > problems as solve them, so if nothing is broken beyond this cache-daemon > thing, I'd just as soon leave it. We do employ the "open extra pipe that is set to close-on-exec so that child that failed to exec can report back" trick, but in order to report the failure back, the standard error of the process in the middle may have to be kept open, so let's not disturb this sleeping dragon ;-)
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c index d0fafdeb9e..195335a783 100644 --- a/builtin/credential-cache.c +++ b/builtin/credential-cache.c @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out) static void spawn_daemon(const char *socket) { struct child_process daemon = CHILD_PROCESS_INIT; - const char *argv[] = { NULL, NULL, NULL }; char buf[128]; int r; - argv[0] = "git-credential-cache--daemon"; - argv[1] = socket; - daemon.argv = argv; + strvec_pushl(&daemon.args, + "credential-cache--daemon", socket, + NULL); + daemon.git_cmd = 1; daemon.no_stdin = 1; daemon.out = -1;
As child_process structure has an embedded strvec args for formulating the command line, let's use it instead of using an out-of-line argv[] whose length needs to be maintained correctly. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/credential-cache.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)