| Message ID | patch-2.5-a411098699d-20211122T153605Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | run-command API: get rid of "argv" | expand |
On Mon, Nov 22, 2021 at 05:04:04PM +0100, Ævar Arnfjörð Bjarmason wrote: > This pattern added [1] in seems to have been intentional, but since > [2] and [3] we've wanted do initialization of what's now the "struct > strvec" "args" and "env_array" members. Let's not trample on that > initialization here. Yes, I came across this one, too, while looking at the same topic. I think this is a good change, but: > int cmd_upload_archive(int argc, const char **argv, const char *prefix) > { > - struct child_process writer = { argv }; > + struct child_process writer = CHILD_PROCESS_INIT; > > if (argc == 2 && !strcmp(argv[1], "-h")) > usage(upload_archive_usage); > @@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) > argv[0] = "upload-archive--writer"; You can drop this assignment over argv[0] now. > writer.out = writer.err = -1; > writer.git_cmd = 1; > + strvec_push(&writer.args, "upload-archive--writer"); > + if (argc > 1) > + strvec_pushv(&writer.args, &argv[1]); The "argc > 1" isn't necessary. If it is not true, strvec_pushv() will see the terminating NULL in the list and just become a noop. (I'd also spell it "argv + 1" to make it more obvious that we are interested in the list and not a single element, but that may be a matter of taste). -Peff
On Mon, Nov 22 2021, Ævar Arnfjörð Bjarmason wrote: > This pattern added [1] in seems to have been intentional, but since > [2] and [3] we've wanted do initialization of what's now the "struct > strvec" "args" and "env_array" members. Let's not trample on that > initialization here. > > 1. 1bc01efed17 (upload-archive: use start_command instead of fork, > 2011-11-19) > 2. c460c0ecdca (run-command: store an optional argv_array, 2014-05-15) > 3. 9a583dc39e (run-command: add env_array, an optional argv_array for > env, 2014-10-19) > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/upload-archive.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c > index 24654b4c9bf..b4b9b3a6262 100644 > --- a/builtin/upload-archive.c > +++ b/builtin/upload-archive.c > @@ -77,7 +77,7 @@ static ssize_t process_input(int child_fd, int band) > > int cmd_upload_archive(int argc, const char **argv, const char *prefix) > { > - struct child_process writer = { argv }; > + struct child_process writer = CHILD_PROCESS_INIT; > > if (argc == 2 && !strcmp(argv[1], "-h")) > usage(upload_archive_usage); > @@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) > argv[0] = "upload-archive--writer"; > writer.out = writer.err = -1; > writer.git_cmd = 1; > + strvec_push(&writer.args, "upload-archive--writer"); > + if (argc > 1) > + strvec_pushv(&writer.args, &argv[1]); > if (start_command(&writer)) { > int err = errno; > packet_write_fmt(1, "NACK unable to spawn subprocess\n"); Changing it to argv + 1 makes sense, I don't know why I did it like that. It works, but is unusual. But as to skipping the "argc > 1" test I've got this still: @@ -89,9 +89,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) * multiplexed out to our fd#1. If the child dies, we tell the other * end over channel #3. */ - argv[0] = "upload-archive--writer"; writer.out = writer.err = -1; writer.git_cmd = 1; + strvec_push(&writer.args, "upload-archive--writer"); + if (argc > 1) + strvec_pushv(&writer.args, argv + 1); if (start_command(&writer)) { int err = errno; packet_write_fmt(1, "NACK unable to spawn subprocess\n"); We'll segfault if we give NULL to strvec_pushv() so we still need that check. Were you thinking of strvec_pushl(), or am I missing something? (Even with strvec_pushl() we can't interpolate it as-is into the list, since it'll point to uninitialized memory in this case).
On Mon, Nov 22, 2021 at 09:53:34PM +0100, Ævar Arnfjörð Bjarmason wrote: > But as to skipping the "argc > 1" test I've got this still: > > @@ -89,9 +89,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) > * multiplexed out to our fd#1. If the child dies, we tell the other > * end over channel #3. > */ > - argv[0] = "upload-archive--writer"; > writer.out = writer.err = -1; > writer.git_cmd = 1; > + strvec_push(&writer.args, "upload-archive--writer"); > + if (argc > 1) > + strvec_pushv(&writer.args, argv + 1); > if (start_command(&writer)) { > int err = errno; > packet_write_fmt(1, "NACK unable to spawn subprocess\n"); > > We'll segfault if we give NULL to strvec_pushv() so we still need that > check. Were you thinking of strvec_pushl(), or am I missing something? We wouldn't be giving NULL to strvec_pushv(). We'd be giving argv+1, which is guaranteed non-NULL, but may point to NULL. In that case the loop condition in strvec_pushv() would turn it into a noop. -Peff
On Mon, Nov 22 2021, Jeff King wrote: > On Mon, Nov 22, 2021 at 09:53:34PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> But as to skipping the "argc > 1" test I've got this still: >> >> @@ -89,9 +89,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) >> * multiplexed out to our fd#1. If the child dies, we tell the other >> * end over channel #3. >> */ >> - argv[0] = "upload-archive--writer"; >> writer.out = writer.err = -1; >> writer.git_cmd = 1; >> + strvec_push(&writer.args, "upload-archive--writer"); >> + if (argc > 1) >> + strvec_pushv(&writer.args, argv + 1); >> if (start_command(&writer)) { >> int err = errno; >> packet_write_fmt(1, "NACK unable to spawn subprocess\n"); >> >> We'll segfault if we give NULL to strvec_pushv() so we still need that >> check. Were you thinking of strvec_pushl(), or am I missing something? > > We wouldn't be giving NULL to strvec_pushv(). We'd be giving argv+1, > which is guaranteed non-NULL, but may point to NULL. In that case the > loop condition in strvec_pushv() would turn it into a noop. D'oh. Thanks. Yes I was stupidly conflating a case where I'd tested it with strvec_pushl() and moved that dereferenced version to strvec_pushv(), sorry.
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c index 24654b4c9bf..b4b9b3a6262 100644 --- a/builtin/upload-archive.c +++ b/builtin/upload-archive.c @@ -77,7 +77,7 @@ static ssize_t process_input(int child_fd, int band) int cmd_upload_archive(int argc, const char **argv, const char *prefix) { - struct child_process writer = { argv }; + struct child_process writer = CHILD_PROCESS_INIT; if (argc == 2 && !strcmp(argv[1], "-h")) usage(upload_archive_usage); @@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) argv[0] = "upload-archive--writer"; writer.out = writer.err = -1; writer.git_cmd = 1; + strvec_push(&writer.args, "upload-archive--writer"); + if (argc > 1) + strvec_pushv(&writer.args, &argv[1]); if (start_command(&writer)) { int err = errno; packet_write_fmt(1, "NACK unable to spawn subprocess\n");
This pattern added [1] in seems to have been intentional, but since [2] and [3] we've wanted do initialization of what's now the "struct strvec" "args" and "env_array" members. Let's not trample on that initialization here. 1. 1bc01efed17 (upload-archive: use start_command instead of fork, 2011-11-19) 2. c460c0ecdca (run-command: store an optional argv_array, 2014-05-15) 3. 9a583dc39e (run-command: add env_array, an optional argv_array for env, 2014-10-19) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/upload-archive.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)