Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210226181158.6538-1-sonnysasaka@chromium.org>
 <69611743-89EB-4E86-A1F3-93DEE833774E@holtmann.org> <CAO271mmPpPmJxR8zBvy-tDn441z+c0_G0_F_LTTpiMwTgCDqXA@mail.gmail.com>
In-Reply-To: <CAO271mmPpPmJxR8zBvy-tDn441z+c0_G0_F_LTTpiMwTgCDqXA@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Fri, 26 Feb 2021 12:53:57 -0800
Message-ID: <CAO271m==xPAggx-o=4oRYNXUSqwdBYFr=nFAo4WQ247ixRYbYQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ] monitor: Add option to set fallback width
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Daniel Winkler <danielwinkler@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Marcel,

I missed one of your comments in v2. Please take a look at v3 instead. Than=
ks!

On Fri, Feb 26, 2021 at 12:50 PM Sonny Sasaka <sonnysasaka@chromium.org> wr=
ote:
>
> 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@holtmann.org> wr=
ote:
> >
> > 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@google.com>
> > >
> > > ---
> > > 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 page=
r)
> > >       }
> > >
> > >       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 =3D 0;
> > > +static int output_width =3D 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_c=
ol =3D=3D 0)
> > > -                     cached_num_columns =3D FALLBACK_TERMINAL_WIDTH;
> > > +                     cached_num_columns =3D output_width > 0 ?
> > > +                             output_width : FALLBACK_TERMINAL_WIDTH;
> > >               else
> > >                       cached_num_columns =3D 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 =3D 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, u=
int64_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 termina=
l\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[] =3D {
> > >       { "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 =3D NULL;
> > >       char *rtt =3D NULL;
> > > +     int column =3D 0;
> > >       int exit_status;
> > >
> > >       mainloop_init();
> > > @@ -121,7 +124,7 @@ int main(int argc, char *argv[])
> > >               struct sockaddr_un addr;
> > >
> > >               opt =3D 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=E2=80=99t put this at the end. The vh is at the end on purpo=
se.
> >
> > >                                       main_options, NULL);
> > >               if (opt < 0)
> > >                       break;
> > > @@ -205,6 +208,9 @@ int main(int argc, char *argv[])
> > >               case 'R':
> > >                       rtt =3D optarg;
> > >                       break;
> > > +             case 'c':
> > > +                     column =3D 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 eit=
her num_columns or pager_width or something like that.
> >
> > >               return EXIT_SUCCESS;
> > >       }
> >
> > Regards
> >
> > Marcel
> >