fix: make monitor more readable #3702
Conversation
anoushk1234
force-pushed
the
anoushk1234/sep-monitor-panes
branch
from
2c11519 to
b1bc7f1
Compare
|
Hey @ripatel-fd did you get a chance to review this yet? |
|
Also the sandbox prevents reading STDIN preventing tab switching in the monitor, is there a way to allow that for non dev builds without disabling the sandbox? |
Signed-off-by: Anoushk Kharangate <[email protected]>
anoushk1234
force-pushed
the
anoushk1234/sep-monitor-panes
branch
from
b1bc7f1 to
84fd7c0
Compare
@anoushk1234 Thank you for the contribution! I tagged @mmcgee-jump for review.
Yes, the syscall policy for the monitor is defined in |
Thanks! @ripatel-fd Let me push the updated policy |
Great, thanks! I forgot to mention, could you please also run |
src/app/fdctl/monitor/helper.c
Outdated
| int | ||
| fd_getch() | ||
| { | ||
| struct termios oldt, newt; | ||
| int ch; | ||
| int oldf; | ||
| /* Disables character echo and canonical mode since we want the input to be processes immediately. | ||
| * Terminal also set to non blocking in case the user doesn't send any input. | ||
| * */ | ||
| tcgetattr(STDIN_FILENO, &oldt); | ||
| newt = oldt; | ||
| newt.c_lflag &= (tcflag_t)~(ICANON | ECHO); | ||
| tcsetattr(STDIN_FILENO, TCSANOW, &newt); | ||
| oldf = fcntl(STDIN_FILENO, F_GETFL, 0); | ||
| fcntl(STDIN_FILENO, F_SETFL, oldf | O_NONBLOCK); | ||
|
|
||
| ch = getchar(); | ||
|
|
||
| /*Restore the terminal back to it's original configuration*/ | ||
| tcsetattr(STDIN_FILENO, TCSANOW, &oldt); | ||
| fcntl(STDIN_FILENO, F_SETFL, oldf); | ||
|
|
||
| return ch; | ||
| } |
There was a problem hiding this comment.
A couple code style suggestions, some of which are in https://github.com/firedancer-io/firedancer/blob/main/CONTRIBUTING.md
- We follow the C strict prototypes rule where we use
voidto signal a blank argument list. - The curly goes on the same line as the method definition
int
fd_getch( void ) {- Comment style looks like this:
/* Disables character echo and canonical mode since we want the input to be processes immediately.
Terminal also set to non blocking in case the user doesn't send any input. */- Variable declarations go as close as possible to the definition
- libc library functions should do error handling
- Spaces inside brackets when doing control flow statements and function calls
struct termios oldt;
if( FD_UNLIKELY( 0!=tcgetattr( STDIN_FILENO, &oldt ) ) ) {
... handle error ...
}
struct termios newt = oldt;
newt.c_lflag &= (tcflag_t)~(ICANON | ECHO);There was a problem hiding this comment.
@ripatel-fd thanks for the points regarding the style, I'll be more careful to stick to it next time. Given we are not resetting the terminal back to it's old state now, there isn't a good reason to have a separate function for this and I can just move the tcsetattr code into monitor.c before the rendering loop starts.
There was a problem hiding this comment.
sidenote: in your example if( FD_UNLIKELY( 0!=tcgetattr( STDIN_FILENO, &oldt ) ) ) tcgetattr has spaces within the brackets but in the style it doesn't:
memcpy( dst, src, sizeof(fd_rng_t) ); /* good */
memcpy( dst, src, sizeof( fd_rng_t ) ); /* WRONG! */
There was a problem hiding this comment.
Good point. sizeof is an operator, not a function call. So we usually don't use spaces there
There was a problem hiding this comment.
Good point. sizeof is an operator, not a function call. So we usually don't use spaces there
Makes sense, this example needs to be updated then.
Co-authored-by: ripatel-fd <[email protected]>
|
Hey @ripatel-fd made the changes, apologies for the delay, I was out of office. Would appreciate another review ( Don't mind the multiple commits, I'll rebase it all into one once the PR is approved ) Regarding the seccomp policy I add |
anoushk1234
force-pushed
the
anoushk1234/sep-monitor-panes
branch
2 times, most recently
from
4e41a01 to
7a06199
Compare
anoushk1234
force-pushed
the
anoushk1234/sep-monitor-panes
branch
from
7a06199 to
f88cf8d
Compare
Fixes #3676