Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

record: suggestion for fallback with named pipe creation #1805

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DanielTimLee
Copy link
Contributor

Specifically, some file systems like vboxsf and FAT do not support
mkfifo(). As a result, the 'uftrace record' fails when creating the
.channel file for communication.

# uftrace record -vvv -P . ./spintest
uftrace: running uftrace v0.14-21-gafed ( x86_64 dwarf python3 luajit tui perf sched dynamic )
uftrace: checking binary ./spintest
uftrace: removing uftrace.data.old directory
uftrace: /home/vagrant/uftrace/cmds/record.c:2273:command_record
 ERROR: cannot create a communication channel: Operation not permitted

To address this issue, one proposed solution is creating a fallback
channel
through tmpfs, which does support mkfifo(). In this
suggestion, the .channel file will be named as the following.
/tmp/<dirname>.channel

Close #1804

Specifically, some file systems like vboxsf and FAT do not support
mkfifo(). As a result, the 'uftrace record' fails when creating the
.channel file for communication.

    # uftrace record -vvv -P . ./spintest
    uftrace: running uftrace v0.14-21-gafed ( x86_64 dwarf python3 luajit tui perf sched dynamic  )
    uftrace: checking binary ./spintest
    uftrace: removing uftrace.data.old directory
    uftrace: /home/vagrant/uftrace/cmds/record.c:2273:command_record
     ERROR: cannot create a communication channel: Operation not permitted

This patch, creates a fallback channel through 'tmpfs', which does
support mkfifo(). In this suggestion, the .channel file will be named
as the following. '/tmp/<dirname>.channel'

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
Currently, compiler generates mulitple dependency file with its own
unique identifiers. This commit ignores auxiliary dependency files as
well.

        libtraceevent/.event-parse.d.12994
        libtraceevent/.event-plugin.d.12995
        libtraceevent/.kbuffer-parse.d.13005
        libtraceevent/.parse-filter.d.12999
        libtraceevent/.parse-utils.d.13003
        libtraceevent/.trace-seq.d.12998

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
@DanielTimLee
Copy link
Contributor Author

DanielTimLee commented Aug 18, 2023

To pick up from where we left off, #1804 (comment)
I'm not pretty sure how to handle android case without explicitly specifying additional mechanism at the source.

Using anon pipe might be the case, but as the commit 0b50230 states it's a no-go option.

It used a pipe to communicate each other but it might be closed by a
target process. It'd better using a name pipe so that child can open
during init of libmcount.

Other IPC mechanism may work but performance matters.
With these considerations, stating to the source code looks preferable.

One approach that could be done without explicitly specifying such specific case is using macros.

diff --git a/cmds/record.c b/cmds/record.c
index ec40ca17..05e3d930 100644
--- a/cmds/record.c
+++ b/cmds/record.c
@@ -2095,6 +2095,12 @@ int do_main_loop(int ready, struct uftrace_opts *opts, int pid)

        xasprintf(&channel, "%s/%s", opts->dirname, ".channel");

+#ifndef CHANNEL_PREFIX 
+#define CHANNEL_PREFIX  "/tmp"
+#endif
+       if (access(channel, F_OK) != 0)
+               xasprintf(&channel, CHANNEL_PREFIX "/%s%s", opts->dirname, ".channel");
+
        wd.pid = pid;
        wd.pipefd = open(channel, O_RDONLY | O_NONBLOCK);
cc … -DCHANNEL_PREFIX="@TERMUX_PREFIX@/tmp"

This could be work as a trade-off for adding additional macros relevant to build target.

Opinions would be much appreciated.

@honggyukim
Copy link
Collaborator

Thanks for the upload. I will need to test again in my android phone but need a bit more time. Please wait a little bit more.

@namhyung
Copy link
Owner

Originally it used a (unnamed) pipe by passing a file descriptor via "UFTRACE_PIPE" environ variable. You can check the git history to enable it as a fallback.

@DanielTimLee
Copy link
Contributor Author

DanielTimLee commented Aug 23, 2023

I might not following this properly, but,

Isn't the fallback to (unnamed) pipe will bring up the problem again with the possibility of "pipe ... closed by a target process" as stated at the commit 0b50230 ?

It used a pipe to communicate each other but it might be closed by a
target process. It'd better using a name pipe so that child can open
during init of libmcount.

(I'm not sure what was the exact situation that leads to the close of the (unnamed) pipe back in 0b50230 (v0.9.3).)

@namhyung
Copy link
Owner

For some daemon programs, they want to close all existing file descriptors before start. But I guess it's a rare condition and should be ok to pass a pipe fd for most cases.

@honggyukim
Copy link
Collaborator

We introduced named pipe when we found that there is no renderer process tracing data in chrome browser tracing.

@namhyung
Copy link
Owner

Ah, forgot about that. Yeah so it won't work if the target process active closes all existing file descriptors usually for security reasons. Then I'm ok to fallback to the temp directory, but we can keep the (unnamed) pipe as a last resort (for example, if the temp directory also refuses name pipes).

@DanielTimLee
Copy link
Contributor Author

So far as we speak, the following two approaches should be implemented with this patch:

  1. Fallback with the temp directory
  2. Additional fallback with the (unnamed) pipe

And regarding the first matter, I have question about specifying the path.

As stated earlier #1805 (comment) ,
approach with the macro should bring few changes with the MAKEFILE in order to specify the build target, which in this case would be Termux. Is it acceptable to specify the fallback path with macros, or should it be done differently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

record: suggestion for fallback with named pipe creation
3 participants