[bug report] tools: iio: iio_generic_buffer: allow continuous looping

[bug report] tools: iio: iio_generic_buffer: allow continuous looping

[Dan Carpenter]

Hello Martin Kelly,

The patch 55dda0abcf9d: "tools: iio: iio_generic_buffer: allow
continuous looping" from May 17, 2018, leads to the following static
checker warning:

	./tools/iio/iio_generic_buffer.c:640 main()
	warn: unsigned 'num_loops' is never less than zero.

./tools/iio/iio_generic_buffer.c
    331 int main(int argc, char **argv)
    332 {
    333 	unsigned long long num_loops = 2;
                ^^^^^^^^

    334 	unsigned long timedelay = 1000000;
    335 	unsigned long buf_len = 128;
    336 
    337 	ssize_t i;
    338 	unsigned long long j;
    339 	unsigned long toread;
    340 	int ret, c;
    341 	int fp = -1;
    342 
    343 	int num_channels = 0;
    344 	char *trigger_name = NULL, *device_name = NULL;
    345 
    346 	char *data = NULL;
    347 	ssize_t read_size;
    348 	int dev_num = -1, trig_num = -1;
    349 	char *buffer_access = NULL;
    350 	int scan_size;
    351 	int noevents = 0;
    352 	int notrigger = 0;
    353 	char *dummy;
    354 	bool force_autochannels = false;
    355 
    356 	struct iio_channel_info *channels = NULL;
    357 

[ snip ]

    632 	/* Attempt to open non blocking the access dev */
    633 	fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
    634 	if (fp == -1) { /* TODO: If it isn't there make the node */
    635 		ret = -errno;
    636 		fprintf(stderr, "Failed to open %s\n", buffer_access);
    637 		goto error;
    638 	}
    639 
--> 640 	for (j = 0; j < num_loops || num_loops < 0; j++) {
                                             ^^^^^^^^^^^^^
If num_loops is -1 then it's supposed to loop forever.  It basically
does, except for the static checker warning.

    641 		if (!noevents) {
    642 			struct pollfd pfd = {
    643 				.fd = fp,
    644 				.events = POLLIN,
    645 			};
    646 
    647 			ret = poll(&pfd, 1, -1);
    648 			if (ret < 0) {
    649 				ret = -errno;
    650 				goto error;
    651 			} else if (ret == 0) {
    652 				continue;
    653 			}
    654 
    655 			toread = buf_len;
    656 		} else {
    657 			usleep(timedelay);
    658 			toread = 64;
    659 		}

regards,
dan carpenter

Re: [bug report] tools: iio: iio_generic_buffer: allow continuous looping

[Martin Kelly]

On 1/8/19 12:38 PM, Dan Carpenter wrote:
> Hello Martin Kelly,
> 
> The patch 55dda0abcf9d: "tools: iio: iio_generic_buffer: allow
> continuous looping" from May 17, 2018, leads to the following static
> checker warning:
> 
> 	./tools/iio/iio_generic_buffer.c:640 main()
> 	warn: unsigned 'num_loops' is never less than zero.
> 

Thanks for catching this. How can I repro this so that I can make sure 
the static analyzer is happy once I fix it?

> ./tools/iio/iio_generic_buffer.c
>      331 int main(int argc, char **argv)
>      332 {
>      333 	unsigned long long num_loops = 2;
>                  ^^^^^^^^
> 
>      334 	unsigned long timedelay = 1000000;
>      335 	unsigned long buf_len = 128;
>      336
>      337 	ssize_t i;
>      338 	unsigned long long j;
>      339 	unsigned long toread;
>      340 	int ret, c;
>      341 	int fp = -1;
>      342
>      343 	int num_channels = 0;
>      344 	char *trigger_name = NULL, *device_name = NULL;
>      345
>      346 	char *data = NULL;
>      347 	ssize_t read_size;
>      348 	int dev_num = -1, trig_num = -1;
>      349 	char *buffer_access = NULL;
>      350 	int scan_size;
>      351 	int noevents = 0;
>      352 	int notrigger = 0;
>      353 	char *dummy;
>      354 	bool force_autochannels = false;
>      355
>      356 	struct iio_channel_info *channels = NULL;
>      357
> 
> [ snip ]
> 
>      632 	/* Attempt to open non blocking the access dev */
>      633 	fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>      634 	if (fp == -1) { /* TODO: If it isn't there make the node */
>      635 		ret = -errno;
>      636 		fprintf(stderr, "Failed to open %s\n", buffer_access);
>      637 		goto error;
>      638 	}
>      639
> --> 640 	for (j = 0; j < num_loops || num_loops < 0; j++) {
>                                               ^^^^^^^^^^^^^
> If num_loops is -1 then it's supposed to loop forever.  It basically
> does, except for the static checker warning.
> 

Yep, looks like -1 gets converted to ULONG_LONG_MAX and it loops for a 
very long time but not forever. I will fix it soon.

>      641 		if (!noevents) {
>      642 			struct pollfd pfd = {
>      643 				.fd = fp,
>      644 				.events = POLLIN,
>      645 			};
>      646
>      647 			ret = poll(&pfd, 1, -1);
>      648 			if (ret < 0) {
>      649 				ret = -errno;
>      650 				goto error;
>      651 			} else if (ret == 0) {
>      652 				continue;
>      653 			}
>      654
>      655 			toread = buf_len;
>      656 		} else {
>      657 			usleep(timedelay);
>      658 			toread = 64;
>      659 		}
> 
> regards,
> dan carpenter
> 

Re: [bug report] tools: iio: iio_generic_buffer: allow continuous looping

[Dan Carpenter]

On Tue, Jan 08, 2019 at 10:20:38PM +0000, Martin Kelly wrote:
> On 1/8/19 12:38 PM, Dan Carpenter wrote:
> > Hello Martin Kelly,
> > 
> > The patch 55dda0abcf9d: "tools: iio: iio_generic_buffer: allow
> > continuous looping" from May 17, 2018, leads to the following static
> > checker warning:
> > 
> > 	./tools/iio/iio_generic_buffer.c:640 main()
> > 	warn: unsigned 'num_loops' is never less than zero.
> > 
> 
> Thanks for catching this. How can I repro this so that I can make sure 
> the static analyzer is happy once I fix it?
> 

It's a Smatch warning.  In this case you would just do:

~/path/to/smatch/smatch tools/iio/iio_generic_buffer.c

regards,
dan carpenter

Re: [bug report] tools: iio: iio_generic_buffer: allow continuous looping

[Martin Kelly]

On 1/8/19 10:23 PM, Dan Carpenter wrote>
> It's a Smatch warning.  In this case you would just do:
> 
> ~/path/to/smatch/smatch tools/iio/iio_generic_buffer.c
> 

Thanks, I sent a patch to fix the issue.