Hi Marcel,
I have addressed your comments in v2. Please take another look. Thanks!
On Fri, Feb 26, 2021 at 12:09 PM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Sonny,
>
> > Sometimes we want to be able to pipe the output of btmon to a
> > non-terminal device. The current fallback width is usually not long
> > enough so this patch adds an option to specify the column width. This is
> > especially needed for text logs from bluetoothd.
> >
> > Reviewed-by: Daniel Winkler <danielwinkler@xxxxxxxxxx>
> >
> > ---
> > monitor/control.c | 4 ++--
> > monitor/control.h | 2 +-
> > monitor/display.c | 8 ++++++--
> > monitor/display.h | 2 +-
> > monitor/main.c | 10 ++++++++--
> > 5 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/monitor/control.c b/monitor/control.c
> > index d1ba97d37..00001bf1e 100644
> > --- a/monitor/control.c
> > +++ b/monitor/control.c
> > @@ -1474,7 +1474,7 @@ bool control_writer(const char *path)
> > return !!btsnoop_file;
> > }
> >
> > -void control_reader(const char *path, bool pager)
> > +void control_reader(const char *path, bool pager, int column)
> > {
> > unsigned char buf[BTSNOOP_MAX_PACKET_SIZE];
> > uint16_t pktlen;
> > @@ -1500,7 +1500,7 @@ void control_reader(const char *path, bool pager)
> > }
> >
> > if (pager)
> > - open_pager();
> > + open_pager(column);
> >
> > switch (format) {
> > case BTSNOOP_FORMAT_HCI:
> > diff --git a/monitor/control.h b/monitor/control.h
> > index 29616c4f1..ad16358b1 100644
> > --- a/monitor/control.h
> > +++ b/monitor/control.h
> > @@ -12,7 +12,7 @@
> > #include <stdint.h>
> >
> > bool control_writer(const char *path);
> > -void control_reader(const char *path, bool pager);
> > +void control_reader(const char *path, bool pager, int column);
> > void control_server(const char *path);
> > int control_tty(const char *path, unsigned int speed);
> > int control_rtt(char *jlink, char *rtt);
> > diff --git a/monitor/display.c b/monitor/display.c
> > index b11b71d5d..598db8080 100644
> > --- a/monitor/display.c
> > +++ b/monitor/display.c
> > @@ -28,6 +28,7 @@
> > #include "display.h"
> >
> > static pid_t pager_pid = 0;
> > +static int output_width = 0;
>
> why not set this to FALLBACK_TERMINAL_WIDTH?
>
> > bool use_color(void)
> > {
> > @@ -48,7 +49,8 @@ int num_columns(void)
> >
> > if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) < 0 ||
> > ws.ws_col == 0)
> > - cached_num_columns = FALLBACK_TERMINAL_WIDTH;
> > + cached_num_columns = output_width > 0 ?
> > + output_width : FALLBACK_TERMINAL_WIDTH;
> > else
> > cached_num_columns = ws.ws_col;
> > }
> > @@ -81,12 +83,14 @@ static void wait_for_terminate(pid_t pid)
> > }
> > }
> >
> > -void open_pager(void)
> > +void open_pager(int column)
> > {
> > const char *pager;
> > pid_t parent_pid;
> > int fd[2];
> >
> > + output_width = column;
> > +
> > if (pager_pid > 0)
> > return;
> >
> > diff --git a/monitor/display.h b/monitor/display.h
> > index f3a614b81..70734d590 100644
> > --- a/monitor/display.h
> > +++ b/monitor/display.h
> > @@ -75,5 +75,5 @@ static inline uint64_t print_bitfield(int indent, uint64_t val,
> >
> > int num_columns(void);
> >
> > -void open_pager(void);
> > +void open_pager(int column);
> > void close_pager(void);
> > diff --git a/monitor/main.c b/monitor/main.c
> > index 0f5eb4a3b..5996eed40 100644
> > --- a/monitor/main.c
> > +++ b/monitor/main.c
> > @@ -67,6 +67,7 @@ static void usage(void)
> > "\t Read data from RTT\n"
> > "\t-R --rtt [<address>],[<area>],[<name>]\n"
> > "\t RTT control block parameters\n"
> > + "\t-c, --column [width] Output width if not a terminal\n"
>
> I think we should be using uppercase C.
>
> > "\t-h, --help Show help options\n");
> > }
> >
> > @@ -90,6 +91,7 @@ static const struct option main_options[] = {
> > { "no-pager", no_argument, NULL, 'P' },
> > { "jlink", required_argument, NULL, 'J' },
> > { "rtt", required_argument, NULL, 'R' },
> > + { "column", required_argument, NULL, 'c' },
> > { "todo", no_argument, NULL, '#' },
> > { "version", no_argument, NULL, 'v' },
> > { "help", no_argument, NULL, 'h' },
> > @@ -110,6 +112,7 @@ int main(int argc, char *argv[])
> > const char *str;
> > char *jlink = NULL;
> > char *rtt = NULL;
> > + int column = 0;
> > int exit_status;
> >
> > mainloop_init();
> > @@ -121,7 +124,7 @@ int main(int argc, char *argv[])
> > struct sockaddr_un addr;
> >
> > opt = getopt_long(argc, argv,
> > - "r:w:a:s:p:i:d:B:V:MNtTSAE:PJ:R:vh",
> > + "r:w:a:s:p:i:d:B:V:MNtTSAE:PJ:R:vhc:",
>
> Please don’t put this at the end. The vh is at the end on purpose.
>
> > main_options, NULL);
> > if (opt < 0)
> > break;
> > @@ -205,6 +208,9 @@ int main(int argc, char *argv[])
> > case 'R':
> > rtt = optarg;
> > break;
> > + case 'c':
> > + column = atoi(optarg);
> > + break;
> > case '#':
> > packet_todo();
> > lmp_todo();
> > @@ -245,7 +251,7 @@ int main(int argc, char *argv[])
> > if (ellisys_server)
> > ellisys_enable(ellisys_server, ellisys_port);
> >
> > - control_reader(reader_path, use_pager);
> > + control_reader(reader_path, use_pager, column);
>
> I prefer that we use a bit more descriptive variable names here. So either num_columns or pager_width or something like that.
>
> > return EXIT_SUCCESS;
> > }
>
> Regards
>
> Marcel
>
