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

fix redirect when reuse fd #261

Merged
merged 1 commit into from
Jul 3, 2024
Merged

fix redirect when reuse fd #261

merged 1 commit into from
Jul 3, 2024

Conversation

ChrisCatCP
Copy link
Contributor

No description provided.

@ChrisCatCP ChrisCatCP force-pushed the redirect branch 2 times, most recently from 668f3ff to 86005dd Compare July 3, 2024 05:56
@ChrisCatCP
Copy link
Contributor Author

已经修改了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


It has been modified

@@ -300,6 +300,10 @@ static tb_process_ref_t tb_process_init_spawn(tb_char_t const* pathname, tb_char
// set attributes
if (attr)
{
tb_int_t oriin = -1;
tb_int_t oriout = -1;
tb_int_t orierr = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

直接把里面的 infd, outfd 定义,移到外面不就好了么。。为啥还要再搞一坨变量

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

下面 openfile的地方 也是用的infd 这几个变量 要挪到外面还得改那几个 所以我就新开了三个

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

哪?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 497

                    // open file
                    tb_int_t infd = open(attr->in.path, tb_process_file_flags(attr->inmode), tb_process_file_modes(attr->inmode));
                    tb_assertf_pass_and_check_break(infd, "cannot redirect stdin to file: %s, error: %d", attr->in.path, errno);

这里

@ChrisCatCP
Copy link
Contributor Author

去掉空行了 顺便问下 项目不准备加.clang-format吗

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


The blank lines have been removed. By the way, are you not going to add .clang-format to the project?

@waruqi
Copy link
Member

waruqi commented Jul 3, 2024

还是没懂,为啥要额外加三,不能直接挪外面定义。。另外 什么情况下 infd 会等于 outfd?

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


I still don’t understand why we need to add an extra three. We can’t just move the definition outside. . In addition, under what circumstances will infd equal outfd?

@waruqi
Copy link
Member

waruqi commented Jul 3, 2024

去掉空行了 顺便问下 项目不准备加.clang-format吗

如果你能整出一个完全符合当前风格的 format ,那可以加,前提是要完全风格一致。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


Removed the blank lines. By the way, are you not going to add .clang-format to the project?

If you can create a format that completely matches the current style, you can add it, provided that the style is completely consistent.

@ChrisCatCP
Copy link
Contributor Author

感觉还得你自己加才行

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


I feel like you have to add it yourself.

@ChrisCatCP
Copy link
Contributor Author

还是没懂,为啥要额外加三,不能直接挪外面定义。。另外 什么情况下 infd 会等于 outfd?

额外加三个是因为不想改497 行那边的变量名 不然还是得加三个变量名 差不多的
in和out err 一般情况下确实不会相同吧 但是感觉还是判断下好

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


I still don’t understand why we need to add an extra three. We can’t just move the definition outside. . In addition, under what circumstances will infd equal outfd?

The reason I added three extra variables is because I don’t want to change the variable name on line 497. Otherwise, I still have to add three variable names. It’s almost the same.
In and out err are indeed not the same under normal circumstances, but it feels like it is better to make a judgment.

@waruqi
Copy link
Member

waruqi commented Jul 3, 2024

额外加三个是因为不想改497 行那边的变量名 不然还是得加三个变量名 差不多的

为啥要改名,同样用 infd ,又不可能同时存在 file 和 pipe 的 infd,而且都是要 close 的。。而且 openfile 的 三个 fd ,也不可能重复,即使重复,走的 close 逻辑也是跟 pipe 一样的。。完全可以一起在外面定义。

in和out err 一般情况下确实不会相同吧 但是感觉还是判断下好

既然不可能相同,干嘛还要去做冗余的无意义判断。。而且目前的 if 判断太复杂了,能尽可能精简尽量精简

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


I added three extra ones because I don’t want to change the variable name on line 497, otherwise I still have to add three variable names. It’s almost the same.

Why do we need to change the name? We also use infd, and it is impossible to have infd for file and pipe at the same time, and they both need to be closed. . Moreover, the three fds of openfile cannot be repeated. Even if they are repeated, the close logic is the same as that of pipe. . It can be defined outside together.

In and out err are generally not the same, but it feels better to make a judgment.

Since it can't be the same, why bother making redundant and meaningless judgments. . Moreover, the current if judgment is too complicated. It can be simplified as much as possible.

@waruqi
Copy link
Member

waruqi commented Jul 3, 2024

感觉还得你自己加才行

我就是因为懒得加,所以没加。。没这么多时间折腾这个,反正自己敲代码都是手动 format 的,自己完全没这个需求,除非有现成 pr 过来。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


I feel like you have to add it yourself

I just didn't add it because I was too lazy to add it. . I don’t have so much time to bother with this. Anyway, I type the code manually and format it. I have no need for this at all, unless there is a ready-made PR.

@ChrisCatCP
Copy link
Contributor Author

修改了 看了下可能 确实in和out err不可能相等 已经都改过来了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


Modified. After looking at the possibility, it is true that in and out err cannot be equal. They have been changed.

@waruqi waruqi merged commit 8ada0ab into tboox:dev Jul 3, 2024
13 checks passed
@waruqi
Copy link
Member

waruqi commented Jul 3, 2024

merge 了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


Merged

@ChrisCatCP ChrisCatCP deleted the redirect branch July 3, 2024 09:01
@ChrisCatCP
Copy link
Contributor Author

好的 好的

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


ok ok

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.

3 participants