[PATCH v2 0/4] tools: iio: Send error messages to stderr

[PATCH v2 0/4] tools: iio: Send error messages to stderr

[Cristina Opriceana]

This patchset indends to make some cleanup and send printf
error messages to stderr. The changes were performed with coccinelle for
failure messages and manual for other cases. The following script
was used:

@r@
expression e1, e2, stdout;
@@
(printf(e1);
|
printf(e1, e2);
|
fprintf(stdout, e1);
|
fprintf(stdout, e1, e2);
)
@script: python get_string@
e << r.e1;
tdres;
@@
if 'could not' in e.lower() or 'fail' in e.lower() \
or 'problem' in e.lower() or 'not set' in e.lower():
coccinelle.tdres = e
else:
cocci.include_match(False)
@r_match@
expression r.stdout, r.e1, r.e2;
identifier get_string.tdres;
@@
(
- printf(e1);
+ fprintf(stderr, tdres);
|
- printf(e1, e2);
+ fprintf(stderr, tdres, e2);
|
- fprintf(stdout, e1)
+ fprintf(stderr, tdres)
|
- fprintf(stdout, e1, e2)
+ fprintf(stderr, tdres, e2)
)

Changes in v2:
	- s/failiure/failure

Cristina Opriceana (4):
  tools: iio: Move printf failure messages to stderr
  tools: iio: Send usage error messages to stderr
  tools: iio: generic_buffer: Maintain fprintf() messages consistent
  iio: tools: iio_utils: Move general error messages to stderr

 tools/iio/generic_buffer.c    | 21 ++++++++++--------
 tools/iio/iio_event_monitor.c |  8 +++----
 tools/iio/iio_utils.c         | 51 +++++++++++++++++++++++++------------------
 tools/iio/lsiio.c             |  2 +-
 4 files changed, 47 insertions(+), 35 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Cristina Opriceana]

Replace printf error messages with fprintf(stderr, ...) in order
to ensure consistency and to make faults easier to identify.
This patch uses coccinelle to detect and apply the changes.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
Changes in v2:
 - s/failiure/failure

 tools/iio/generic_buffer.c    | 17 ++++++++++-------
 tools/iio/iio_event_monitor.c |  6 +++---
 tools/iio/iio_utils.c         | 34 ++++++++++++++++++++--------------
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index 0e73723..2f4e12f 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -271,7 +271,7 @@ int main(int argc, char **argv)
 	}
 
 	if (device_name == NULL) {
-		printf("Device name not set\n");
+		fprintf(stderr, "Device name not set\n");
 		print_usage();
 		return -1;
 	}
@@ -279,7 +279,7 @@ int main(int argc, char **argv)
 	/* Find the device requested */
 	dev_num = find_type_by_name(device_name, "iio:device");
 	if (dev_num < 0) {
-		printf("Failed to find the %s\n", device_name);
+		fprintf(stderr, "Failed to find the %s\n", device_name);
 		return dev_num;
 	}
 
@@ -307,7 +307,8 @@ int main(int argc, char **argv)
 		/* Verify the trigger exists */
 		trig_num = find_type_by_name(trigger_name, "trigger");
 		if (trig_num < 0) {
-			printf("Failed to find the trigger %s\n", trigger_name);
+			fprintf(stderr, "Failed to find the trigger %s\n",
+				trigger_name);
 			ret = trig_num;
 			goto error_free_triggername;
 		}
@@ -323,7 +324,7 @@ int main(int argc, char **argv)
 	 */
 	ret = build_channel_array(dev_dir_name, &channels, &num_channels);
 	if (ret) {
-		printf("Problem reading scan element information\n");
+		fprintf(stderr, "Problem reading scan element information\n");
 		printf("diag %s\n", dev_dir_name);
 		goto error_free_triggername;
 	}
@@ -350,7 +351,8 @@ int main(int argc, char **argv)
 						    dev_dir_name,
 						    trigger_name);
 		if (ret < 0) {
-			printf("Failed to write current_trigger file\n");
+			fprintf(stderr,
+				"Failed to write current_trigger file\n");
 			goto error_free_buf_dir_name;
 		}
 	}
@@ -382,7 +384,7 @@ int main(int argc, char **argv)
 	fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
 	if (fp == -1) { /* TODO: If it isn't there make the node */
 		ret = -errno;
-		printf("Failed to open %s\n", buffer_access);
+		fprintf(stderr, "Failed to open %s\n", buffer_access);
 		goto error_free_buffer_access;
 	}
 
@@ -431,7 +433,8 @@ int main(int argc, char **argv)
 		ret = write_sysfs_string("trigger/current_trigger",
 					 dev_dir_name, "NULL");
 		if (ret < 0)
-			printf("Failed to write to %s\n", dev_dir_name);
+			fprintf(stderr, "Failed to write to %s\n",
+				dev_dir_name);
 
 error_close_buffer_access:
 	if (close(fp) == -1)
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index 703f4cb..843bc4c 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -278,14 +278,14 @@ int main(int argc, char **argv)
 	fd = open(chrdev_name, 0);
 	if (fd == -1) {
 		ret = -errno;
-		fprintf(stdout, "Failed to open %s\n", chrdev_name);
+		fprintf(stderr, "Failed to open %s\n", chrdev_name);
 		goto error_free_chrdev_name;
 	}
 
 	ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
 	if (ret == -1 || event_fd == -1) {
 		ret = -errno;
-		fprintf(stdout, "Failed to retrieve event fd\n");
+		fprintf(stderr, "Failed to retrieve event fd\n");
 		if (close(fd) == -1)
 			perror("Failed to close character device file");
 
@@ -311,7 +311,7 @@ int main(int argc, char **argv)
 		}
 
 		if (ret != sizeof(event)) {
-			printf("Reading event failed!\n");
+			fprintf(stderr, "Reading event failed!\n");
 			ret = -EIO;
 			break;
 		}
diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
index 8fb3214..46dfa3f 100644
--- a/tools/iio/iio_utils.c
+++ b/tools/iio/iio_utils.c
@@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
 			sysfsfp = fopen(filename, "r");
 			if (sysfsfp == NULL) {
 				ret = -errno;
-				printf("failed to open %s\n", filename);
+				fprintf(stderr, "failed to open %s\n",
+					filename);
 				goto error_free_filename;
 			}
 
@@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
 				     &padint, shift);
 			if (ret < 0) {
 				ret = -errno;
-				printf("failed to pass scan type description\n");
+				fprintf(stderr,
+					"failed to pass scan type description\n");
 				goto error_close_sysfsfp;
 			} else if (ret != 5) {
 				ret = -EIO;
@@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
 			*is_signed = (signchar == 's');
 			if (fclose(sysfsfp)) {
 				ret = -errno;
-				printf("Failed to close %s\n", filename);
+				fprintf(stderr, "Failed to close %s\n",
+					filename);
 				goto error_free_filename;
 			}
 
@@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
 			sysfsfp = fopen(filename, "r");
 			if (sysfsfp == NULL) {
 				ret = -errno;
-				printf("failed to open %s\n", filename);
+				fprintf(stderr, "failed to open %s\n",
+					filename);
 				free(filename);
 				goto error_cleanup_array;
 			}
@@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const char *type)
 			ret = sscanf(ent->d_name + strlen(type), "%d", &number);
 			if (ret < 0) {
 				ret = -errno;
-				printf("failed to read element number\n");
+				fprintf(stderr,
+					"failed to read element number\n");
 				goto error_close_dir;
 			} else if (ret != 1) {
 				ret = -EIO;
-				printf("failed to match element number\n");
+				fprintf(stderr,
+					"failed to match element number\n");
 				goto error_close_dir;
 			}
 
@@ -664,7 +670,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
 	sysfsfp = fopen(temp, "w");
 	if (sysfsfp == NULL) {
 		ret = -errno;
-		printf("failed to open %s\n", temp);
+		fprintf(stderr, "failed to open %s\n", temp);
 		goto error_free;
 	}
 
@@ -685,7 +691,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
 		sysfsfp = fopen(temp, "r");
 		if (sysfsfp == NULL) {
 			ret = -errno;
-			printf("failed to open %s\n", temp);
+			fprintf(stderr, "failed to open %s\n", temp);
 			goto error_free;
 		}
 
@@ -750,7 +756,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
 	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
 
 	if (temp == NULL) {
-		printf("Memory allocation failed\n");
+		fprintf(stderr, "Memory allocation failed\n");
 		return -ENOMEM;
 	}
 
@@ -761,7 +767,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
 	sysfsfp = fopen(temp, "w");
 	if (sysfsfp == NULL) {
 		ret = -errno;
-		printf("Could not open %s\n", temp);
+		fprintf(stderr, "Could not open %s\n", temp);
 		goto error_free;
 	}
 
@@ -782,7 +788,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
 		sysfsfp = fopen(temp, "r");
 		if (sysfsfp == NULL) {
 			ret = -errno;
-			printf("Could not open file to verify\n");
+			fprintf(stderr, "Could not open file to verify\n");
 			goto error_free;
 		}
 
@@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename, const char *basedir)
 	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
 
 	if (temp == NULL) {
-		printf("Memory allocation failed");
+		fprintf(stderr, "Memory allocation failed");
 		return -ENOMEM;
 	}
 
@@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename, const char *basedir, float *val)
 	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
 
 	if (temp == NULL) {
-		printf("Memory allocation failed");
+		fprintf(stderr, "Memory allocation failed");
 		return -ENOMEM;
 	}
 
@@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename, const char *basedir, char *str)
 	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
 
 	if (temp == NULL) {
-		printf("Memory allocation failed");
+		fprintf(stderr, "Memory allocation failed");
 		return -ENOMEM;
 	}
 
-- 
1.9.1

[PATCH v2 2/4] tools: iio: Send usage error messages to stderr

[Cristina Opriceana]

Incorrect usage messages should be sent to stderr.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
 tools/iio/generic_buffer.c    | 2 +-
 tools/iio/iio_event_monitor.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index 2f4e12f..af350cc 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -193,7 +193,7 @@ void process_scan(char *data,
 
 void print_usage(void)
 {
-	printf("Usage: generic_buffer [options]...\n"
+	fprintf(stderr, "Usage: generic_buffer [options]...\n"
 	       "Capture, convert and output data from IIO device buffer\n"
 	       "  -c <n>     Do n conversions\n"
 	       "  -e         Disable wait for event (new data)\n"
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index 843bc4c..1c138fc 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -251,7 +251,7 @@ int main(int argc, char **argv)
 	int fd, event_fd;
 
 	if (argc <= 1) {
-		printf("Usage: %s <device_name>\n", argv[0]);
+		fprintf(stderr, "Usage: %s <device_name>\n", argv[0]);
 		return -1;
 	}
 
-- 
1.9.1

[PATCH v2 3/4] tools: iio: generic_buffer: Maintain fprintf() messages consistent

[Cristina Opriceana]

Ensure that both of the error messages get to the
same standard stream.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
 tools/iio/generic_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index af350cc..ac9b954 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -325,7 +325,7 @@ int main(int argc, char **argv)
 	ret = build_channel_array(dev_dir_name, &channels, &num_channels);
 	if (ret) {
 		fprintf(stderr, "Problem reading scan element information\n");
-		printf("diag %s\n", dev_dir_name);
+		fprintf(stderr, "diag %s\n", dev_dir_name);
 		goto error_free_triggername;
 	}
 
-- 
1.9.1

[PATCH v2 4/4] iio: tools: iio_utils: Move general error messages to stderr

[Cristina Opriceana]

This patch replaces stdout general error messages to a more appropriate
standard stream to ensure consistency.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
 tools/iio/iio_utils.c | 17 ++++++++++-------
 tools/iio/lsiio.c     |  2 +-
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
index 46dfa3f..8b218bd 100644
--- a/tools/iio/iio_utils.c
+++ b/tools/iio/iio_utils.c
@@ -158,7 +158,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
 				goto error_close_sysfsfp;
 			} else if (ret != 5) {
 				ret = -EIO;
-				printf("scan type description didn't match\n");
+				fprintf(stderr,
+					"scan type description didn't match\n");
 				goto error_close_sysfsfp;
 			}
 
@@ -572,7 +573,7 @@ int find_type_by_name(const char *name, const char *type)
 
 	dp = opendir(iio_dir);
 	if (dp == NULL) {
-		printf("No industrialio devices available\n");
+		fprintf(stderr, "No industrialio devices available\n");
 		return -ENODEV;
 	}
 
@@ -709,8 +710,9 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
 		}
 
 		if (test != val) {
-			printf("Possible failure in int write %d to %s/%s\n",
-			       val, basedir, filename);
+			fprintf(stderr,
+				"Possible failure in int write %d to %s/%s\n",
+				val, basedir, filename);
 			ret = -1;
 		}
 	}
@@ -806,9 +808,10 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
 		}
 
 		if (strcmp(temp, val) != 0) {
-			printf("Possible failure in string write of %s "
-			       "Should be %s written to %s/%s\n", temp, val,
-			       basedir, filename);
+			fprintf(stderr,
+				"Possible failure in string write of %s "
+				"Should be %s written to %s/%s\n", temp, val,
+				basedir, filename);
 			ret = -1;
 		}
 	}
diff --git a/tools/iio/lsiio.c b/tools/iio/lsiio.c
index 7f432a5..31996de 100644
--- a/tools/iio/lsiio.c
+++ b/tools/iio/lsiio.c
@@ -108,7 +108,7 @@ static int dump_devices(void)
 
 	dp = opendir(iio_dir);
 	if (dp == NULL) {
-		printf("No industrial I/O devices available\n");
+		fprintf(stderr, "No industrial I/O devices available\n");
 		return -ENODEV;
 	}
 
-- 
1.9.1

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Hartmut Knaack]

Cristina Opriceana schrieb am 10.07.2015 um 12:56:
> Replace printf error messages with fprintf(stderr, ...) in order
> to ensure consistency and to make faults easier to identify.
> This patch uses coccinelle to detect and apply the changes.
> 

Hi Cristina,
I just had a look at the series. You have all cases I regard necessary
covered. There are however a few cases which probably qualify as error
messages, too. Please see inline.
However, for my personal taste, this could have been merged all in a
single patch. Especially the third patch should have been included in
this one (as during review, people certainly think that you missed the
second line, just to find it fixed two patches later).

> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> ---
> Changes in v2:
>  - s/failiure/failure
> 
>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
>  tools/iio/iio_event_monitor.c |  6 +++---
>  tools/iio/iio_utils.c         | 34 ++++++++++++++++++++--------------
>  3 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
> index 0e73723..2f4e12f 100644
> --- a/tools/iio/generic_buffer.c
> +++ b/tools/iio/generic_buffer.c
> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (device_name == NULL) {
> -		printf("Device name not set\n");
> +		fprintf(stderr, "Device name not set\n");
>  		print_usage();
>  		return -1;
>  	}
> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
>  	/* Find the device requested */
>  	dev_num = find_type_by_name(device_name, "iio:device");
>  	if (dev_num < 0) {
> -		printf("Failed to find the %s\n", device_name);
> +		fprintf(stderr, "Failed to find the %s\n", device_name);
>  		return dev_num;
>  	}
>  
> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
>  		/* Verify the trigger exists */
>  		trig_num = find_type_by_name(trigger_name, "trigger");
>  		if (trig_num < 0) {
> -			printf("Failed to find the trigger %s\n", trigger_name);
> +			fprintf(stderr, "Failed to find the trigger %s\n",
> +				trigger_name);
>  			ret = trig_num;
>  			goto error_free_triggername;
>  		}
> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
>  	 */
>  	ret = build_channel_array(dev_dir_name, &channels, &num_channels);
>  	if (ret) {
> -		printf("Problem reading scan element information\n");
> +		fprintf(stderr, "Problem reading scan element information\n");
>  		printf("diag %s\n", dev_dir_name);

My preference would even be to print it all in just one fprintf.

>  		goto error_free_triggername;
>  	}
> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
>  						    dev_dir_name,
>  						    trigger_name);
>  		if (ret < 0) {
> -			printf("Failed to write current_trigger file\n");
> +			fprintf(stderr,
> +				"Failed to write current_trigger file\n");
>  			goto error_free_buf_dir_name;
>  		}
>  	}
> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
>  	fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>  	if (fp == -1) { /* TODO: If it isn't there make the node */
>  		ret = -errno;
> -		printf("Failed to open %s\n", buffer_access);
> +		fprintf(stderr, "Failed to open %s\n", buffer_access);
>  		goto error_free_buffer_access;
>  	}
>  

At line 410 we have a block:
		read_size = read(fp, data, toread * scan_size);
		if (read_size < 0) {
			if (errno == EAGAIN) {
				printf("nothing available\n");
				continue;

I'm tempted to say,that this should go to stderr, as well. Any opinions?

> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
>  		ret = write_sysfs_string("trigger/current_trigger",
>  					 dev_dir_name, "NULL");
>  		if (ret < 0)
> -			printf("Failed to write to %s\n", dev_dir_name);
> +			fprintf(stderr, "Failed to write to %s\n",
> +				dev_dir_name);
>  
>  error_close_buffer_access:
>  	if (close(fp) == -1)
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index 703f4cb..843bc4c 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c

At line 217:
	if (!event_is_known(event)) {
		printf("Unknown event: time: %lld, id: %llx\n",
		       event->timestamp, event->id);

		return;
Better have this on stderr, as well?

> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
>  	fd = open(chrdev_name, 0);
>  	if (fd == -1) {
>  		ret = -errno;
> -		fprintf(stdout, "Failed to open %s\n", chrdev_name);
> +		fprintf(stderr, "Failed to open %s\n", chrdev_name);
>  		goto error_free_chrdev_name;
>  	}
>  
>  	ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
>  	if (ret == -1 || event_fd == -1) {
>  		ret = -errno;
> -		fprintf(stdout, "Failed to retrieve event fd\n");
> +		fprintf(stderr, "Failed to retrieve event fd\n");
>  		if (close(fd) == -1)
>  			perror("Failed to close character device file");
>  

A similar borderline case as above in line 301:
		ret = read(event_fd, &event, sizeof(event));
		if (ret == -1) {
			if (errno == EAGAIN) {
				printf("nothing available\n");
				continue;

> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
>  		}
>  
>  		if (ret != sizeof(event)) {
> -			printf("Reading event failed!\n");
> +			fprintf(stderr, "Reading event failed!\n");
>  			ret = -EIO;
>  			break;
>  		}
> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> index 8fb3214..46dfa3f 100644
> --- a/tools/iio/iio_utils.c
> +++ b/tools/iio/iio_utils.c
> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>  			sysfsfp = fopen(filename, "r");
>  			if (sysfsfp == NULL) {
>  				ret = -errno;
> -				printf("failed to open %s\n", filename);
> +				fprintf(stderr, "failed to open %s\n",
> +					filename);
>  				goto error_free_filename;
>  			}
>  
> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>  				     &padint, shift);
>  			if (ret < 0) {
>  				ret = -errno;
> -				printf("failed to pass scan type description\n");
> +				fprintf(stderr,
> +					"failed to pass scan type description\n");
>  				goto error_close_sysfsfp;
>  			} else if (ret != 5) {
>  				ret = -EIO;
> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>  			*is_signed = (signchar == 's');
>  			if (fclose(sysfsfp)) {
>  				ret = -errno;
> -				printf("Failed to close %s\n", filename);
> +				fprintf(stderr, "Failed to close %s\n",
> +					filename);
>  				goto error_free_filename;
>  			}
>  
> @@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
>  			sysfsfp = fopen(filename, "r");
>  			if (sysfsfp == NULL) {
>  				ret = -errno;
> -				printf("failed to open %s\n", filename);
> +				fprintf(stderr, "failed to open %s\n",
> +					filename);
>  				free(filename);
>  				goto error_cleanup_array;
>  			}
> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const char *type)
>  			ret = sscanf(ent->d_name + strlen(type), "%d", &number);
>  			if (ret < 0) {
>  				ret = -errno;
> -				printf("failed to read element number\n");
> +				fprintf(stderr,
> +					"failed to read element number\n");
>  				goto error_close_dir;
>  			} else if (ret != 1) {
>  				ret = -EIO;
> -				printf("failed to match element number\n");
> +				fprintf(stderr,
> +					"failed to match element number\n");
>  				goto error_close_dir;
>  			}
>  
> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>  	sysfsfp = fopen(temp, "w");
>  	if (sysfsfp == NULL) {
>  		ret = -errno;
> -		printf("failed to open %s\n", temp);
> +		fprintf(stderr, "failed to open %s\n", temp);
>  		goto error_free;
>  	}
>  
> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>  		sysfsfp = fopen(temp, "r");
>  		if (sysfsfp == NULL) {
>  			ret = -errno;
> -			printf("failed to open %s\n", temp);
> +			fprintf(stderr, "failed to open %s\n", temp);
>  			goto error_free;
>  		}
>  
> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>  	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>  
>  	if (temp == NULL) {
> -		printf("Memory allocation failed\n");
> +		fprintf(stderr, "Memory allocation failed\n");
>  		return -ENOMEM;
>  	}
>  
> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>  	sysfsfp = fopen(temp, "w");
>  	if (sysfsfp == NULL) {
>  		ret = -errno;
> -		printf("Could not open %s\n", temp);
> +		fprintf(stderr, "Could not open %s\n", temp);
>  		goto error_free;
>  	}
>  
> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>  		sysfsfp = fopen(temp, "r");
>  		if (sysfsfp == NULL) {
>  			ret = -errno;
> -			printf("Could not open file to verify\n");
> +			fprintf(stderr, "Could not open file to verify\n");
>  			goto error_free;
>  		}
>  
> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename, const char *basedir)
>  	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>  
>  	if (temp == NULL) {
> -		printf("Memory allocation failed");
> +		fprintf(stderr, "Memory allocation failed");
>  		return -ENOMEM;
>  	}
>  
> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename, const char *basedir, float *val)
>  	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>  
>  	if (temp == NULL) {
> -		printf("Memory allocation failed");
> +		fprintf(stderr, "Memory allocation failed");
>  		return -ENOMEM;
>  	}
>  
> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename, const char *basedir, char *str)
>  	char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>  
>  	if (temp == NULL) {
> -		printf("Memory allocation failed");
> +		fprintf(stderr, "Memory allocation failed");
>  		return -ENOMEM;
>  	}
>  
> 

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Cristina Georgiana Opriceana]

On Sat, Jul 11, 2015 at 12:42 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Cristina Opriceana schrieb am 10.07.2015 um 12:56:
>> Replace printf error messages with fprintf(stderr, ...) in order
>> to ensure consistency and to make faults easier to identify.
>> This patch uses coccinelle to detect and apply the changes.
>>
>
> Hi Cristina,
> I just had a look at the series. You have all cases I regard necessary
> covered. There are however a few cases which probably qualify as error
> messages, too. Please see inline.
> However, for my personal taste, this could have been merged all in a
> single patch. Especially the third patch should have been included in
> this one (as during review, people certainly think that you missed the
> second line, just to find it fixed two patches later).

Hi,

Yes, I could have included all in a single patch, but I tried to
automatize this task and build a rather generic semantic patch in
coccinelle for the substitutions. Had I included all in one patch, the
changes with coccinelle wouldn't have been differentiated from the
other ones. If that is okay, I think I can merge them in one patch.

>> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
>> ---
>> Changes in v2:
>>  - s/failiure/failure
>>
>>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
>>  tools/iio/iio_event_monitor.c |  6 +++---
>>  tools/iio/iio_utils.c         | 34 ++++++++++++++++++++--------------
>>  3 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
>> index 0e73723..2f4e12f 100644
>> --- a/tools/iio/generic_buffer.c
>> +++ b/tools/iio/generic_buffer.c
>> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
>>       }
>>
>>       if (device_name == NULL) {
>> -             printf("Device name not set\n");
>> +             fprintf(stderr, "Device name not set\n");
>>               print_usage();
>>               return -1;
>>       }
>> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
>>       /* Find the device requested */
>>       dev_num = find_type_by_name(device_name, "iio:device");
>>       if (dev_num < 0) {
>> -             printf("Failed to find the %s\n", device_name);
>> +             fprintf(stderr, "Failed to find the %s\n", device_name);
>>               return dev_num;
>>       }
>>
>> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
>>               /* Verify the trigger exists */
>>               trig_num = find_type_by_name(trigger_name, "trigger");
>>               if (trig_num < 0) {
>> -                     printf("Failed to find the trigger %s\n", trigger_name);
>> +                     fprintf(stderr, "Failed to find the trigger %s\n",
>> +                             trigger_name);
>>                       ret = trig_num;
>>                       goto error_free_triggername;
>>               }
>> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
>>        */
>>       ret = build_channel_array(dev_dir_name, &channels, &num_channels);
>>       if (ret) {
>> -             printf("Problem reading scan element information\n");
>> +             fprintf(stderr, "Problem reading scan element information\n");
>>               printf("diag %s\n", dev_dir_name);
>
> My preference would even be to print it all in just one fprintf.

I thought so, also, but the string would go beyond 80 characters and
would have to be split which is ugly and brings a warning on it.

>>               goto error_free_triggername;
>>       }
>> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
>>                                                   dev_dir_name,
>>                                                   trigger_name);
>>               if (ret < 0) {
>> -                     printf("Failed to write current_trigger file\n");
>> +                     fprintf(stderr,
>> +                             "Failed to write current_trigger file\n");
>>                       goto error_free_buf_dir_name;
>>               }
>>       }
>> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
>>       fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>>       if (fp == -1) { /* TODO: If it isn't there make the node */
>>               ret = -errno;
>> -             printf("Failed to open %s\n", buffer_access);
>> +             fprintf(stderr, "Failed to open %s\n", buffer_access);
>>               goto error_free_buffer_access;
>>       }
>>
>
> At line 410 we have a block:
>                 read_size = read(fp, data, toread * scan_size);
>                 if (read_size < 0) {
>                         if (errno == EAGAIN) {
>                                 printf("nothing available\n");
>                                 continue;
>
> I'm tempted to say,that this should go to stderr, as well. Any opinions?

I see it more as an informing note, since the device continues looping
for data, but it could be considered an error as well.

>> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
>>               ret = write_sysfs_string("trigger/current_trigger",
>>                                        dev_dir_name, "NULL");
>>               if (ret < 0)
>> -                     printf("Failed to write to %s\n", dev_dir_name);
>> +                     fprintf(stderr, "Failed to write to %s\n",
>> +                             dev_dir_name);
>>
>>  error_close_buffer_access:
>>       if (close(fp) == -1)
>> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
>> index 703f4cb..843bc4c 100644
>> --- a/tools/iio/iio_event_monitor.c
>> +++ b/tools/iio/iio_event_monitor.c
>
> At line 217:
>         if (!event_is_known(event)) {
>                 printf("Unknown event: time: %lld, id: %llx\n",
>                        event->timestamp, event->id);
>
>                 return;
> Better have this on stderr, as well?

This is more suitable for stderr, indeed.

>> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
>>       fd = open(chrdev_name, 0);
>>       if (fd == -1) {
>>               ret = -errno;
>> -             fprintf(stdout, "Failed to open %s\n", chrdev_name);
>> +             fprintf(stderr, "Failed to open %s\n", chrdev_name);
>>               goto error_free_chrdev_name;
>>       }
>>
>>       ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
>>       if (ret == -1 || event_fd == -1) {
>>               ret = -errno;
>> -             fprintf(stdout, "Failed to retrieve event fd\n");
>> +             fprintf(stderr, "Failed to retrieve event fd\n");
>>               if (close(fd) == -1)
>>                       perror("Failed to close character device file");
>>
>
> A similar borderline case as above in line 301:
>                 ret = read(event_fd, &event, sizeof(event));
>                 if (ret == -1) {
>                         if (errno == EAGAIN) {
>                                 printf("nothing available\n");
>                                 continue;
>
>> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
>>               }
>>
>>               if (ret != sizeof(event)) {
>> -                     printf("Reading event failed!\n");
>> +                     fprintf(stderr, "Reading event failed!\n");
>>                       ret = -EIO;
>>                       break;
>>               }
>> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
>> index 8fb3214..46dfa3f 100644
>> --- a/tools/iio/iio_utils.c
>> +++ b/tools/iio/iio_utils.c
>> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>                       sysfsfp = fopen(filename, "r");
>>                       if (sysfsfp == NULL) {
>>                               ret = -errno;
>> -                             printf("failed to open %s\n", filename);
>> +                             fprintf(stderr, "failed to open %s\n",
>> +                                     filename);
>>                               goto error_free_filename;
>>                       }
>>
>> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>                                    &padint, shift);
>>                       if (ret < 0) {
>>                               ret = -errno;
>> -                             printf("failed to pass scan type description\n");
>> +                             fprintf(stderr,
>> +                                     "failed to pass scan type description\n");
>>                               goto error_close_sysfsfp;
>>                       } else if (ret != 5) {
>>                               ret = -EIO;
>> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>                       *is_signed = (signchar == 's');
>>                       if (fclose(sysfsfp)) {
>>                               ret = -errno;
>> -                             printf("Failed to close %s\n", filename);
>> +                             fprintf(stderr, "Failed to close %s\n",
>> +                                     filename);
>>                               goto error_free_filename;
>>                       }
>>
>> @@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
>>                       sysfsfp = fopen(filename, "r");
>>                       if (sysfsfp == NULL) {
>>                               ret = -errno;
>> -                             printf("failed to open %s\n", filename);
>> +                             fprintf(stderr, "failed to open %s\n",
>> +                                     filename);
>>                               free(filename);
>>                               goto error_cleanup_array;
>>                       }
>> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const char *type)
>>                       ret = sscanf(ent->d_name + strlen(type), "%d", &number);
>>                       if (ret < 0) {
>>                               ret = -errno;
>> -                             printf("failed to read element number\n");
>> +                             fprintf(stderr,
>> +                                     "failed to read element number\n");
>>                               goto error_close_dir;
>>                       } else if (ret != 1) {
>>                               ret = -EIO;
>> -                             printf("failed to match element number\n");
>> +                             fprintf(stderr,
>> +                                     "failed to match element number\n");
>>                               goto error_close_dir;
>>                       }
>>
>> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>>       sysfsfp = fopen(temp, "w");
>>       if (sysfsfp == NULL) {
>>               ret = -errno;
>> -             printf("failed to open %s\n", temp);
>> +             fprintf(stderr, "failed to open %s\n", temp);
>>               goto error_free;
>>       }
>>
>> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>>               sysfsfp = fopen(temp, "r");
>>               if (sysfsfp == NULL) {
>>                       ret = -errno;
>> -                     printf("failed to open %s\n", temp);
>> +                     fprintf(stderr, "failed to open %s\n", temp);
>>                       goto error_free;
>>               }
>>
>> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>
>>       if (temp == NULL) {
>> -             printf("Memory allocation failed\n");
>> +             fprintf(stderr, "Memory allocation failed\n");
>>               return -ENOMEM;
>>       }
>>
>> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>       sysfsfp = fopen(temp, "w");
>>       if (sysfsfp == NULL) {
>>               ret = -errno;
>> -             printf("Could not open %s\n", temp);
>> +             fprintf(stderr, "Could not open %s\n", temp);
>>               goto error_free;
>>       }
>>
>> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>               sysfsfp = fopen(temp, "r");
>>               if (sysfsfp == NULL) {
>>                       ret = -errno;
>> -                     printf("Could not open file to verify\n");
>> +                     fprintf(stderr, "Could not open file to verify\n");
>>                       goto error_free;
>>               }
>>
>> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename, const char *basedir)
>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>
>>       if (temp == NULL) {
>> -             printf("Memory allocation failed");
>> +             fprintf(stderr, "Memory allocation failed");
>>               return -ENOMEM;
>>       }
>>
>> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename, const char *basedir, float *val)
>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>
>>       if (temp == NULL) {
>> -             printf("Memory allocation failed");
>> +             fprintf(stderr, "Memory allocation failed");
>>               return -ENOMEM;
>>       }
>>
>> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename, const char *basedir, char *str)
>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>
>>       if (temp == NULL) {
>> -             printf("Memory allocation failed");
>> +             fprintf(stderr, "Memory allocation failed");
>>               return -ENOMEM;
>>       }
>>
>>
>

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Julia Lawall]

> Yes, I could have included all in a single patch, but I tried to
> automatize this task and build a rather generic semantic patch in
> coccinelle for the substitutions. Had I included all in one patch, the
> changes with coccinelle wouldn't have been differentiated from the
> other ones. If that is okay, I think I can merge them in one patch.

If it seems important, then you can say in the commit message what was done
by hand.

julia


> >> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> >> ---
> >> Changes in v2:
> >>  - s/failiure/failure
> >>
> >>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
> >>  tools/iio/iio_event_monitor.c |  6 +++---
> >>  tools/iio/iio_utils.c         | 34 ++++++++++++++++++++--------------
> >>  3 files changed, 33 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
> >> index 0e73723..2f4e12f 100644
> >> --- a/tools/iio/generic_buffer.c
> >> +++ b/tools/iio/generic_buffer.c
> >> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
> >>       }
> >>
> >>       if (device_name == NULL) {
> >> -             printf("Device name not set\n");
> >> +             fprintf(stderr, "Device name not set\n");
> >>               print_usage();
> >>               return -1;
> >>       }
> >> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
> >>       /* Find the device requested */
> >>       dev_num = find_type_by_name(device_name, "iio:device");
> >>       if (dev_num < 0) {
> >> -             printf("Failed to find the %s\n", device_name);
> >> +             fprintf(stderr, "Failed to find the %s\n", device_name);
> >>               return dev_num;
> >>       }
> >>
> >> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
> >>               /* Verify the trigger exists */
> >>               trig_num = find_type_by_name(trigger_name, "trigger");
> >>               if (trig_num < 0) {
> >> -                     printf("Failed to find the trigger %s\n", trigger_name);
> >> +                     fprintf(stderr, "Failed to find the trigger %s\n",
> >> +                             trigger_name);
> >>                       ret = trig_num;
> >>                       goto error_free_triggername;
> >>               }
> >> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
> >>        */
> >>       ret = build_channel_array(dev_dir_name, &channels, &num_channels);
> >>       if (ret) {
> >> -             printf("Problem reading scan element information\n");
> >> +             fprintf(stderr, "Problem reading scan element information\n");
> >>               printf("diag %s\n", dev_dir_name);
> >
> > My preference would even be to print it all in just one fprintf.
>
> I thought so, also, but the string would go beyond 80 characters and
> would have to be split which is ugly and brings a warning on it.
>
> >>               goto error_free_triggername;
> >>       }
> >> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
> >>                                                   dev_dir_name,
> >>                                                   trigger_name);
> >>               if (ret < 0) {
> >> -                     printf("Failed to write current_trigger file\n");
> >> +                     fprintf(stderr,
> >> +                             "Failed to write current_trigger file\n");
> >>                       goto error_free_buf_dir_name;
> >>               }
> >>       }
> >> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
> >>       fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
> >>       if (fp == -1) { /* TODO: If it isn't there make the node */
> >>               ret = -errno;
> >> -             printf("Failed to open %s\n", buffer_access);
> >> +             fprintf(stderr, "Failed to open %s\n", buffer_access);
> >>               goto error_free_buffer_access;
> >>       }
> >>
> >
> > At line 410 we have a block:
> >                 read_size = read(fp, data, toread * scan_size);
> >                 if (read_size < 0) {
> >                         if (errno == EAGAIN) {
> >                                 printf("nothing available\n");
> >                                 continue;
> >
> > I'm tempted to say,that this should go to stderr, as well. Any opinions?
>
> I see it more as an informing note, since the device continues looping
> for data, but it could be considered an error as well.
>
> >> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
> >>               ret = write_sysfs_string("trigger/current_trigger",
> >>                                        dev_dir_name, "NULL");
> >>               if (ret < 0)
> >> -                     printf("Failed to write to %s\n", dev_dir_name);
> >> +                     fprintf(stderr, "Failed to write to %s\n",
> >> +                             dev_dir_name);
> >>
> >>  error_close_buffer_access:
> >>       if (close(fp) == -1)
> >> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> >> index 703f4cb..843bc4c 100644
> >> --- a/tools/iio/iio_event_monitor.c
> >> +++ b/tools/iio/iio_event_monitor.c
> >
> > At line 217:
> >         if (!event_is_known(event)) {
> >                 printf("Unknown event: time: %lld, id: %llx\n",
> >                        event->timestamp, event->id);
> >
> >                 return;
> > Better have this on stderr, as well?
>
> This is more suitable for stderr, indeed.
>
> >> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
> >>       fd = open(chrdev_name, 0);
> >>       if (fd == -1) {
> >>               ret = -errno;
> >> -             fprintf(stdout, "Failed to open %s\n", chrdev_name);
> >> +             fprintf(stderr, "Failed to open %s\n", chrdev_name);
> >>               goto error_free_chrdev_name;
> >>       }
> >>
> >>       ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
> >>       if (ret == -1 || event_fd == -1) {
> >>               ret = -errno;
> >> -             fprintf(stdout, "Failed to retrieve event fd\n");
> >> +             fprintf(stderr, "Failed to retrieve event fd\n");
> >>               if (close(fd) == -1)
> >>                       perror("Failed to close character device file");
> >>
> >
> > A similar borderline case as above in line 301:
> >                 ret = read(event_fd, &event, sizeof(event));
> >                 if (ret == -1) {
> >                         if (errno == EAGAIN) {
> >                                 printf("nothing available\n");
> >                                 continue;
> >
> >> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
> >>               }
> >>
> >>               if (ret != sizeof(event)) {
> >> -                     printf("Reading event failed!\n");
> >> +                     fprintf(stderr, "Reading event failed!\n");
> >>                       ret = -EIO;
> >>                       break;
> >>               }
> >> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> >> index 8fb3214..46dfa3f 100644
> >> --- a/tools/iio/iio_utils.c
> >> +++ b/tools/iio/iio_utils.c
> >> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
> >>                       sysfsfp = fopen(filename, "r");
> >>                       if (sysfsfp == NULL) {
> >>                               ret = -errno;
> >> -                             printf("failed to open %s\n", filename);
> >> +                             fprintf(stderr, "failed to open %s\n",
> >> +                                     filename);
> >>                               goto error_free_filename;
> >>                       }
> >>
> >> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
> >>                                    &padint, shift);
> >>                       if (ret < 0) {
> >>                               ret = -errno;
> >> -                             printf("failed to pass scan type description\n");
> >> +                             fprintf(stderr,
> >> +                                     "failed to pass scan type description\n");
> >>                               goto error_close_sysfsfp;
> >>                       } else if (ret != 5) {
> >>                               ret = -EIO;
> >> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
> >>                       *is_signed = (signchar == 's');
> >>                       if (fclose(sysfsfp)) {
> >>                               ret = -errno;
> >> -                             printf("Failed to close %s\n", filename);
> >> +                             fprintf(stderr, "Failed to close %s\n",
> >> +                                     filename);
> >>                               goto error_free_filename;
> >>                       }
> >>
> >> @@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
> >>                       sysfsfp = fopen(filename, "r");
> >>                       if (sysfsfp == NULL) {
> >>                               ret = -errno;
> >> -                             printf("failed to open %s\n", filename);
> >> +                             fprintf(stderr, "failed to open %s\n",
> >> +                                     filename);
> >>                               free(filename);
> >>                               goto error_cleanup_array;
> >>                       }
> >> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const char *type)
> >>                       ret = sscanf(ent->d_name + strlen(type), "%d", &number);
> >>                       if (ret < 0) {
> >>                               ret = -errno;
> >> -                             printf("failed to read element number\n");
> >> +                             fprintf(stderr,
> >> +                                     "failed to read element number\n");
> >>                               goto error_close_dir;
> >>                       } else if (ret != 1) {
> >>                               ret = -EIO;
> >> -                             printf("failed to match element number\n");
> >> +                             fprintf(stderr,
> >> +                                     "failed to match element number\n");
> >>                               goto error_close_dir;
> >>                       }
> >>
> >> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
> >>       sysfsfp = fopen(temp, "w");
> >>       if (sysfsfp == NULL) {
> >>               ret = -errno;
> >> -             printf("failed to open %s\n", temp);
> >> +             fprintf(stderr, "failed to open %s\n", temp);
> >>               goto error_free;
> >>       }
> >>
> >> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
> >>               sysfsfp = fopen(temp, "r");
> >>               if (sysfsfp == NULL) {
> >>                       ret = -errno;
> >> -                     printf("failed to open %s\n", temp);
> >> +                     fprintf(stderr, "failed to open %s\n", temp);
> >>                       goto error_free;
> >>               }
> >>
> >> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
> >>
> >>       if (temp == NULL) {
> >> -             printf("Memory allocation failed\n");
> >> +             fprintf(stderr, "Memory allocation failed\n");
> >>               return -ENOMEM;
> >>       }
> >>
> >> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
> >>       sysfsfp = fopen(temp, "w");
> >>       if (sysfsfp == NULL) {
> >>               ret = -errno;
> >> -             printf("Could not open %s\n", temp);
> >> +             fprintf(stderr, "Could not open %s\n", temp);
> >>               goto error_free;
> >>       }
> >>
> >> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
> >>               sysfsfp = fopen(temp, "r");
> >>               if (sysfsfp == NULL) {
> >>                       ret = -errno;
> >> -                     printf("Could not open file to verify\n");
> >> +                     fprintf(stderr, "Could not open file to verify\n");
> >>                       goto error_free;
> >>               }
> >>
> >> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename, const char *basedir)
> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
> >>
> >>       if (temp == NULL) {
> >> -             printf("Memory allocation failed");
> >> +             fprintf(stderr, "Memory allocation failed");
> >>               return -ENOMEM;
> >>       }
> >>
> >> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename, const char *basedir, float *val)
> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
> >>
> >>       if (temp == NULL) {
> >> -             printf("Memory allocation failed");
> >> +             fprintf(stderr, "Memory allocation failed");
> >>               return -ENOMEM;
> >>       }
> >>
> >> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename, const char *basedir, char *str)
> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
> >>
> >>       if (temp == NULL) {
> >> -             printf("Memory allocation failed");
> >> +             fprintf(stderr, "Memory allocation failed");
> >>               return -ENOMEM;
> >>       }
> >>
> >>
> >
>

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Hartmut Knaack]

Cristina Georgiana Opriceana schrieb am 12.07.2015 um 13:38:
> On Sat, Jul 11, 2015 at 12:42 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> Cristina Opriceana schrieb am 10.07.2015 um 12:56:
>>> Replace printf error messages with fprintf(stderr, ...) in order
>>> to ensure consistency and to make faults easier to identify.
>>> This patch uses coccinelle to detect and apply the changes.
>>>
>>
>> Hi Cristina,
>> I just had a look at the series. You have all cases I regard necessary
>> covered. There are however a few cases which probably qualify as error
>> messages, too. Please see inline.
>> However, for my personal taste, this could have been merged all in a
>> single patch. Especially the third patch should have been included in
>> this one (as during review, people certainly think that you missed the
>> second line, just to find it fixed two patches later).
> 
> Hi,
> 
> Yes, I could have included all in a single patch, but I tried to
> automatize this task and build a rather generic semantic patch in
> coccinelle for the substitutions. Had I included all in one patch, the
> changes with coccinelle wouldn't have been differentiated from the
> other ones. If that is okay, I think I can merge them in one patch.
> 

Point taken for making it reproducible. I just like to raise awareness:
The maintainers are very busy. They have a full time job, respond to the
mailing list, review patches and do all the git magic. That keeps them
pretty busy, so I would aim to make the maintainers life as easy as possible.
Checking just one patch against some sources can be quicker/easier than
checking multiple ones. Same goes for applying patches.
Other than that, I just feel strong about the third patch standing separate,
which I don't regard a good idea. Either your script should have been
extended to catch such cases, or if it is not worth the effort, it should
have been changed by hand. But it should have gone in one pass.

>>> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
>>> ---
>>> Changes in v2:
>>>  - s/failiure/failure
>>>
>>>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
>>>  tools/iio/iio_event_monitor.c |  6 +++---
>>>  tools/iio/iio_utils.c         | 34 ++++++++++++++++++++--------------
>>>  3 files changed, 33 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
>>> index 0e73723..2f4e12f 100644
>>> --- a/tools/iio/generic_buffer.c
>>> +++ b/tools/iio/generic_buffer.c
>>> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
>>>       }
>>>
>>>       if (device_name == NULL) {
>>> -             printf("Device name not set\n");
>>> +             fprintf(stderr, "Device name not set\n");
>>>               print_usage();
>>>               return -1;
>>>       }
>>> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
>>>       /* Find the device requested */
>>>       dev_num = find_type_by_name(device_name, "iio:device");
>>>       if (dev_num < 0) {
>>> -             printf("Failed to find the %s\n", device_name);
>>> +             fprintf(stderr, "Failed to find the %s\n", device_name);
>>>               return dev_num;
>>>       }
>>>
>>> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
>>>               /* Verify the trigger exists */
>>>               trig_num = find_type_by_name(trigger_name, "trigger");
>>>               if (trig_num < 0) {
>>> -                     printf("Failed to find the trigger %s\n", trigger_name);
>>> +                     fprintf(stderr, "Failed to find the trigger %s\n",
>>> +                             trigger_name);
>>>                       ret = trig_num;
>>>                       goto error_free_triggername;
>>>               }
>>> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
>>>        */
>>>       ret = build_channel_array(dev_dir_name, &channels, &num_channels);
>>>       if (ret) {
>>> -             printf("Problem reading scan element information\n");
>>> +             fprintf(stderr, "Problem reading scan element information\n");
>>>               printf("diag %s\n", dev_dir_name);
>>
>> My preference would even be to print it all in just one fprintf.
> 
> I thought so, also, but the string would go beyond 80 characters and
> would have to be split which is ugly and brings a warning on it.
> 

I just gave it a try compile testing:
		fprintf(stderr, "Problem reading scan element information\n"
			"diag %s\n", dev_dir_name);

Compiled just fine, and the same format is used in your second patch as well.

>>>               goto error_free_triggername;
>>>       }
>>> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
>>>                                                   dev_dir_name,
>>>                                                   trigger_name);
>>>               if (ret < 0) {
>>> -                     printf("Failed to write current_trigger file\n");
>>> +                     fprintf(stderr,
>>> +                             "Failed to write current_trigger file\n");
>>>                       goto error_free_buf_dir_name;
>>>               }
>>>       }
>>> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
>>>       fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>>>       if (fp == -1) { /* TODO: If it isn't there make the node */
>>>               ret = -errno;
>>> -             printf("Failed to open %s\n", buffer_access);
>>> +             fprintf(stderr, "Failed to open %s\n", buffer_access);
>>>               goto error_free_buffer_access;
>>>       }
>>>
>>
>> At line 410 we have a block:
>>                 read_size = read(fp, data, toread * scan_size);
>>                 if (read_size < 0) {
>>                         if (errno == EAGAIN) {
>>                                 printf("nothing available\n");
>>                                 continue;
>>
>> I'm tempted to say,that this should go to stderr, as well. Any opinions?
> 
> I see it more as an informing note, since the device continues looping
> for data, but it could be considered an error as well.

I thought about the case when stdout gets piped to a data file while stderr
goes into a logfile (or an application reads one data stream while checking
the error stream).

> 
>>> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
>>>               ret = write_sysfs_string("trigger/current_trigger",
>>>                                        dev_dir_name, "NULL");
>>>               if (ret < 0)
>>> -                     printf("Failed to write to %s\n", dev_dir_name);
>>> +                     fprintf(stderr, "Failed to write to %s\n",
>>> +                             dev_dir_name);
>>>
>>>  error_close_buffer_access:
>>>       if (close(fp) == -1)
>>> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
>>> index 703f4cb..843bc4c 100644
>>> --- a/tools/iio/iio_event_monitor.c
>>> +++ b/tools/iio/iio_event_monitor.c
>>
>> At line 217:
>>         if (!event_is_known(event)) {
>>                 printf("Unknown event: time: %lld, id: %llx\n",
>>                        event->timestamp, event->id);
>>
>>                 return;
>> Better have this on stderr, as well?
> 
> This is more suitable for stderr, indeed.
> 
>>> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
>>>       fd = open(chrdev_name, 0);
>>>       if (fd == -1) {
>>>               ret = -errno;
>>> -             fprintf(stdout, "Failed to open %s\n", chrdev_name);
>>> +             fprintf(stderr, "Failed to open %s\n", chrdev_name);
>>>               goto error_free_chrdev_name;
>>>       }
>>>
>>>       ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
>>>       if (ret == -1 || event_fd == -1) {
>>>               ret = -errno;
>>> -             fprintf(stdout, "Failed to retrieve event fd\n");
>>> +             fprintf(stderr, "Failed to retrieve event fd\n");
>>>               if (close(fd) == -1)
>>>                       perror("Failed to close character device file");
>>>
>>
>> A similar borderline case as above in line 301:
>>                 ret = read(event_fd, &event, sizeof(event));
>>                 if (ret == -1) {
>>                         if (errno == EAGAIN) {
>>                                 printf("nothing available\n");
>>                                 continue;
>>
>>> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
>>>               }
>>>
>>>               if (ret != sizeof(event)) {
>>> -                     printf("Reading event failed!\n");
>>> +                     fprintf(stderr, "Reading event failed!\n");
>>>                       ret = -EIO;
>>>                       break;
>>>               }
>>> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
>>> index 8fb3214..46dfa3f 100644
>>> --- a/tools/iio/iio_utils.c
>>> +++ b/tools/iio/iio_utils.c
>>> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>>                       sysfsfp = fopen(filename, "r");
>>>                       if (sysfsfp == NULL) {
>>>                               ret = -errno;
>>> -                             printf("failed to open %s\n", filename);
>>> +                             fprintf(stderr, "failed to open %s\n",
>>> +                                     filename);
>>>                               goto error_free_filename;
>>>                       }
>>>
>>> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>>                                    &padint, shift);
>>>                       if (ret < 0) {
>>>                               ret = -errno;
>>> -                             printf("failed to pass scan type description\n");
>>> +                             fprintf(stderr,
>>> +                                     "failed to pass scan type description\n");
>>>                               goto error_close_sysfsfp;
>>>                       } else if (ret != 5) {
>>>                               ret = -EIO;
>>> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>>                       *is_signed = (signchar == 's');
>>>                       if (fclose(sysfsfp)) {
>>>                               ret = -errno;
>>> -                             printf("Failed to close %s\n", filename);
>>> +                             fprintf(stderr, "Failed to close %s\n",
>>> +                                     filename);
>>>                               goto error_free_filename;
>>>                       }
>>>
>>> @@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
>>>                       sysfsfp = fopen(filename, "r");
>>>                       if (sysfsfp == NULL) {
>>>                               ret = -errno;
>>> -                             printf("failed to open %s\n", filename);
>>> +                             fprintf(stderr, "failed to open %s\n",
>>> +                                     filename);
>>>                               free(filename);
>>>                               goto error_cleanup_array;
>>>                       }
>>> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const char *type)
>>>                       ret = sscanf(ent->d_name + strlen(type), "%d", &number);
>>>                       if (ret < 0) {
>>>                               ret = -errno;
>>> -                             printf("failed to read element number\n");
>>> +                             fprintf(stderr,
>>> +                                     "failed to read element number\n");
>>>                               goto error_close_dir;
>>>                       } else if (ret != 1) {
>>>                               ret = -EIO;
>>> -                             printf("failed to match element number\n");
>>> +                             fprintf(stderr,
>>> +                                     "failed to match element number\n");
>>>                               goto error_close_dir;
>>>                       }
>>>
>>> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>>>       sysfsfp = fopen(temp, "w");
>>>       if (sysfsfp == NULL) {
>>>               ret = -errno;
>>> -             printf("failed to open %s\n", temp);
>>> +             fprintf(stderr, "failed to open %s\n", temp);
>>>               goto error_free;
>>>       }
>>>
>>> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>>>               sysfsfp = fopen(temp, "r");
>>>               if (sysfsfp == NULL) {
>>>                       ret = -errno;
>>> -                     printf("failed to open %s\n", temp);
>>> +                     fprintf(stderr, "failed to open %s\n", temp);
>>>                       goto error_free;
>>>               }
>>>
>>> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>
>>>       if (temp == NULL) {
>>> -             printf("Memory allocation failed\n");
>>> +             fprintf(stderr, "Memory allocation failed\n");
>>>               return -ENOMEM;
>>>       }
>>>
>>> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>>       sysfsfp = fopen(temp, "w");
>>>       if (sysfsfp == NULL) {
>>>               ret = -errno;
>>> -             printf("Could not open %s\n", temp);
>>> +             fprintf(stderr, "Could not open %s\n", temp);
>>>               goto error_free;
>>>       }
>>>
>>> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>>               sysfsfp = fopen(temp, "r");
>>>               if (sysfsfp == NULL) {
>>>                       ret = -errno;
>>> -                     printf("Could not open file to verify\n");
>>> +                     fprintf(stderr, "Could not open file to verify\n");
>>>                       goto error_free;
>>>               }
>>>
>>> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename, const char *basedir)
>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>
>>>       if (temp == NULL) {
>>> -             printf("Memory allocation failed");
>>> +             fprintf(stderr, "Memory allocation failed");
>>>               return -ENOMEM;
>>>       }
>>>
>>> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename, const char *basedir, float *val)
>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>
>>>       if (temp == NULL) {
>>> -             printf("Memory allocation failed");
>>> +             fprintf(stderr, "Memory allocation failed");
>>>               return -ENOMEM;
>>>       }
>>>
>>> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename, const char *basedir, char *str)
>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>
>>>       if (temp == NULL) {
>>> -             printf("Memory allocation failed");
>>> +             fprintf(stderr, "Memory allocation failed");
>>>               return -ENOMEM;
>>>       }
>>>
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Cristina Georgiana Opriceana]

On Sun, Jul 12, 2015 at 4:07 PM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Cristina Georgiana Opriceana schrieb am 12.07.2015 um 13:38:
>> On Sat, Jul 11, 2015 at 12:42 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Cristina Opriceana schrieb am 10.07.2015 um 12:56:
>>>> Replace printf error messages with fprintf(stderr, ...) in order
>>>> to ensure consistency and to make faults easier to identify.
>>>> This patch uses coccinelle to detect and apply the changes.
>>>>
>>>
>>> Hi Cristina,
>>> I just had a look at the series. You have all cases I regard necessary
>>> covered. There are however a few cases which probably qualify as error
>>> messages, too. Please see inline.
>>> However, for my personal taste, this could have been merged all in a
>>> single patch. Especially the third patch should have been included in
>>> this one (as during review, people certainly think that you missed the
>>> second line, just to find it fixed two patches later).
>>
>> Hi,
>>
>> Yes, I could have included all in a single patch, but I tried to
>> automatize this task and build a rather generic semantic patch in
>> coccinelle for the substitutions. Had I included all in one patch, the
>> changes with coccinelle wouldn't have been differentiated from the
>> other ones. If that is okay, I think I can merge them in one patch.
>>
>
> Point taken for making it reproducible. I just like to raise awareness:
> The maintainers are very busy. They have a full time job, respond to the
> mailing list, review patches and do all the git magic. That keeps them
> pretty busy, so I would aim to make the maintainers life as easy as possible.
> Checking just one patch against some sources can be quicker/easier than
> checking multiple ones. Same goes for applying patches.
> Other than that, I just feel strong about the third patch standing separate,
> which I don't regard a good idea. Either your script should have been
> extended to catch such cases, or if it is not worth the effort, it should
> have been changed by hand. But it should have gone in one pass.

Okay, I'll resend it all merged.
Thanks for the feedback!

Cristina

>>>> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>  - s/failiure/failure
>>>>
>>>>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
>>>>  tools/iio/iio_event_monitor.c |  6 +++---
>>>>  tools/iio/iio_utils.c         | 34 ++++++++++++++++++++--------------
>>>>  3 files changed, 33 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
>>>> index 0e73723..2f4e12f 100644
>>>> --- a/tools/iio/generic_buffer.c
>>>> +++ b/tools/iio/generic_buffer.c
>>>> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
>>>>       }
>>>>
>>>>       if (device_name == NULL) {
>>>> -             printf("Device name not set\n");
>>>> +             fprintf(stderr, "Device name not set\n");
>>>>               print_usage();
>>>>               return -1;
>>>>       }
>>>> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
>>>>       /* Find the device requested */
>>>>       dev_num = find_type_by_name(device_name, "iio:device");
>>>>       if (dev_num < 0) {
>>>> -             printf("Failed to find the %s\n", device_name);
>>>> +             fprintf(stderr, "Failed to find the %s\n", device_name);
>>>>               return dev_num;
>>>>       }
>>>>
>>>> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
>>>>               /* Verify the trigger exists */
>>>>               trig_num = find_type_by_name(trigger_name, "trigger");
>>>>               if (trig_num < 0) {
>>>> -                     printf("Failed to find the trigger %s\n", trigger_name);
>>>> +                     fprintf(stderr, "Failed to find the trigger %s\n",
>>>> +                             trigger_name);
>>>>                       ret = trig_num;
>>>>                       goto error_free_triggername;
>>>>               }
>>>> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
>>>>        */
>>>>       ret = build_channel_array(dev_dir_name, &channels, &num_channels);
>>>>       if (ret) {
>>>> -             printf("Problem reading scan element information\n");
>>>> +             fprintf(stderr, "Problem reading scan element information\n");
>>>>               printf("diag %s\n", dev_dir_name);
>>>
>>> My preference would even be to print it all in just one fprintf.
>>
>> I thought so, also, but the string would go beyond 80 characters and
>> would have to be split which is ugly and brings a warning on it.
>>
>
> I just gave it a try compile testing:
>                 fprintf(stderr, "Problem reading scan element information\n"
>                         "diag %s\n", dev_dir_name);
>
> Compiled just fine, and the same format is used in your second patch as well.
>
>>>>               goto error_free_triggername;
>>>>       }
>>>> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
>>>>                                                   dev_dir_name,
>>>>                                                   trigger_name);
>>>>               if (ret < 0) {
>>>> -                     printf("Failed to write current_trigger file\n");
>>>> +                     fprintf(stderr,
>>>> +                             "Failed to write current_trigger file\n");
>>>>                       goto error_free_buf_dir_name;
>>>>               }
>>>>       }
>>>> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
>>>>       fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>>>>       if (fp == -1) { /* TODO: If it isn't there make the node */
>>>>               ret = -errno;
>>>> -             printf("Failed to open %s\n", buffer_access);
>>>> +             fprintf(stderr, "Failed to open %s\n", buffer_access);
>>>>               goto error_free_buffer_access;
>>>>       }
>>>>
>>>
>>> At line 410 we have a block:
>>>                 read_size = read(fp, data, toread * scan_size);
>>>                 if (read_size < 0) {
>>>                         if (errno == EAGAIN) {
>>>                                 printf("nothing available\n");
>>>                                 continue;
>>>
>>> I'm tempted to say,that this should go to stderr, as well. Any opinions?
>>
>> I see it more as an informing note, since the device continues looping
>> for data, but it could be considered an error as well.
>
> I thought about the case when stdout gets piped to a data file while stderr
> goes into a logfile (or an application reads one data stream while checking
> the error stream).
>
>>
>>>> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
>>>>               ret = write_sysfs_string("trigger/current_trigger",
>>>>                                        dev_dir_name, "NULL");
>>>>               if (ret < 0)
>>>> -                     printf("Failed to write to %s\n", dev_dir_name);
>>>> +                     fprintf(stderr, "Failed to write to %s\n",
>>>> +                             dev_dir_name);
>>>>
>>>>  error_close_buffer_access:
>>>>       if (close(fp) == -1)
>>>> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
>>>> index 703f4cb..843bc4c 100644
>>>> --- a/tools/iio/iio_event_monitor.c
>>>> +++ b/tools/iio/iio_event_monitor.c
>>>
>>> At line 217:
>>>         if (!event_is_known(event)) {
>>>                 printf("Unknown event: time: %lld, id: %llx\n",
>>>                        event->timestamp, event->id);
>>>
>>>                 return;
>>> Better have this on stderr, as well?
>>
>> This is more suitable for stderr, indeed.
>>
>>>> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
>>>>       fd = open(chrdev_name, 0);
>>>>       if (fd == -1) {
>>>>               ret = -errno;
>>>> -             fprintf(stdout, "Failed to open %s\n", chrdev_name);
>>>> +             fprintf(stderr, "Failed to open %s\n", chrdev_name);
>>>>               goto error_free_chrdev_name;
>>>>       }
>>>>
>>>>       ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
>>>>       if (ret == -1 || event_fd == -1) {
>>>>               ret = -errno;
>>>> -             fprintf(stdout, "Failed to retrieve event fd\n");
>>>> +             fprintf(stderr, "Failed to retrieve event fd\n");
>>>>               if (close(fd) == -1)
>>>>                       perror("Failed to close character device file");
>>>>
>>>
>>> A similar borderline case as above in line 301:
>>>                 ret = read(event_fd, &event, sizeof(event));
>>>                 if (ret == -1) {
>>>                         if (errno == EAGAIN) {
>>>                                 printf("nothing available\n");
>>>                                 continue;
>>>
>>>> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
>>>>               }
>>>>
>>>>               if (ret != sizeof(event)) {
>>>> -                     printf("Reading event failed!\n");
>>>> +                     fprintf(stderr, "Reading event failed!\n");
>>>>                       ret = -EIO;
>>>>                       break;
>>>>               }
>>>> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
>>>> index 8fb3214..46dfa3f 100644
>>>> --- a/tools/iio/iio_utils.c
>>>> +++ b/tools/iio/iio_utils.c
>>>> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>>>                       sysfsfp = fopen(filename, "r");
>>>>                       if (sysfsfp == NULL) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to open %s\n", filename);
>>>> +                             fprintf(stderr, "failed to open %s\n",
>>>> +                                     filename);
>>>>                               goto error_free_filename;
>>>>                       }
>>>>
>>>> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>>>                                    &padint, shift);
>>>>                       if (ret < 0) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to pass scan type description\n");
>>>> +                             fprintf(stderr,
>>>> +                                     "failed to pass scan type description\n");
>>>>                               goto error_close_sysfsfp;
>>>>                       } else if (ret != 5) {
>>>>                               ret = -EIO;
>>>> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>>>>                       *is_signed = (signchar == 's');
>>>>                       if (fclose(sysfsfp)) {
>>>>                               ret = -errno;
>>>> -                             printf("Failed to close %s\n", filename);
>>>> +                             fprintf(stderr, "Failed to close %s\n",
>>>> +                                     filename);
>>>>                               goto error_free_filename;
>>>>                       }
>>>>
>>>> @@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
>>>>                       sysfsfp = fopen(filename, "r");
>>>>                       if (sysfsfp == NULL) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to open %s\n", filename);
>>>> +                             fprintf(stderr, "failed to open %s\n",
>>>> +                                     filename);
>>>>                               free(filename);
>>>>                               goto error_cleanup_array;
>>>>                       }
>>>> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const char *type)
>>>>                       ret = sscanf(ent->d_name + strlen(type), "%d", &number);
>>>>                       if (ret < 0) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to read element number\n");
>>>> +                             fprintf(stderr,
>>>> +                                     "failed to read element number\n");
>>>>                               goto error_close_dir;
>>>>                       } else if (ret != 1) {
>>>>                               ret = -EIO;
>>>> -                             printf("failed to match element number\n");
>>>> +                             fprintf(stderr,
>>>> +                                     "failed to match element number\n");
>>>>                               goto error_close_dir;
>>>>                       }
>>>>
>>>> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>>>>       sysfsfp = fopen(temp, "w");
>>>>       if (sysfsfp == NULL) {
>>>>               ret = -errno;
>>>> -             printf("failed to open %s\n", temp);
>>>> +             fprintf(stderr, "failed to open %s\n", temp);
>>>>               goto error_free;
>>>>       }
>>>>
>>>> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char *filename, const char *basedir, int val,
>>>>               sysfsfp = fopen(temp, "r");
>>>>               if (sysfsfp == NULL) {
>>>>                       ret = -errno;
>>>> -                     printf("failed to open %s\n", temp);
>>>> +                     fprintf(stderr, "failed to open %s\n", temp);
>>>>                       goto error_free;
>>>>               }
>>>>
>>>> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed\n");
>>>> +             fprintf(stderr, "Memory allocation failed\n");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>>>       sysfsfp = fopen(temp, "w");
>>>>       if (sysfsfp == NULL) {
>>>>               ret = -errno;
>>>> -             printf("Could not open %s\n", temp);
>>>> +             fprintf(stderr, "Could not open %s\n", temp);
>>>>               goto error_free;
>>>>       }
>>>>
>>>> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char *filename, const char *basedir,
>>>>               sysfsfp = fopen(temp, "r");
>>>>               if (sysfsfp == NULL) {
>>>>                       ret = -errno;
>>>> -                     printf("Could not open file to verify\n");
>>>> +                     fprintf(stderr, "Could not open file to verify\n");
>>>>                       goto error_free;
>>>>               }
>>>>
>>>> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename, const char *basedir)
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed");
>>>> +             fprintf(stderr, "Memory allocation failed");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename, const char *basedir, float *val)
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed");
>>>> +             fprintf(stderr, "Memory allocation failed");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename, const char *basedir, char *str)
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed");
>>>> +             fprintf(stderr, "Memory allocation failed");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Jonathan Cameron]

On 12 July 2015 13:20:45 BST, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> Yes, I could have included all in a single patch, but I tried to
>> automatize this task and build a rather generic semantic patch in
>> coccinelle for the substitutions. Had I included all in one patch,
>the
>> changes with coccinelle wouldn't have been differentiated from the
>> other ones. If that is okay, I think I can merge them in one patch.
>
>If it seems important, then you can say in the commit message what was
>done
>by hand.
>
>julia
Agreed. This is interesting information so a brief note would be great.
>
>
>> >> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
>> >> ---
>> >> Changes in v2:
>> >>  - s/failiure/failure
>> >>
>> >>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
>> >>  tools/iio/iio_event_monitor.c |  6 +++---
>> >>  tools/iio/iio_utils.c         | 34
>++++++++++++++++++++--------------
>> >>  3 files changed, 33 insertions(+), 24 deletions(-)
>> >>
>> >> diff --git a/tools/iio/generic_buffer.c
>b/tools/iio/generic_buffer.c
>> >> index 0e73723..2f4e12f 100644
>> >> --- a/tools/iio/generic_buffer.c
>> >> +++ b/tools/iio/generic_buffer.c
>> >> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
>> >>       }
>> >>
>> >>       if (device_name == NULL) {
>> >> -             printf("Device name not set\n");
>> >> +             fprintf(stderr, "Device name not set\n");
>> >>               print_usage();
>> >>               return -1;
>> >>       }
>> >> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
>> >>       /* Find the device requested */
>> >>       dev_num = find_type_by_name(device_name, "iio:device");
>> >>       if (dev_num < 0) {
>> >> -             printf("Failed to find the %s\n", device_name);
>> >> +             fprintf(stderr, "Failed to find the %s\n",
>device_name);
>> >>               return dev_num;
>> >>       }
>> >>
>> >> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
>> >>               /* Verify the trigger exists */
>> >>               trig_num = find_type_by_name(trigger_name,
>"trigger");
>> >>               if (trig_num < 0) {
>> >> -                     printf("Failed to find the trigger %s\n",
>trigger_name);
>> >> +                     fprintf(stderr, "Failed to find the trigger
>%s\n",
>> >> +                             trigger_name);
>> >>                       ret = trig_num;
>> >>                       goto error_free_triggername;
>> >>               }
>> >> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
>> >>        */
>> >>       ret = build_channel_array(dev_dir_name, &channels,
>&num_channels);
>> >>       if (ret) {
>> >> -             printf("Problem reading scan element
>information\n");
>> >> +             fprintf(stderr, "Problem reading scan element
>information\n");
>> >>               printf("diag %s\n", dev_dir_name);
>> >
>> > My preference would even be to print it all in just one fprintf.
>>
>> I thought so, also, but the string would go beyond 80 characters and
>> would have to be split which is ugly and brings a warning on it.
>>
>> >>               goto error_free_triggername;
>> >>       }
>> >> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
>> >>                                                   dev_dir_name,
>> >>                                                   trigger_name);
>> >>               if (ret < 0) {
>> >> -                     printf("Failed to write current_trigger
>file\n");
>> >> +                     fprintf(stderr,
>> >> +                             "Failed to write current_trigger
>file\n");
>> >>                       goto error_free_buf_dir_name;
>> >>               }
>> >>       }
>> >> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
>> >>       fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>> >>       if (fp == -1) { /* TODO: If it isn't there make the node */
>> >>               ret = -errno;
>> >> -             printf("Failed to open %s\n", buffer_access);
>> >> +             fprintf(stderr, "Failed to open %s\n",
>buffer_access);
>> >>               goto error_free_buffer_access;
>> >>       }
>> >>
>> >
>> > At line 410 we have a block:
>> >                 read_size = read(fp, data, toread * scan_size);
>> >                 if (read_size < 0) {
>> >                         if (errno == EAGAIN) {
>> >                                 printf("nothing available\n");
>> >                                 continue;
>> >
>> > I'm tempted to say,that this should go to stderr, as well. Any
>opinions?
>>
>> I see it more as an informing note, since the device continues
>looping
>> for data, but it could be considered an error as well.
>>
>> >> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
>> >>               ret = write_sysfs_string("trigger/current_trigger",
>> >>                                        dev_dir_name, "NULL");
>> >>               if (ret < 0)
>> >> -                     printf("Failed to write to %s\n",
>dev_dir_name);
>> >> +                     fprintf(stderr, "Failed to write to %s\n",
>> >> +                             dev_dir_name);
>> >>
>> >>  error_close_buffer_access:
>> >>       if (close(fp) == -1)
>> >> diff --git a/tools/iio/iio_event_monitor.c
>b/tools/iio/iio_event_monitor.c
>> >> index 703f4cb..843bc4c 100644
>> >> --- a/tools/iio/iio_event_monitor.c
>> >> +++ b/tools/iio/iio_event_monitor.c
>> >
>> > At line 217:
>> >         if (!event_is_known(event)) {
>> >                 printf("Unknown event: time: %lld, id: %llx\n",
>> >                        event->timestamp, event->id);
>> >
>> >                 return;
>> > Better have this on stderr, as well?
>>
>> This is more suitable for stderr, indeed.
>>
>> >> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
>> >>       fd = open(chrdev_name, 0);
>> >>       if (fd == -1) {
>> >>               ret = -errno;
>> >> -             fprintf(stdout, "Failed to open %s\n", chrdev_name);
>> >> +             fprintf(stderr, "Failed to open %s\n", chrdev_name);
>> >>               goto error_free_chrdev_name;
>> >>       }
>> >>
>> >>       ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
>> >>       if (ret == -1 || event_fd == -1) {
>> >>               ret = -errno;
>> >> -             fprintf(stdout, "Failed to retrieve event fd\n");
>> >> +             fprintf(stderr, "Failed to retrieve event fd\n");
>> >>               if (close(fd) == -1)
>> >>                       perror("Failed to close character device
>file");
>> >>
>> >
>> > A similar borderline case as above in line 301:
>> >                 ret = read(event_fd, &event, sizeof(event));
>> >                 if (ret == -1) {
>> >                         if (errno == EAGAIN) {
>> >                                 printf("nothing available\n");
>> >                                 continue;
>> >
>> >> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
>> >>               }
>> >>
>> >>               if (ret != sizeof(event)) {
>> >> -                     printf("Reading event failed!\n");
>> >> +                     fprintf(stderr, "Reading event failed!\n");
>> >>                       ret = -EIO;
>> >>                       break;
>> >>               }
>> >> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
>> >> index 8fb3214..46dfa3f 100644
>> >> --- a/tools/iio/iio_utils.c
>> >> +++ b/tools/iio/iio_utils.c
>> >> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed,
>unsigned *bytes, unsigned *bits_used,
>> >>                       sysfsfp = fopen(filename, "r");
>> >>                       if (sysfsfp == NULL) {
>> >>                               ret = -errno;
>> >> -                             printf("failed to open %s\n",
>filename);
>> >> +                             fprintf(stderr, "failed to open
>%s\n",
>> >> +                                     filename);
>> >>                               goto error_free_filename;
>> >>                       }
>> >>
>> >> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed,
>unsigned *bytes, unsigned *bits_used,
>> >>                                    &padint, shift);
>> >>                       if (ret < 0) {
>> >>                               ret = -errno;
>> >> -                             printf("failed to pass scan type
>description\n");
>> >> +                             fprintf(stderr,
>> >> +                                     "failed to pass scan type
>description\n");
>> >>                               goto error_close_sysfsfp;
>> >>                       } else if (ret != 5) {
>> >>                               ret = -EIO;
>> >> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed,
>unsigned *bytes, unsigned *bits_used,
>> >>                       *is_signed = (signchar == 's');
>> >>                       if (fclose(sysfsfp)) {
>> >>                               ret = -errno;
>> >> -                             printf("Failed to close %s\n",
>filename);
>> >> +                             fprintf(stderr, "Failed to close
>%s\n",
>> >> +                                     filename);
>> >>                               goto error_free_filename;
>> >>                       }
>> >>
>> >> @@ -454,7 +457,8 @@ int build_channel_array(const char
>*device_dir,
>> >>                       sysfsfp = fopen(filename, "r");
>> >>                       if (sysfsfp == NULL) {
>> >>                               ret = -errno;
>> >> -                             printf("failed to open %s\n",
>filename);
>> >> +                             fprintf(stderr, "failed to open
>%s\n",
>> >> +                                     filename);
>> >>                               free(filename);
>> >>                               goto error_cleanup_array;
>> >>                       }
>> >> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name,
>const char *type)
>> >>                       ret = sscanf(ent->d_name + strlen(type),
>"%d", &number);
>> >>                       if (ret < 0) {
>> >>                               ret = -errno;
>> >> -                             printf("failed to read element
>number\n");
>> >> +                             fprintf(stderr,
>> >> +                                     "failed to read element
>number\n");
>> >>                               goto error_close_dir;
>> >>                       } else if (ret != 1) {
>> >>                               ret = -EIO;
>> >> -                             printf("failed to match element
>number\n");
>> >> +                             fprintf(stderr,
>> >> +                                     "failed to match element
>number\n");
>> >>                               goto error_close_dir;
>> >>                       }
>> >>
>> >> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char
>*filename, const char *basedir, int val,
>> >>       sysfsfp = fopen(temp, "w");
>> >>       if (sysfsfp == NULL) {
>> >>               ret = -errno;
>> >> -             printf("failed to open %s\n", temp);
>> >> +             fprintf(stderr, "failed to open %s\n", temp);
>> >>               goto error_free;
>> >>       }
>> >>
>> >> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char
>*filename, const char *basedir, int val,
>> >>               sysfsfp = fopen(temp, "r");
>> >>               if (sysfsfp == NULL) {
>> >>                       ret = -errno;
>> >> -                     printf("failed to open %s\n", temp);
>> >> +                     fprintf(stderr, "failed to open %s\n",
>temp);
>> >>                       goto error_free;
>> >>               }
>> >>
>> >> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char
>*filename, const char *basedir,
>> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>> >>
>> >>       if (temp == NULL) {
>> >> -             printf("Memory allocation failed\n");
>> >> +             fprintf(stderr, "Memory allocation failed\n");
>> >>               return -ENOMEM;
>> >>       }
>> >>
>> >> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char
>*filename, const char *basedir,
>> >>       sysfsfp = fopen(temp, "w");
>> >>       if (sysfsfp == NULL) {
>> >>               ret = -errno;
>> >> -             printf("Could not open %s\n", temp);
>> >> +             fprintf(stderr, "Could not open %s\n", temp);
>> >>               goto error_free;
>> >>       }
>> >>
>> >> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char
>*filename, const char *basedir,
>> >>               sysfsfp = fopen(temp, "r");
>> >>               if (sysfsfp == NULL) {
>> >>                       ret = -errno;
>> >> -                     printf("Could not open file to verify\n");
>> >> +                     fprintf(stderr, "Could not open file to
>verify\n");
>> >>                       goto error_free;
>> >>               }
>> >>
>> >> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename,
>const char *basedir)
>> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>> >>
>> >>       if (temp == NULL) {
>> >> -             printf("Memory allocation failed");
>> >> +             fprintf(stderr, "Memory allocation failed");
>> >>               return -ENOMEM;
>> >>       }
>> >>
>> >> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename,
>const char *basedir, float *val)
>> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>> >>
>> >>       if (temp == NULL) {
>> >> -             printf("Memory allocation failed");
>> >> +             fprintf(stderr, "Memory allocation failed");
>> >>               return -ENOMEM;
>> >>       }
>> >>
>> >> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename,
>const char *basedir, char *str)
>> >>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>> >>
>> >>       if (temp == NULL) {
>> >> -             printf("Memory allocation failed");
>> >> +             fprintf(stderr, "Memory allocation failed");
>> >>               return -ENOMEM;
>> >>       }
>> >>
>> >>
>> >
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2 1/4] tools: iio: Move printf failure messages to stderr

[Jonathan Cameron]

On 12 July 2015 14:07:15 BST, Hartmut Knaack <knaack.h@gmx.de> wrote:
>Cristina Georgiana Opriceana schrieb am 12.07.2015 um 13:38:
>> On Sat, Jul 11, 2015 at 12:42 AM, Hartmut Knaack <knaack.h@gmx.de>
>wrote:
>>> Cristina Opriceana schrieb am 10.07.2015 um 12:56:
>>>> Replace printf error messages with fprintf(stderr, ...) in order
>>>> to ensure consistency and to make faults easier to identify.
>>>> This patch uses coccinelle to detect and apply the changes.
>>>>
>>>
>>> Hi Cristina,
>>> I just had a look at the series. You have all cases I regard
>necessary
>>> covered. There are however a few cases which probably qualify as
>error
>>> messages, too. Please see inline.
>>> However, for my personal taste, this could have been merged all in a
>>> single patch. Especially the third patch should have been included
>in
>>> this one (as during review, people certainly think that you missed
>the
>>> second line, just to find it fixed two patches later).
>> 
>> Hi,
>> 
>> Yes, I could have included all in a single patch, but I tried to
>> automatize this task and build a rather generic semantic patch in
>> coccinelle for the substitutions. Had I included all in one patch,
>the
>> changes with coccinelle wouldn't have been differentiated from the
>> other ones. If that is okay, I think I can merge them in one patch.
>> 
>
>Point taken for making it reproducible. I just like to raise awareness:
>The maintainers are very busy. They have a full time job, respond to
>the
>mailing list, review patches and do all the git magic. That keeps them
>pretty busy, so I would aim to make the maintainers life as easy as
>possible.
>Checking just one patch against some sources can be quicker/easier than
>checking multiple ones. Same goes for applying patches.

Always a balance needing to be struck here.  I always slightly prefer too many patches to too few.

Here however the breaking up doesn't really help with clarity.  I don't feel
 strongly about it though! (Particularly as I can rely on Hartmut's review here to
 cover what was missed and only quickly double check validity of what changed.,)


Jonathan
>Other than that, I just feel strong about the third patch standing
>separate,
>which I don't regard a good idea. Either your script should have been
>extended to catch such cases, or if it is not worth the effort, it
>should
>have been changed by hand. But it should have gone in one pass.
>
>>>> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>  - s/failiure/failure
>>>>
>>>>  tools/iio/generic_buffer.c    | 17 ++++++++++-------
>>>>  tools/iio/iio_event_monitor.c |  6 +++---
>>>>  tools/iio/iio_utils.c         | 34
>++++++++++++++++++++--------------
>>>>  3 files changed, 33 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/tools/iio/generic_buffer.c
>b/tools/iio/generic_buffer.c
>>>> index 0e73723..2f4e12f 100644
>>>> --- a/tools/iio/generic_buffer.c
>>>> +++ b/tools/iio/generic_buffer.c
>>>> @@ -271,7 +271,7 @@ int main(int argc, char **argv)
>>>>       }
>>>>
>>>>       if (device_name == NULL) {
>>>> -             printf("Device name not set\n");
>>>> +             fprintf(stderr, "Device name not set\n");
>>>>               print_usage();
>>>>               return -1;
>>>>       }
>>>> @@ -279,7 +279,7 @@ int main(int argc, char **argv)
>>>>       /* Find the device requested */
>>>>       dev_num = find_type_by_name(device_name, "iio:device");
>>>>       if (dev_num < 0) {
>>>> -             printf("Failed to find the %s\n", device_name);
>>>> +             fprintf(stderr, "Failed to find the %s\n",
>device_name);
>>>>               return dev_num;
>>>>       }
>>>>
>>>> @@ -307,7 +307,8 @@ int main(int argc, char **argv)
>>>>               /* Verify the trigger exists */
>>>>               trig_num = find_type_by_name(trigger_name,
>"trigger");
>>>>               if (trig_num < 0) {
>>>> -                     printf("Failed to find the trigger %s\n",
>trigger_name);
>>>> +                     fprintf(stderr, "Failed to find the trigger
>%s\n",
>>>> +                             trigger_name);
>>>>                       ret = trig_num;
>>>>                       goto error_free_triggername;
>>>>               }
>>>> @@ -323,7 +324,7 @@ int main(int argc, char **argv)
>>>>        */
>>>>       ret = build_channel_array(dev_dir_name, &channels,
>&num_channels);
>>>>       if (ret) {
>>>> -             printf("Problem reading scan element information\n");
>>>> +             fprintf(stderr, "Problem reading scan element
>information\n");
>>>>               printf("diag %s\n", dev_dir_name);
>>>
>>> My preference would even be to print it all in just one fprintf.
>> 
>> I thought so, also, but the string would go beyond 80 characters and
>> would have to be split which is ugly and brings a warning on it.
>> 
>
>I just gave it a try compile testing:
>		fprintf(stderr, "Problem reading scan element information\n"
>			"diag %s\n", dev_dir_name);
>
>Compiled just fine, and the same format is used in your second patch as
>well.
>
>>>>               goto error_free_triggername;
>>>>       }
>>>> @@ -350,7 +351,8 @@ int main(int argc, char **argv)
>>>>                                                   dev_dir_name,
>>>>                                                   trigger_name);
>>>>               if (ret < 0) {
>>>> -                     printf("Failed to write current_trigger
>file\n");
>>>> +                     fprintf(stderr,
>>>> +                             "Failed to write current_trigger
>file\n");
>>>>                       goto error_free_buf_dir_name;
>>>>               }
>>>>       }
>>>> @@ -382,7 +384,7 @@ int main(int argc, char **argv)
>>>>       fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
>>>>       if (fp == -1) { /* TODO: If it isn't there make the node */
>>>>               ret = -errno;
>>>> -             printf("Failed to open %s\n", buffer_access);
>>>> +             fprintf(stderr, "Failed to open %s\n",
>buffer_access);
>>>>               goto error_free_buffer_access;
>>>>       }
>>>>
>>>
>>> At line 410 we have a block:
>>>                 read_size = read(fp, data, toread * scan_size);
>>>                 if (read_size < 0) {
>>>                         if (errno == EAGAIN) {
>>>                                 printf("nothing available\n");
>>>                                 continue;
>>>
>>> I'm tempted to say,that this should go to stderr, as well. Any
>opinions?
>> 
>> I see it more as an informing note, since the device continues
>looping
>> for data, but it could be considered an error as well.
>
>I thought about the case when stdout gets piped to a data file while
>stderr
>goes into a logfile (or an application reads one data stream while
>checking
>the error stream).
>
>> 
>>>> @@ -431,7 +433,8 @@ int main(int argc, char **argv)
>>>>               ret = write_sysfs_string("trigger/current_trigger",
>>>>                                        dev_dir_name, "NULL");
>>>>               if (ret < 0)
>>>> -                     printf("Failed to write to %s\n",
>dev_dir_name);
>>>> +                     fprintf(stderr, "Failed to write to %s\n",
>>>> +                             dev_dir_name);
>>>>
>>>>  error_close_buffer_access:
>>>>       if (close(fp) == -1)
>>>> diff --git a/tools/iio/iio_event_monitor.c
>b/tools/iio/iio_event_monitor.c
>>>> index 703f4cb..843bc4c 100644
>>>> --- a/tools/iio/iio_event_monitor.c
>>>> +++ b/tools/iio/iio_event_monitor.c
>>>
>>> At line 217:
>>>         if (!event_is_known(event)) {
>>>                 printf("Unknown event: time: %lld, id: %llx\n",
>>>                        event->timestamp, event->id);
>>>
>>>                 return;
>>> Better have this on stderr, as well?
>> 
>> This is more suitable for stderr, indeed.
>> 
>>>> @@ -278,14 +278,14 @@ int main(int argc, char **argv)
>>>>       fd = open(chrdev_name, 0);
>>>>       if (fd == -1) {
>>>>               ret = -errno;
>>>> -             fprintf(stdout, "Failed to open %s\n", chrdev_name);
>>>> +             fprintf(stderr, "Failed to open %s\n", chrdev_name);
>>>>               goto error_free_chrdev_name;
>>>>       }
>>>>
>>>>       ret = ioctl(fd, IIO_GET_EVENT_FD_IOCTL, &event_fd);
>>>>       if (ret == -1 || event_fd == -1) {
>>>>               ret = -errno;
>>>> -             fprintf(stdout, "Failed to retrieve event fd\n");
>>>> +             fprintf(stderr, "Failed to retrieve event fd\n");
>>>>               if (close(fd) == -1)
>>>>                       perror("Failed to close character device
>file");
>>>>
>>>
>>> A similar borderline case as above in line 301:
>>>                 ret = read(event_fd, &event, sizeof(event));
>>>                 if (ret == -1) {
>>>                         if (errno == EAGAIN) {
>>>                                 printf("nothing available\n");
>>>                                 continue;
>>>
>>>> @@ -311,7 +311,7 @@ int main(int argc, char **argv)
>>>>               }
>>>>
>>>>               if (ret != sizeof(event)) {
>>>> -                     printf("Reading event failed!\n");
>>>> +                     fprintf(stderr, "Reading event failed!\n");
>>>>                       ret = -EIO;
>>>>                       break;
>>>>               }
>>>> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
>>>> index 8fb3214..46dfa3f 100644
>>>> --- a/tools/iio/iio_utils.c
>>>> +++ b/tools/iio/iio_utils.c
>>>> @@ -140,7 +140,8 @@ int iioutils_get_type(unsigned *is_signed,
>unsigned *bytes, unsigned *bits_used,
>>>>                       sysfsfp = fopen(filename, "r");
>>>>                       if (sysfsfp == NULL) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to open %s\n",
>filename);
>>>> +                             fprintf(stderr, "failed to open
>%s\n",
>>>> +                                     filename);
>>>>                               goto error_free_filename;
>>>>                       }
>>>>
>>>> @@ -152,7 +153,8 @@ int iioutils_get_type(unsigned *is_signed,
>unsigned *bytes, unsigned *bits_used,
>>>>                                    &padint, shift);
>>>>                       if (ret < 0) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to pass scan type
>description\n");
>>>> +                             fprintf(stderr,
>>>> +                                     "failed to pass scan type
>description\n");
>>>>                               goto error_close_sysfsfp;
>>>>                       } else if (ret != 5) {
>>>>                               ret = -EIO;
>>>> @@ -170,7 +172,8 @@ int iioutils_get_type(unsigned *is_signed,
>unsigned *bytes, unsigned *bits_used,
>>>>                       *is_signed = (signchar == 's');
>>>>                       if (fclose(sysfsfp)) {
>>>>                               ret = -errno;
>>>> -                             printf("Failed to close %s\n",
>filename);
>>>> +                             fprintf(stderr, "Failed to close
>%s\n",
>>>> +                                     filename);
>>>>                               goto error_free_filename;
>>>>                       }
>>>>
>>>> @@ -454,7 +457,8 @@ int build_channel_array(const char *device_dir,
>>>>                       sysfsfp = fopen(filename, "r");
>>>>                       if (sysfsfp == NULL) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to open %s\n",
>filename);
>>>> +                             fprintf(stderr, "failed to open
>%s\n",
>>>> +                                     filename);
>>>>                               free(filename);
>>>>                               goto error_cleanup_array;
>>>>                       }
>>>> @@ -581,11 +585,13 @@ int find_type_by_name(const char *name, const
>char *type)
>>>>                       ret = sscanf(ent->d_name + strlen(type),
>"%d", &number);
>>>>                       if (ret < 0) {
>>>>                               ret = -errno;
>>>> -                             printf("failed to read element
>number\n");
>>>> +                             fprintf(stderr,
>>>> +                                     "failed to read element
>number\n");
>>>>                               goto error_close_dir;
>>>>                       } else if (ret != 1) {
>>>>                               ret = -EIO;
>>>> -                             printf("failed to match element
>number\n");
>>>> +                             fprintf(stderr,
>>>> +                                     "failed to match element
>number\n");
>>>>                               goto error_close_dir;
>>>>                       }
>>>>
>>>> @@ -664,7 +670,7 @@ static int _write_sysfs_int(const char
>*filename, const char *basedir, int val,
>>>>       sysfsfp = fopen(temp, "w");
>>>>       if (sysfsfp == NULL) {
>>>>               ret = -errno;
>>>> -             printf("failed to open %s\n", temp);
>>>> +             fprintf(stderr, "failed to open %s\n", temp);
>>>>               goto error_free;
>>>>       }
>>>>
>>>> @@ -685,7 +691,7 @@ static int _write_sysfs_int(const char
>*filename, const char *basedir, int val,
>>>>               sysfsfp = fopen(temp, "r");
>>>>               if (sysfsfp == NULL) {
>>>>                       ret = -errno;
>>>> -                     printf("failed to open %s\n", temp);
>>>> +                     fprintf(stderr, "failed to open %s\n", temp);
>>>>                       goto error_free;
>>>>               }
>>>>
>>>> @@ -750,7 +756,7 @@ static int _write_sysfs_string(const char
>*filename, const char *basedir,
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed\n");
>>>> +             fprintf(stderr, "Memory allocation failed\n");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>> @@ -761,7 +767,7 @@ static int _write_sysfs_string(const char
>*filename, const char *basedir,
>>>>       sysfsfp = fopen(temp, "w");
>>>>       if (sysfsfp == NULL) {
>>>>               ret = -errno;
>>>> -             printf("Could not open %s\n", temp);
>>>> +             fprintf(stderr, "Could not open %s\n", temp);
>>>>               goto error_free;
>>>>       }
>>>>
>>>> @@ -782,7 +788,7 @@ static int _write_sysfs_string(const char
>*filename, const char *basedir,
>>>>               sysfsfp = fopen(temp, "r");
>>>>               if (sysfsfp == NULL) {
>>>>                       ret = -errno;
>>>> -                     printf("Could not open file to verify\n");
>>>> +                     fprintf(stderr, "Could not open file to
>verify\n");
>>>>                       goto error_free;
>>>>               }
>>>>
>>>> @@ -856,7 +862,7 @@ int read_sysfs_posint(const char *filename,
>const char *basedir)
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed");
>>>> +             fprintf(stderr, "Memory allocation failed");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>> @@ -903,7 +909,7 @@ int read_sysfs_float(const char *filename,
>const char *basedir, float *val)
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed");
>>>> +             fprintf(stderr, "Memory allocation failed");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>> @@ -950,7 +956,7 @@ int read_sysfs_string(const char *filename,
>const char *basedir, char *str)
>>>>       char *temp = malloc(strlen(basedir) + strlen(filename) + 2);
>>>>
>>>>       if (temp == NULL) {
>>>> -             printf("Memory allocation failed");
>>>> +             fprintf(stderr, "Memory allocation failed");
>>>>               return -ENOMEM;
>>>>       }
>>>>
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.