Re: [PATCH] monitor: Add support for limiting btsnoop files size

[Szymon Janc <szymon.janc@codecoup.pl>]

Hi Marcel,

On Friday, 5 January 2018 15:47:26 CET Marcel Holtmann wrote:
> Hi Szymon,
>=20
> > This adds new option -l (--limit) which allows to limit maximum
> > size of writen btsnoop file. After limit is reached new btsnoop
> > file will be created. To distinguish file postfix in for of
> > _DATE_TIME is added to path provided with -w option. For convenience
> > KkMm modifiers are also supported.
> >=20
> > This is especially useful on unattended systems for tiding btmon
> > with log rotation.
> >=20
> > Example result of running btmon -w test.bin -l 10k
> > test.log_2018-01-04_14:55:06_694638
> > test.log_2018-01-04_14:56:15_933158
> > test.log_2018-01-04_14:57:20_741201
> > test.log_2018-01-04_14:58:13_280835
> > test.log_2018-01-04_14:59:02_183569
> > test.log_2018-01-04_15:00:05_352733
> > test.log_2018-01-04_15:00:54_475147
> > test.log_2018-01-04_15:01:57_183352
>=20
> this is the quick and dirty approach. However a bit cleaner one would be
> that we actually add information to each file where the next one can be
> found. And preferably also which one was the previous one. I realize that
> any kind of renaming kills this, so maybe this needs to be based on some
> sort of hash that we include in each file as a identification header. And
> then the reader needs to figure this out by itself and maybe try some
> patterns to find the file or as proposed by Luiz, you give a glob matching
> number of files and we ensure they are all sorted correctly.
>=20
> In addition to this, maybe a round-robin schema is also useful, so that
> earlier logs get overwritten. Something like "use max 10M and split over =
10
> files=E2=80=9D is generally more useful then some uncontrolled rotation. =
So
> suffixes of *.1, *.2 etc are normally easier to deal with.
>=20
> Also in that context, introducing compressed log files would be interesti=
ng
> feature as well. I would be curious if compressing these actually has a b=
ig
> impact. I would assume with audio data it might actually safe a lot. If y=
ou
> rotate and compress, then this becomes really useful.

I want to keep this as simple as possible since this is intended to work wi=
th=20
log rotation systems, not be used directly by human. The main goal was to=20
avoid need of restarting btmon when switching btsnoop file (to avoid loosin=
g=20
traces). Compression, removing old etc is job of logrotate, not btmon. I=20
choose date-time for suffixes not only for human convenience but also for=20
(reasonable) level of uniqueness eg in case of btmon restart (I could short=
en=20
it and skip usecs to keep it a bit simpler).


> > ---
> > monitor/control.c    | 55
> > ++++++++++++++++++++++++++++++++++++++++++++++++++-- monitor/control.h =
 =20
> > |  2 +-
> > monitor/main.c       | 35 +++++++++++++++++++++++++++++++--
> > src/shared/btsnoop.c | 17 ----------------
> > src/shared/btsnoop.h | 17 ++++++++++++++++
> > tools/btsnoop.c      | 17 ----------------
> > 6 files changed, 104 insertions(+), 39 deletions(-)
> >=20
> > diff --git a/monitor/control.c b/monitor/control.c
> > index 1cd79ca5d..6b69f3575 100644
> > --- a/monitor/control.c
> > +++ b/monitor/control.c
> > @@ -32,6 +32,7 @@
> > #include <unistd.h>
> > #include <stdlib.h>
> > #include <string.h>
> > +#include <time.h>
> > #include <sys/time.h>
> > #include <sys/socket.h>
> > #include <sys/un.h>
> > @@ -59,6 +60,10 @@ static struct btsnoop *btsnoop_file =3D NULL;
> > static bool hcidump_fallback =3D false;
> > static bool decode_control =3D true;
> >=20
> > +static size_t writer_limit =3D 0;
> > +static size_t writer_size =3D BTSNOOP_HDR_SIZE;
> > +static const char *writer_path =3D NULL;
> > +
> > struct control_data {
> >=20
> > 	uint16_t channel;
> > 	int fd;
> >=20
> > @@ -908,6 +913,47 @@ void control_message(uint16_t opcode, const void
> > *data, uint16_t size)>=20
> > 	}
> >=20
> > }
> >=20
> > +static const char *get_real_path(const char *path)
> > +{
> > +	static char real_path[FILENAME_MAX];
> > +	struct timeval tv;
> > +	struct tm tm;
> > +
> > +	if (!writer_limit)
> > +		return path;
> > +
> > +	memset(&tv, 0, sizeof(tv));
> > +
> > +	gettimeofday(&tv, NULL);
> > +	localtime_r(&tv.tv_sec, &tm);
> > +
> > +	snprintf(real_path, FILENAME_MAX,
> > +			"%s_%04d-%02d-%02d_%02d:%02d:%02d_%06lu", path,
> > +			tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> > +			tm.tm_hour, tm.tm_min, tm.tm_sec, tv.tv_usec);
> > +
> > +	real_path[FILENAME_MAX - 1] =3D '\0';
> > +
> > +	return real_path;
> > +}
> > +
> > +static void rotate_btsnoop_file(ssize_t pktlen)
> > +{
> > +	if (!writer_limit)
> > +		return;
> > +
> > +	writer_size +=3D BTSNOOP_PKT_SIZE + pktlen;
> > +
> > +	if (writer_size <=3D writer_limit)
> > +		return;
> > +
> > +	writer_size =3D BTSNOOP_HDR_SIZE + BTSNOOP_PKT_SIZE + pktlen;
> > +
> > +	btsnoop_unref(btsnoop_file);
> > +	btsnoop_file =3D btsnoop_create(get_real_path(writer_path),
> > +							BTSNOOP_FORMAT_MONITOR);
> > +}
> > +
> > static void data_callback(int fd, uint32_t events, void *user_data)
> > {
> >=20
> > 	struct control_data *data =3D user_data;
> >=20
> > @@ -974,6 +1020,8 @@ static void data_callback(int fd, uint32_t events,
> > void *user_data)>=20
> > 							data->buf, pktlen);
> > 		=09
> > 			break;
> > 	=09
> > 		case HCI_CHANNEL_MONITOR:
> > +			rotate_btsnoop_file(pktlen);
> > +
> >=20
> > 			btsnoop_write_hci(btsnoop_file, tv, index, opcode, 0,
> > 		=09
> > 							data->buf, pktlen);
> > 		=09
> > 			ellisys_inject_hci(tv, index, opcode,
> >=20
> > @@ -1371,10 +1419,13 @@ int control_tty(const char *path, unsigned int
> > speed)>=20
> > 	return 0;
> >=20
> > }
> >=20
> > -bool control_writer(const char *path)
> > +bool control_writer(const char *path, size_t limit)
> > {
> > -	btsnoop_file =3D btsnoop_create(path, BTSNOOP_FORMAT_MONITOR);
> > +	writer_path =3D path;
> > +	writer_limit =3D limit;
> >=20
> > +	btsnoop_file =3D btsnoop_create(get_real_path(writer_path),
> > +							BTSNOOP_FORMAT_MONITOR);
> >=20
> > 	return !!btsnoop_file;
> >=20
> > }
> >=20
> > diff --git a/monitor/control.h b/monitor/control.h
> > index 630a852e4..dec19d1d4 100644
> > --- a/monitor/control.h
> > +++ b/monitor/control.h
> > @@ -24,7 +24,7 @@
> >=20
> > #include <stdint.h>
> >=20
> > -bool control_writer(const char *path);
> > +bool control_writer(const char *path, size_t limit);
> > void control_reader(const char *path);
> > void control_server(const char *path);
> > int control_tty(const char *path, unsigned int speed);
> > diff --git a/monitor/main.c b/monitor/main.c
> > index 3e61a4661..3755d01ae 100644
> > --- a/monitor/main.c
> > +++ b/monitor/main.c
> > @@ -31,6 +31,7 @@
> > #include <stdlib.h>
> > #include <string.h>
> > #include <getopt.h>
> > +#include <limits.h>
> > #include <sys/un.h>
> >=20
> > #include "src/shared/mainloop.h"
> > @@ -61,6 +62,7 @@ static void usage(void)
> >=20
> > 	printf("options:\n"
> > =09
> > 		"\t-r, --read <file>      Read traces in btsnoop format\n"
> > 		"\t-w, --write <file>     Save traces in btsnoop format\n"
> >=20
> > +		"\t-l, --limit <bytes>    Rotate btsnoop files\n"
> >=20
> > 		"\t-a, --analyze <file>   Analyze traces in btsnoop format\n"
> > 		"\t-s, --server <socket>  Start monitor server socket\n"
> > 		"\t-p, --priority <level> Show only priority or lower\n"
> >=20
> > @@ -80,6 +82,7 @@ static const struct option main_options[] =3D {
> >=20
> > 	{ "tty-speed", required_argument, NULL, 'B' },
> > 	{ "read",    required_argument, NULL, 'r' },
> > 	{ "write",   required_argument, NULL, 'w' },
> >=20
> > +	{ "limit",   required_argument, NULL, 'l' },
> >=20
> > 	{ "analyze", required_argument, NULL, 'a' },
> > 	{ "server",  required_argument, NULL, 's' },
> > 	{ "priority",required_argument, NULL, 'p' },
> >=20
> > @@ -108,6 +111,8 @@ int main(int argc, char *argv[])
> >=20
> > 	const char *str;
> > 	int exit_status;
> > 	sigset_t mask;
> >=20
> > +	size_t writer_limit =3D 0;
> > +	char *endptr;
> >=20
> > 	mainloop_init();
> >=20
> > @@ -117,7 +122,7 @@ int main(int argc, char *argv[])
> >=20
> > 		int opt;
> > 		struct sockaddr_un addr;
> >=20
> > -		opt =3D getopt_long(argc, argv, "d:r:w:a:s:p:i:tTSAE:vh",
> > +		opt =3D getopt_long(argc, argv, "d:r:w:l:a:s:p:i:tTSAE:vh",
> >=20
> > 						main_options, NULL);
> > 	=09
> > 		if (opt < 0)
> > 	=09
> > 			break;
> >=20
> > @@ -139,6 +144,32 @@ int main(int argc, char *argv[])
> >=20
> > 		case 'w':
> > 			writer_path =3D optarg;
> > 			break;
> >=20
> > +		case 'l':
> > +			str =3D optarg;
> > +			writer_limit =3D strtoul(str, &endptr, 10);
> > +
> > +			if (writer_limit =3D=3D ULONG_MAX) {
> > +				fprintf(stderr, "Invalid limit value\n");
> > +				return EXIT_FAILURE;
> > +			}
> > +
> > +			if (*endptr !=3D '\0') {
> > +				if (*endptr =3D=3D 'K' || *endptr =3D=3D 'k') {
> > +					writer_limit *=3D 1024;
> > +				} else if (*endptr =3D=3D 'M' || *endptr =3D=3D 'm') {
> > +					writer_limit *=3D 1024 * 1024;
> > +				} else {
> > +					fprintf(stderr, "Invalid limit value\n");
> > +					return EXIT_FAILURE;
> > +				}
> > +			}
> > +
> > +			/* avoid creating too small files */
> > +			if (writer_limit < 1024) {
> > +				fprintf(stderr, "Too small limit value\n");
> > +				return EXIT_FAILURE;
> > +			}
> > +			break;
> >=20
> > 		case 'a':
> > 			analyze_path =3D optarg;
> > 			break;
> >=20
> > @@ -232,7 +263,7 @@ int main(int argc, char *argv[])
> >=20
> > 		return EXIT_SUCCESS;
> > =09
> > 	}
> >=20
> > -	if (writer_path && !control_writer(writer_path)) {
> > +	if (writer_path && !control_writer(writer_path, writer_limit)) {
> >=20
> > 		printf("Failed to open '%s'\n", writer_path);
> > 		return EXIT_FAILURE;
> > =09
> > 	}
> >=20
> > diff --git a/src/shared/btsnoop.c b/src/shared/btsnoop.c
> > index e20d1b382..17e3faf2a 100644
> > --- a/src/shared/btsnoop.c
> > +++ b/src/shared/btsnoop.c
> > @@ -35,23 +35,6 @@
> >=20
> > #include "src/shared/btsnoop.h"
> >=20
> > -struct btsnoop_hdr {
> > -	uint8_t		id[8];		/* Identification Pattern */
> > -	uint32_t	version;	/* Version Number =3D 1 */
> > -	uint32_t	type;		/* Datalink Type */
> > -} __attribute__ ((packed));
> > -#define BTSNOOP_HDR_SIZE (sizeof(struct btsnoop_hdr))
> > -
> > -struct btsnoop_pkt {
> > -	uint32_t	size;		/* Original Length */
> > -	uint32_t	len;		/* Included Length */
> > -	uint32_t	flags;		/* Packet Flags */
> > -	uint32_t	drops;		/* Cumulative Drops */
> > -	uint64_t	ts;		/* Timestamp microseconds */
> > -	uint8_t		data[0];	/* Packet Data */
> > -} __attribute__ ((packed));
> > -#define BTSNOOP_PKT_SIZE (sizeof(struct btsnoop_pkt))
> > -
> > static const uint8_t btsnoop_id[] =3D { 0x62, 0x74, 0x73, 0x6e,
> >=20
> > 				      0x6f, 0x6f, 0x70, 0x00 };
> >=20
> > diff --git a/src/shared/btsnoop.h b/src/shared/btsnoop.h
> > index 3df8998a3..011aa1d68 100644
> > --- a/src/shared/btsnoop.h
> > +++ b/src/shared/btsnoop.h
> > @@ -91,6 +91,23 @@ struct btsnoop_opcode_index_info {
> > #define BTSNOOP_PRIORITY_INFO		6
> > #define BTSNOOP_PRIORITY_DEBUG		7
> >=20
> > +struct btsnoop_hdr {
> > +	uint8_t		id[8];		/* Identification Pattern */
> > +	uint32_t	version;	/* Version Number =3D 1 */
> > +	uint32_t	type;		/* Datalink Type */
> > +} __attribute__ ((packed));
> > +#define BTSNOOP_HDR_SIZE (sizeof(struct btsnoop_hdr))
> > +
> > +struct btsnoop_pkt {
> > +	uint32_t	size;		/* Original Length */
> > +	uint32_t	len;		/* Included Length */
> > +	uint32_t	flags;		/* Packet Flags */
> > +	uint32_t	drops;		/* Cumulative Drops */
> > +	uint64_t	ts;		/* Timestamp microseconds */
> > +	uint8_t		data[0];	/* Packet Data */
> > +} __attribute__ ((packed));
> > +#define BTSNOOP_PKT_SIZE (sizeof(struct btsnoop_pkt))
> > +
> > struct btsnoop_opcode_user_logging {
> >=20
> > 	uint8_t  priority;
> > 	uint8_t  ident_len;
> >=20
> > diff --git a/tools/btsnoop.c b/tools/btsnoop.c
> > index 3eb8082dd..465938961 100644
> > --- a/tools/btsnoop.c
> > +++ b/tools/btsnoop.c
> > @@ -41,23 +41,6 @@
> >=20
> > #include "src/shared/btsnoop.h"
> >=20
> > -struct btsnoop_hdr {
> > -	uint8_t		id[8];		/* Identification Pattern */
> > -	uint32_t	version;	/* Version Number =3D 1 */
> > -	uint32_t	type;		/* Datalink Type */
> > -} __attribute__ ((packed));
> > -#define BTSNOOP_HDR_SIZE (sizeof(struct btsnoop_hdr))
> > -
> > -struct btsnoop_pkt {
> > -	uint32_t	size;		/* Original Length */
> > -	uint32_t	len;		/* Included Length */
> > -	uint32_t	flags;		/* Packet Flags */
> > -	uint32_t	drops;		/* Cumulative Drops */
> > -	uint64_t	ts;		/* Timestamp microseconds */
> > -	uint8_t		data[0];	/* Packet Data */
> > -} __attribute__ ((packed));
> > -#define BTSNOOP_PKT_SIZE (sizeof(struct btsnoop_pkt))
> > -
> > static const uint8_t btsnoop_id[] =3D { 0x62, 0x74, 0x73, 0x6e,
> >=20
> > 				      0x6f, 0x6f, 0x70, 0x00 };
>=20
> This change seems unrelated. So lets avoid this. And initially I didn=E2=
=80=99t want
> the file headers and details exposed. They were suppose to be all accessed
> via functions. If we want to unify this, then I need to have a second look
> at it.

It is needed for proper counting of written bytes. But I can simply do copy=
 of=20
those in btmon (just like it is done in btsnoop and hcidump..).


> Regards
>=20
> Marcel


=2D-=20
pozdrawiam
Szymon Janc