fix: make monitor more readable #3702

anoushk1234 · 2024-12-14T10:17:06Z

anoushk1234 · 2024-12-17T13:23:13Z

Hey @ripatel-fd did you get a chance to review this yet?

anoushk1234 · 2024-12-17T13:49:33Z

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]>

ripatel-fd · 2024-12-18T14:57:59Z

Hey @ripatel-fd did you get a chance to review this yet?

@anoushk1234 Thank you for the contribution! I tagged @mmcgee-jump for review.

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?

Yes, the syscall policy for the monitor is defined in src/app/fdctl/monitor/monitor.seccomppolicy.
You'd probably want to amend the expression for the read syscall to allow file descriptor 0 (stdin).

anoushk1234 · 2024-12-19T09:47:25Z

Hey @ripatel-fd did you get a chance to review this yet?

@anoushk1234 Thank you for the contribution! I tagged @mmcgee-jump for review.

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?

Yes, the syscall policy for the monitor is defined in src/app/fdctl/monitor/monitor.seccomppolicy. You'd probably want to amend the expression for the read syscall to allow file descriptor 0 (stdin).

Thanks! @ripatel-fd Let me push the updated policy

ripatel-fd · 2024-12-19T12:22:54Z

Thanks! @ripatel-fd Let me push the updated policy

Great, thanks! I forgot to mention, could you please also run make seccomp-policies to regenerate the C header files created from those seccomp policy files?

ripatel-fd · 2024-12-19T12:26:30Z

src/app/fdctl/monitor/helper.h

@@ -107,4 +107,6 @@ printf_pct( char ** buf,
            ulong  den_then,
            double lhopital_den );

+int
+fd_getch(void);


Could you add documentation what this function does?

Suggested change

fd_getch(void);

/* fd_getch silently reads a single character. The terminal is temporarily switched

to non-canonical non-echo mode. */

fd_getch( void );

ripatel-fd · 2024-12-19T12:29:13Z

src/app/fdctl/monitor/helper.c

+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;
+}


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 void to 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);

ripatel-fd

Thank you for the contribution. It looks good overall. I posted a couple nits above.

src/app/fdctl/monitor/monitor.c

ripatel-fd · 2024-12-19T12:30:11Z

src/app/fdctl/monitor/monitor.c

@@ -293,8 +293,9 @@ run_monitor( config_t * const config,
    if( FD_UNLIKELY( (ulong)n>=buf_sz ) ) FD_LOG_ERR(( "snprintf truncated" )); \
    buf += n; buf_sz -= (ulong)n;                                               \
  } while(0)
-
+#define TAB 9


It may be more readable to use \t instead of a TAB define

ripatel-fd · 2024-12-19T12:34:20Z

src/app/fdctl/monitor/helper.c

+  tcgetattr(STDIN_FILENO, &oldt);
+  newt = oldt;
+  newt.c_lflag &= (tcflag_t)~(ICANON | ECHO);
+  tcsetattr(STDIN_FILENO, TCSANOW, &newt);


I wonder if we can leave this enabled all the time? 🤔
I'm not sure whether that's a good idea though since it requires extra care to restore the terminal when the user exits.

I wonder if we can leave this enabled all the time? 🤔 I'm not sure whether that's a good idea though since it requires extra care to restore the terminal when the user exits.

The terminal restores back fine even if I leave it on so I don't see any issues if you think that's better. It would save us calls to ioctl.

Sounds good to me 👍

Co-authored-by: ripatel-fd <[email protected]>

anoushk1234 force-pushed the anoushk1234/sep-monitor-panes branch from 2c11519 to b1bc7f1 Compare December 17, 2024 13:21

fix: make monitor more readable

84fd7c0

Signed-off-by: Anoushk Kharangate <[email protected]>

anoushk1234 force-pushed the anoushk1234/sep-monitor-panes branch from b1bc7f1 to 84fd7c0 Compare December 17, 2024 13:52

ripatel-fd requested a review from mmcgee-jump December 18, 2024 14:55

ripatel-fd added the operator label Dec 18, 2024

ripatel-fd reviewed Dec 19, 2024

View reviewed changes

ripatel-fd requested changes Dec 19, 2024

View reviewed changes

ripatel-fd reviewed Dec 19, 2024

View reviewed changes

Update src/app/fdctl/monitor/monitor.c

67e0461

Co-authored-by: ripatel-fd <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make monitor more readable #3702

fix: make monitor more readable #3702

anoushk1234 commented Dec 14, 2024 •

edited

Loading

anoushk1234 commented Dec 17, 2024

anoushk1234 commented Dec 17, 2024

ripatel-fd commented Dec 18, 2024

anoushk1234 commented Dec 19, 2024

ripatel-fd commented Dec 19, 2024

ripatel-fd Dec 19, 2024 •

edited

Loading

ripatel-fd Dec 19, 2024

ripatel-fd left a comment

ripatel-fd Dec 19, 2024

ripatel-fd Dec 19, 2024

anoushk1234 Dec 20, 2024

ripatel-fd Dec 20, 2024

-fd_getch(void);
+/* fd_getch silently reads a single character. The terminal is temporarily switched
+   to non-canonical non-echo mode. */
+fd_getch( void );

fix: make monitor more readable #3702

Are you sure you want to change the base?

fix: make monitor more readable #3702

Conversation

anoushk1234 commented Dec 14, 2024 • edited Loading

anoushk1234 commented Dec 17, 2024

anoushk1234 commented Dec 17, 2024

ripatel-fd commented Dec 18, 2024

anoushk1234 commented Dec 19, 2024

ripatel-fd commented Dec 19, 2024

ripatel-fd Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

ripatel-fd Dec 19, 2024

Choose a reason for hiding this comment

ripatel-fd left a comment

Choose a reason for hiding this comment

ripatel-fd Dec 19, 2024

Choose a reason for hiding this comment

ripatel-fd Dec 19, 2024

Choose a reason for hiding this comment

anoushk1234 Dec 20, 2024

Choose a reason for hiding this comment

ripatel-fd Dec 20, 2024

Choose a reason for hiding this comment

anoushk1234 commented Dec 14, 2024 •

edited

Loading

ripatel-fd Dec 19, 2024 •

edited

Loading