CP-45795: Decompress compressed trace files without Forkexecd #6293
Conversation
snwoods
force-pushed
the
private/stevenwo/CP-45795
branch
from
5dd6cf6 to
69bb471
Compare
|
I'd rather wait for the changes in forkexec to be merged rather than special-casing this one case. |
Ah, good point, I'd forgotten about that... |
|
Reopening as discussed in the standup: as xs_trace needs to be portable, it would be best to remove the dependency on forkexec and the dependencies used in the new PR e.g. vfork. |
snwoods
force-pushed
the
private/stevenwo/CP-45795
branch
from
69bb471 to
a106203
Compare
| | Unix.WEXITED 0 -> | ||
| () | ||
| | _ -> | ||
| Printf.eprintf "File %s exited with non-zero\n" path |
There was a problem hiding this comment.
I would prefer to use open_process_args_in as this is currently a string that is interpreted by the shell and that always has the risks of funny problems with spaces in names and so on.
There was a problem hiding this comment.
I don't quite understand the difference between the two. I thought I should just be able to change it to:
let ic = Unix.open_process_args_in "zstdcat" [|path|] in
but it gives the error stdin is a console, aborting when the original code does not have this issue.
There was a problem hiding this comment.
Even the documentation says Unix.open_process_args_in is a safe replacement. I assume that stdin is handled differently by the shell (underlying open_proess and zstdcat. What stdin does the error even refer to?
There was a problem hiding this comment.
You will need to satisfy the convention that argv[0] is the program's name/path when using Unix.open_process_args_in. The usual convention is:
let args = [|"zstdcat"; ...|] in
let ic = Unix.open_process_args_in args.(0) args in
...The error you're getting is the same as if you just executed zstdcat with no user-supplied arguments from a shell:
~ zstdcat
stdin is a console, aborting
There was a problem hiding this comment.
Where is ic closed?
It's closed on line 41 match Unix.close_process_in ic with
There was a problem hiding this comment.
You will need to satisfy the convention that argv[0] is the program's name/path
Ahhh of course. I even read that statement but I think I glossed over it because I'm obviously passing the process name already, but it needs to be in args too! So it's assuming my first arg (path) is the name and therefore I haven't provided any filenames...
There was a problem hiding this comment.
I think the change is now understood; can we change this and merge then?
There was a problem hiding this comment.
I'm sorry, I wanted to test the change first and Jaeger was experiencing unrelated flakiness. Tried again now and it's all good 👍
|
I stopped this from being merged. We want this to use array-based arguments as discussed. |
snwoods
force-pushed
the
private/stevenwo/CP-45795
branch
from
a106203 to
903d8f0
Compare
|
The DCO checks fails - so need an update @snwoods |
This allows an xs_trace.exe to be run on any linux machine, whereas previously the use of forkexecd meant that it had to be run on a XS host. Signed-off-by: Steven Woods <steven.woods@citrix.com>
snwoods
force-pushed
the
private/stevenwo/CP-45795
branch
from
903d8f0 to
c608902
Compare
This allows an xs_trace.exe to be run on any linux machine, whereas previously the use of forkexecd meant that it had to be run on a XS host. It also allows the logic to be simplified (json and ndjson can be handled in the same way).