[PATCH 0/2] new tool 'i2ctransfer'

[PATCH 0/2] new tool 'i2ctransfer'

[Wolfram Sang]

So, here is my updated, better tested, much improved version of i2ctransfer:

* rewrote parser to match agreed syntax (even improved it futher by reusing
  last address if not specified again)

* fixed 0-byte length transfers

* way more error checking

* better error reporting

And further details and cleanups. I still believe we should let Linux clean up
the buffers, but I suspect Jean won't like it, so I added a patch to just do
that.

Please review, test, apply...

Thanks,

   Wolfram


Wolfram Sang (2):
  i2c-tools: add new tool 'i2ctransfer'
  i2c-tools: i2ctransfer: clean up allocated resources

 tools/Module.mk     |   8 +-
 tools/i2ctransfer.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 tools/i2ctransfer.c

-- 
2.1.4

[PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

This tool allows to construct and concat multiple I2C messages into one
single transfer. Its aim is to test I2C master controllers, and so there
is no SMBus fallback.

I've been missing such a tool a number of times now, so I finally got
around to writing it myself. As with all I2C tools, it can be dangerous,
but it can also be very useful when developing. I am not sure if distros
should supply it, I'll leave that to Jean's experience. For embedded
build systems, I think this should be selectable.

Tested with various Renesas I2C IP cores as well as Tegra and AT91.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 tools/Module.mk     |   8 +-
 tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 327 insertions(+), 1 deletion(-)
 create mode 100644 tools/i2ctransfer.c

diff --git a/tools/Module.mk b/tools/Module.mk
index 641ac81..7192361 100644
--- a/tools/Module.mk
+++ b/tools/Module.mk
@@ -18,7 +18,7 @@ else
 TOOLS_LDFLAGS	+= -Llib -li2c
 endif
 
-TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget
+TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
 
 #
 # Programs
@@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
 $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
 	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
 
+$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
+	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
+
 #
 # Objects
 #
@@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
 $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
 	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
 
+$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
+	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
+
 $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
 	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
 
diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
new file mode 100644
index 0000000..27f4d7a
--- /dev/null
+++ b/tools/i2ctransfer.c
@@ -0,0 +1,320 @@
+/*
+    i2ctransfer.c - A user-space program to send concatenated i2c messages
+    Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
+    Copyright (C) 2015 Renesas Electronics Corporation
+
+    Based on i2cget.c:
+    Copyright (C) 2005-2012  Jean Delvare <jdelvare@suse.de>
+
+    which is based on i2cset.c:
+    Copyright (C) 2001-2003  Frodo Looijaard <frodol@dds.nl>, and
+                             Mark D. Studebaker <mdsxyz123@yahoo.com>
+    Copyright (C) 2004-2005  Jean Delvare
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+*/
+
+#include <sys/ioctl.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <linux/i2c.h>
+#include <linux/i2c-dev.h>
+#include "i2cbusses.h"
+#include "util.h"
+#include "../version.h"
+
+enum parse_state {
+	PARSE_GET_DESC,
+	PARSE_GET_DATA
+};
+
+#define PRINT_STDERR	(1 << 0)
+#define PRINT_READ_BUF	(1 << 1)
+#define PRINT_WRITE_BUF	(1 << 2)
+#define PRINT_HEADER	(1 << 3)
+
+static void help(void)
+{
+	fprintf(stderr,
+		"Usage: i2ctransfer [-f] [-y] [-v] [-V] I2CBUS DESC [DATA] [DESC [DATA]]...\n"
+		"  I2CBUS is an integer or an I2C bus name\n"
+		"  DESC describes the transfer in the form: {r|w}LENGTH[@address]\n"
+		"    1) read/write-flag 2) LENGTH (range 0-65535) 3) I2C address (use last one if omitted)\n"
+		"  DATA are LENGTH bytes for a write message. They can be shortened by a suffix:\n"
+		"    = (keep value constant until LENGTH)\n"
+		"    + (increase value by 1 until LENGTH)\n"
+		"    - (decrease value by 1 until LENGTH)\n"
+		"\nExample (bus 0, read 8 byte at offset 0x64 from eeprom at 0x50):\n"
+		"  # i2ctransfer 0 w1@0x50 0x64 r8\n"
+		"Example (same eeprom, at offset 0x42 write 0xff 0xfe .. 0x00 ):\n"
+		"  # i2ctransfer 0 w257@0x50 0x42 0xff-\n"
+		);
+}
+
+static int check_funcs(int file)
+{
+	unsigned long funcs;
+
+	/* check adapter functionality */
+	if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
+		fprintf(stderr, "Error: Could not get the adapter "
+			"functionality matrix: %s\n", strerror(errno));
+		return -1;
+	}
+
+	if (!(funcs & I2C_FUNC_I2C)) {
+		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
+		return -1;
+	}
+
+	return 0;
+}
+
+static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
+{
+	__u32 i, j;
+	FILE *output = flags & PRINT_STDERR ? stderr : stdout;
+
+	for (i = 0; i < nmsgs; i++) {
+		int read = !!(msgs[i].flags & I2C_M_RD);
+		int newline = !!(flags & PRINT_HEADER);
+
+		if (flags & PRINT_HEADER)
+			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
+				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
+		if (msgs[i].len &&
+		   (read = !!(flags & PRINT_READ_BUF) ||
+		   !read = !!(flags & PRINT_WRITE_BUF))) {
+			if (flags & PRINT_HEADER)
+				fprintf(output, ", buf ");
+			for (j = 0; j < msgs[i].len; j++)
+				fprintf(output, "0x%02x ", msgs[i].buf[j]);
+			newline = 1;
+		}
+		if (newline)
+			fprintf(output, "\n");
+	}
+}
+
+static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
+{
+	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
+	fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
+	print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
+
+	fprintf(stderr, "Continue? [y/N] ");
+	fflush(stderr);
+	if (!user_ack(0)) {
+		fprintf(stderr, "Aborting on user request.\n");
+		return 0;
+	}
+
+	return 1;
+}
+
+int main(int argc, char *argv[])
+{
+	char filename[20];
+	char *end;
+	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
+	int force = 0, yes = 0, version = 0, verbose = 0;
+	unsigned buf_idx = 0;
+	unsigned long len, raw_data;
+	__u8 data;
+	__u8 *buf;
+	__u16 flags;
+	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
+	struct i2c_rdwr_ioctl_data rdwr;
+	enum parse_state state = PARSE_GET_DESC;
+
+	/* handle (optional) arg_idx first */
+	while (arg_idx < argc && argv[arg_idx][0] = '-') {
+		switch (argv[arg_idx][1]) {
+		case 'V': version = 1; break;
+		case 'v': verbose = 1; break;
+		case 'f': force = 1; break;
+		case 'y': yes = 1; break;
+		default:
+			fprintf(stderr, "Error: Unsupported option "
+				"\"%s\"!\n", argv[arg_idx]);
+			help();
+			exit(1);
+		}
+		arg_idx++;
+	}
+
+	if (version) {
+		fprintf(stderr, "i2ctransfer version %s\n", VERSION);
+		exit(0);
+	}
+
+	if (arg_idx = argc) {
+		help();
+		exit(0);
+	}
+
+	i2cbus = lookup_i2c_bus(argv[arg_idx++]);
+	if (i2cbus < 0)
+		exit(1);
+
+	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
+	if (file < 0 || check_funcs(file))
+		exit(1);
+
+	while (arg_idx < argc) {
+		char *arg_ptr = argv[arg_idx];
+
+		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
+			fprintf(stderr, "Error: Too many messages (max: %d)\n",
+				I2C_RDRW_IOCTL_MAX_MSGS);
+			exit(1);
+		}
+
+		switch (state) {
+		case PARSE_GET_DESC:
+			flags = 0;
+
+			switch (*arg_ptr++) {
+			case 'r': flags |= I2C_M_RD; break;
+			case 'w': break;
+			default:
+				fprintf(stderr, "Error: Invalid direction\n");
+				goto err_out;
+			}
+
+			len = strtoul(arg_ptr, &end, 0);
+			if (len > 65535) {
+				fprintf(stderr, "Error: Length invalid\n");
+				goto err_out;
+			}
+
+			arg_ptr = end;
+			if (*arg_ptr) {
+				if (*arg_ptr++ != '@') {
+					fprintf(stderr, "Error: No '@' after length\n");
+					goto err_out;
+				}
+
+				/* We skip 10-bit support for now. If we want it, it
+				 * should be marked with a 't' flag before the address
+				 * here.
+				 */
+
+				address = parse_i2c_address(arg_ptr);
+				if (address < 0)
+					goto err_out;
+
+				if (!force && set_slave_addr(file, address, 0))
+					goto err_out;
+
+			} else {
+				/* Reuse last address if possible */
+				if (address < 0) {
+					fprintf(stderr, "Error: No address given\n");
+					goto err_out;
+				}
+			}
+
+			msgs[nmsgs].addr = address;
+			msgs[nmsgs].flags = flags;
+			msgs[nmsgs].len = len;
+
+			if (len) {
+				buf = malloc(len);
+				if (!buf) {
+					fprintf(stderr, "Error: No memory for buffer\n");
+					goto err_out;
+				}
+				memset(buf, 0, len);
+				msgs[nmsgs].buf = buf;
+			}
+
+			if (flags & I2C_M_RD || len = 0) {
+				nmsgs++;
+			} else {
+				buf_idx = 0;
+				state = PARSE_GET_DATA;
+			}
+
+			break;
+
+		case PARSE_GET_DATA:
+			raw_data = strtoul(arg_ptr, &end, 0);
+			if (raw_data > 255) {
+				fprintf(stderr, "Error: Data byte invalid\n");
+				goto err_out;
+			}
+			data = raw_data;
+			len = msgs[nmsgs].len;
+
+			while (buf_idx < len) {
+				msgs[nmsgs].buf[buf_idx++] = data;
+
+				if (!*end)
+					break;
+
+				switch (*end) {
+				case '+': data++; break;
+				case '-': data--; break;
+				case '=': break;
+				default:
+					fprintf(stderr, "Error: Invalid data byte suffix\n");
+					goto err_out;
+				}
+			}
+
+			if (buf_idx = len) {
+				nmsgs++;
+				state = PARSE_GET_DESC;
+			}
+
+			break;
+
+		default:
+			fprintf(stderr, "Error: Unnkown state in state machine!\n");
+			goto err_out;
+		}
+
+		arg_idx++;
+	}
+
+	if (state != PARSE_GET_DESC || nmsgs = 0) {
+		fprintf(stderr, "Error: Incomplete message\n");
+		exit(1);
+	}
+
+	if (!yes && !confirm(filename, msgs, nmsgs))
+		exit(0);
+
+	rdwr.msgs = msgs;
+	rdwr.nmsgs = nmsgs;
+	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
+	if (nmsgs_sent < 0) {
+		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
+		exit(errno);
+	} else if (nmsgs_sent < nmsgs) {
+		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
+	}
+
+	close(file);
+
+	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
+
+	/* let Linux free malloced memory on termination */
+	exit(0);
+
+err_out:
+	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
+	exit(1);
+}
-- 
2.1.4

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Jean Delvare]

Hi Wolfram,

On Fri, 19 Jun 2015 12:40:31 +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> This tool allows to construct and concat multiple I2C messages into one
> single transfer. Its aim is to test I2C master controllers, and so there
> is no SMBus fallback.

Which is fine. That wouldn't make much sense anyway as not all I2C
transactions fit into the SMBus set. For SMBus transactions we already
have i2cdump, i2cget and i2cset.

> I've been missing such a tool a number of times now, so I finally got
> around to writing it myself. As with all I2C tools, it can be dangerous,
> but it can also be very useful when developing. I am not sure if distros
> should supply it, I'll leave that to Jean's experience. For embedded
> build systems, I think this should be selectable.

I think it can be included together with the other tools. It's just as
dangerous a tool as the other ones, not more. The fact that it can't be
used on SMBus-only controllers even kind of makes it less dangerous.

> Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Not needed for i2c-tools contributions.

> ---
>  tools/Module.mk     |   8 +-
>  tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>
>  2 files changed, 327 insertions(+), 1 deletion(-)
>  create mode 100644 tools/i2ctransfer.c

Where is the manual page? We need one, it's mandatory for some
distributions. And "make install" currently fails because
tools/i2ctransfer.8 is missing.

While this is not kernel code, I would recommend that you run the
kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the
problems reported are relevant and fixing them would improve
readability.

Overall it looks really good. I made a lot of comments below but most
of them only express my preference and you are free to ignore them if
you disagree.

> diff --git a/tools/Module.mk b/tools/Module.mk
> index 641ac81..7192361 100644
> --- a/tools/Module.mk
> +++ b/tools/Module.mk
> @@ -18,7 +18,7 @@ else
>  TOOLS_LDFLAGS	+= -Llib -li2c
>  endif
>  
> -TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget
> +TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
>  
>  #
>  # Programs
> @@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
>  $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
>  	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
>  
> +$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
> +	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
> +
>  #
>  # Objects
>  #
> @@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
>  $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
>  	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
>  
> +$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
> +	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
> +
>  $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
>  	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
>  
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> new file mode 100644
> index 0000000..27f4d7a
> --- /dev/null
> +++ b/tools/i2ctransfer.c
> @@ -0,0 +1,320 @@
> +/*
> +    i2ctransfer.c - A user-space program to send concatenated i2c messages
> +    Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
> +    Copyright (C) 2015 Renesas Electronics Corporation
> +
> +    Based on i2cget.c:
> +    Copyright (C) 2005-2012  Jean Delvare <jdelvare@suse.de>
> +
> +    which is based on i2cset.c:
> +    Copyright (C) 2001-2003  Frodo Looijaard <frodol@dds.nl>, and
> +                             Mark D. Studebaker <mdsxyz123@yahoo.com>
> +    Copyright (C) 2004-2005  Jean Delvare

I think you can skip this. If anyone really cares, it's already
mentioned in i2cget.c.

> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 2 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +*/
> +
> +#include <sys/ioctl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-dev.h>
> +#include "i2cbusses.h"
> +#include "util.h"
> +#include "../version.h"
> +
> +enum parse_state {
> +	PARSE_GET_DESC,
> +	PARSE_GET_DATA

There should be a trailing comma, in case you ever need to add a state.

> +};
> +
> +#define PRINT_STDERR	(1 << 0)
> +#define PRINT_READ_BUF	(1 << 1)
> +#define PRINT_WRITE_BUF	(1 << 2)
> +#define PRINT_HEADER	(1 << 3)
> +
> +static void help(void)
> +{
> +	fprintf(stderr,
> +		"Usage: i2ctransfer [-f] [-y] [-v] [-V] I2CBUS DESC [DATA] [DESC [DATA]]...\n"
> +		"  I2CBUS is an integer or an I2C bus name\n"
> +		"  DESC describes the transfer in the form: {r|w}LENGTH[@address]\n"
> +		"    1) read/write-flag 2) LENGTH (range 0-65535) 3) I2C address (use last one if omitted)\n"

Note that the I2C_RDWR ioctl interface currently limits the per-message
length to 8192 bytes. I can't really think of a good reason for doing
so, other than the fact that buffers larger than this may be difficult
to allocate.

> +		"  DATA are LENGTH bytes for a write message. They can be shortened by a suffix:\n"
> +		"    = (keep value constant until LENGTH)\n"
> +		"    + (increase value by 1 until LENGTH)\n"
> +		"    - (decrease value by 1 until LENGTH)\n"
> +		"\nExample (bus 0, read 8 byte at offset 0x64 from eeprom at 0x50):\n"

The leading newline would rather go at end of previous line IMHO, or on
a line on its own to make it even clearer what the output will look
like.

> +		"  # i2ctransfer 0 w1@0x50 0x64 r8\n"
> +		"Example (same eeprom, at offset 0x42 write 0xff 0xfe .. 0x00 ):\n"

No space before the closing parenthesis. A shorter write would probably
serve better as an example, as it's unclear what will happen when the
register offset reaches 0x100.

> +		"  # i2ctransfer 0 w257@0x50 0x42 0xff-\n"
> +		);

The closing parenthesis can go at the end of the previous line, I think.

> +}
> +
> +static int check_funcs(int file)
> +{
> +	unsigned long funcs;
> +
> +	/* check adapter functionality */
> +	if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
> +		fprintf(stderr, "Error: Could not get the adapter "
> +			"functionality matrix: %s\n", strerror(errno));
> +		return -1;
> +	}
> +
> +	if (!(funcs & I2C_FUNC_I2C)) {
> +		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
> +{
> +	__u32 i, j;
> +	FILE *output = flags & PRINT_STDERR ? stderr : stdout;

Why don't you just pass the FILE * argument as the first parameter of
this function?

> +
> +	for (i = 0; i < nmsgs; i++) {
> +		int read = !!(msgs[i].flags & I2C_M_RD);
> +		int newline = !!(flags & PRINT_HEADER);

The double negation isn't needed.

> +
> +		if (flags & PRINT_HEADER)
> +			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
> +				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> +		if (msgs[i].len &&
> +		   (read = !!(flags & PRINT_READ_BUF) ||
> +		   !read = !!(flags & PRINT_WRITE_BUF))) {

Could be indented (aligned) better to improve readability. Also the
last two statements seems needlessly complex. I'd write:

		if (msgs[i].len &&
		    (read && (flags & PRINT_READ_BUF) ||
		     !read && (flags & PRINT_WRITE_BUF))) {

This approach would avoid all the double negations in this function.

> +			if (flags & PRINT_HEADER)
> +				fprintf(output, ", buf ");

You'd rather drop the trailing white space...

> +			for (j = 0; j < msgs[i].len; j++)
> +				fprintf(output, "0x%02x ", msgs[i].buf[j]);

... and move the white space at the beginning here. This avoids printing
a trailing space before the newline (which could cause an extra blank
line on display if you are unlucky with the terminal width.)

> +			newline = 1;
> +		}
> +		if (newline)
> +			fprintf(output, "\n");

In which case exactly would the newline not be printed? Read of length
0? I doubt it is worth caring about. I'd even say that printing a blank
line in this case is preferable, so that the output really corresponds
to what happened. Otherwise you can't tell the difference between read
of length 0 followed by read of length 1 and the other way around.

> +	}
> +}
> +
> +static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
> +{
> +	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
> +	fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
> +	print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
> +
> +	fprintf(stderr, "Continue? [y/N] ");
> +	fflush(stderr);
> +	if (!user_ack(0)) {
> +		fprintf(stderr, "Aborting on user request.\n");
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char filename[20];
> +	char *end;
> +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> +	int force = 0, yes = 0, version = 0, verbose = 0;
> +	unsigned buf_idx = 0;
> +	unsigned long len, raw_data;
> +	__u8 data;
> +	__u8 *buf;
> +	__u16 flags;
> +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> +	struct i2c_rdwr_ioctl_data rdwr;
> +	enum parse_state state = PARSE_GET_DESC;

That's a lot of local variables. I think some of them could be declared
inside the state machine. Did you try moving the state machine to a
separate function? Maybe if would improve readability.

> +
> +	/* handle (optional) arg_idx first */
> +	while (arg_idx < argc && argv[arg_idx][0] = '-') {
> +		switch (argv[arg_idx][1]) {
> +		case 'V': version = 1; break;
> +		case 'v': verbose = 1; break;
> +		case 'f': force = 1; break;
> +		case 'y': yes = 1; break;
> +		default:
> +			fprintf(stderr, "Error: Unsupported option "
> +				"\"%s\"!\n", argv[arg_idx]);
> +			help();
> +			exit(1);
> +		}
> +		arg_idx++;
> +	}
> +
> +	if (version) {
> +		fprintf(stderr, "i2ctransfer version %s\n", VERSION);
> +		exit(0);
> +	}
> +
> +	if (arg_idx = argc) {
> +		help();
> +		exit(0);

I think you want to return an error code here, not 0.

> +	}
> +
> +	i2cbus = lookup_i2c_bus(argv[arg_idx++]);
> +	if (i2cbus < 0)
> +		exit(1);
> +
> +	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
> +	if (file < 0 || check_funcs(file))
> +		exit(1);
> +
> +	while (arg_idx < argc) {
> +		char *arg_ptr = argv[arg_idx];
> +
> +		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
> +			fprintf(stderr, "Error: Too many messages (max: %d)\n",
> +				I2C_RDRW_IOCTL_MAX_MSGS);
> +			exit(1);
> +		}
> +
> +		switch (state) {
> +		case PARSE_GET_DESC:
> +			flags = 0;
> +
> +			switch (*arg_ptr++) {
> +			case 'r': flags |= I2C_M_RD; break;
> +			case 'w': break;
> +			default:
> +				fprintf(stderr, "Error: Invalid direction\n");
> +				goto err_out;
> +			}
> +
> +			len = strtoul(arg_ptr, &end, 0);
> +			if (len > 65535) {

0xffff would be easier to understand IMHO. Also you must check that
arg_ptr != end, i.e. you managed to parse anything. Otherwise a faulty
argument like w@0x50 will be parsed successfully (silently assuming len
= 0) and odds are that the following argument won't be parsed
successfully. In that case the error message will point the user to the
wrong argument.

> +				fprintf(stderr, "Error: Length invalid\n");
> +				goto err_out;
> +			}
> +
> +			arg_ptr = end;
> +			if (*arg_ptr) {
> +				if (*arg_ptr++ != '@') {
> +					fprintf(stderr, "Error: No '@' after length\n");

No @ after length is actually fine, that's what the "else" branch below
handles. The problem is no @ before address.

> +					goto err_out;
> +				}
> +
> +				/* We skip 10-bit support for now. If we want it, it
> +				 * should be marked with a 't' flag before the address
> +				 * here.
> +				 */
> +
> +				address = parse_i2c_address(arg_ptr);
> +				if (address < 0)
> +					goto err_out;
> +
> +				if (!force && set_slave_addr(file, address, 0))
> +					goto err_out;

It took me a while to realize that 1) this was not broken and 2) this
was actually useful. Please add a comment explaining that the only
purpose is to check that the slave address isn't busy. You don't
actually need to set the slave address at this point, the I2C_RDWR
ioctl doesn't make use of it.

> +
> +			} else {
> +				/* Reuse last address if possible */
> +				if (address < 0) {
> +					fprintf(stderr, "Error: No address given\n");
> +					goto err_out;
> +				}
> +			}
> +
> +			msgs[nmsgs].addr = address;
> +			msgs[nmsgs].flags = flags;
> +			msgs[nmsgs].len = len;
> +
> +			if (len) {
> +				buf = malloc(len);
> +				if (!buf) {
> +					fprintf(stderr, "Error: No memory for buffer\n");
> +					goto err_out;
> +				}
> +				memset(buf, 0, len);
> +				msgs[nmsgs].buf = buf;
> +			}

Else msgs[nmsgs].buf should be set to NULL. Otherwise you're passing a
random address value to the kernel.

> +
> +			if (flags & I2C_M_RD || len = 0) {
> +				nmsgs++;
> +			} else {
> +				buf_idx = 0;
> +				state = PARSE_GET_DATA;
> +			}
> +
> +			break;
> +
> +		case PARSE_GET_DATA:
> +			raw_data = strtoul(arg_ptr, &end, 0);
> +			if (raw_data > 255) {

0xff would be easier to understand IMHO. Here too you must check that
end != arg_ptr, otherwise '' or '+' will be parsed successfully
(assuming raw_data = 0.) 'x' would fail but with a confusing error
message ("Invalid data byte suffix".)

> +				fprintf(stderr, "Error: Data byte invalid\n");

"Invalid data byte" would sound better and be more consistent with
other error messages.

> +				goto err_out;
> +			}
> +			data = raw_data;
> +			len = msgs[nmsgs].len;
> +
> +			while (buf_idx < len) {
> +				msgs[nmsgs].buf[buf_idx++] = data;
> +
> +				if (!*end)
> +					break;
> +
> +				switch (*end) {
> +				case '+': data++; break;
> +				case '-': data--; break;
> +				case '=': break;
> +				default:
> +					fprintf(stderr, "Error: Invalid data byte suffix\n");
> +					goto err_out;
> +				}
> +			}
> +
> +			if (buf_idx = len) {
> +				nmsgs++;
> +				state = PARSE_GET_DESC;
> +			}
> +
> +			break;
> +
> +		default:
> +			fprintf(stderr, "Error: Unnkown state in state machine!\n");

Typo: Unknown

This can't actually happen, right? A comment saying so would be
appreciated. And "Internal error" instead of just "Error", so that the
user understands, if it ever happens.

> +			goto err_out;
> +		}
> +
> +		arg_idx++;
> +	}
> +
> +	if (state != PARSE_GET_DESC || nmsgs = 0) {

I'd be surprised if nmsgs = 0 can actually happen. You checked that at
least one parameter was left to parse on the command line before
entering the state machine. If you couldn't parse it then it must have
triggered some error earlier and you'll never reach this test.

> +		fprintf(stderr, "Error: Incomplete message\n");
> +		exit(1);
> +	}
> +
> +	if (!yes && !confirm(filename, msgs, nmsgs))
> +		exit(0);
> +
> +	rdwr.msgs = msgs;
> +	rdwr.nmsgs = nmsgs;
> +	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
> +	if (nmsgs_sent < 0) {
> +		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
> +		exit(errno);

I wouldn't recommend leaking errno back to the caller. Imagine errno is
EPERM (1), the user couldn't tell the difference with all other error
cases. IMHO there's no point in returning different error values unless
1) this is done consistently and 2) this is clearly documented in the
man page.

> +	} else if (nmsgs_sent < nmsgs) {
> +		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
> +	}
> +
> +	close(file);
> +
> +	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
> +
> +	/* let Linux free malloced memory on termination */
> +	exit(0);
> +
> +err_out:
> +	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> +	exit(1);
> +}


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 8203 bytes --]

Hi Jean,

I think we are approaching 1 year since I submitted the first version,
so I thought it might finally be time for getting this done :)

Thank you for your careful review. All comments which I did not respond
to are silently acknowledged, same for most comments to patch 2/2. I
have implemented most of the changes already, but still need to do the
testing and the man page. However, here are already the responses to
your comments.

> I think it can be included together with the other tools. It's just as
> dangerous a tool as the other ones, not more. The fact that it can't be
> used on SMBus-only controllers even kind of makes it less dangerous.

Yes, I happily agree.

> > Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Not needed for i2c-tools contributions.

Feel free to drop it. I'd still like to be clear when posting patches on
public mailing lists.

> >  tools/Module.mk     |   8 +-
> >  tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> >  2 files changed, 327 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/i2ctransfer.c
> 
> Where is the manual page? We need one, it's mandatory for some
> distributions. And "make install" currently fails because
> tools/i2ctransfer.8 is missing.

I wanted to check the review before doing one.

> While this is not kernel code, I would recommend that you run the
> kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the
> problems reported are relevant and fixing them would improve
> readability.

Some of them are to be consistent with the rest of the I2C tools, e.g.

ERROR: trailing statements should be on next line
#136: FILE: /home/ninja/Tools/i2c-tools/tools/i2ctransfer.c:136:
+		case 'V': version = 1; break;

And I am not too strict on the 80 char limit. Are you?

> > +	fprintf(stderr,
> > +		"Usage: i2ctransfer [-f] [-y] [-v] [-V] I2CBUS DESC [DATA] [DESC [DATA]]...\n"
> > +		"  I2CBUS is an integer or an I2C bus name\n"
> > +		"  DESC describes the transfer in the form: {r|w}LENGTH[@address]\n"
> > +		"    1) read/write-flag 2) LENGTH (range 0-65535) 3) I2C address (use last one if omitted)\n"
> 
> Note that the I2C_RDWR ioctl interface currently limits the per-message
> length to 8192 bytes. I can't really think of a good reason for doing
> so, other than the fact that buffers larger than this may be difficult
> to allocate.

I'd guess it was introduced to prevent congestion on the bus? I am
tempted to remove it in i2c-dev and allow 65535 bytes. What do you
think?

> > +static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
> > +{
> > +	__u32 i, j;
> > +	FILE *output = flags & PRINT_STDERR ? stderr : stdout;
> 
> Why don't you just pass the FILE * argument as the first parameter of
> this function?

To keep the argument list of this function small.

> > +
> > +		if (flags & PRINT_HEADER)
> > +			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
> > +				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> > +		if (msgs[i].len &&
> > +		   (read == !!(flags & PRINT_READ_BUF) ||
> > +		   !read == !!(flags & PRINT_WRITE_BUF))) {
> 
> Could be indented (aligned) better to improve readability. Also the
> last two statements seems needlessly complex. I'd write:
> 
> 		if (msgs[i].len &&
> 		    (read && (flags & PRINT_READ_BUF) ||
> 		     !read && (flags & PRINT_WRITE_BUF))) {

Yes, I like. I even moved the last two lines into a seperate assignment
of a variable 'print_buf' and do now:

	if (msgs[i].len && print_buf) { ... }

> 
> > +			if (flags & PRINT_HEADER)
> > +				fprintf(output, ", buf ");
> 
> You'd rather drop the trailing white space...
> 
> > +			for (j = 0; j < msgs[i].len; j++)
> > +				fprintf(output, "0x%02x ", msgs[i].buf[j]);
> 
> ... and move the white space at the beginning here. This avoids printing
> a trailing space before the newline (which could cause an extra blank
> line on display if you are unlucky with the terminal width.)

I can't do that because I then I get a leading white space when
!PRINT_HEADER. I use an extra fprintf for the last byte now. That also
gave me an idea how to refactor the newline handling. All the double
negation is gone, too. Looks much better now!

> > +			newline = 1;
> > +		}
> > +		if (newline)
> > +			fprintf(output, "\n");
> 
> In which case exactly would the newline not be printed? Read of length
> 0? I doubt it is worth caring about. I'd even say that printing a blank
> line in this case is preferable, so that the output really corresponds
> to what happened. Otherwise you can't tell the difference between read
> of length 0 followed by read of length 1 and the other way around.

The idea is: For !PRINT_HEADER, print only the data bytes received. Because the
user crafted the transfer, I consider it known which line means what. If this
is not the case, '-v' (PRINT_HEADER) should be used for debugging. With that in
mind, I don't think a newline should be printed for read messages with length
0.

> > +int main(int argc, char *argv[])
> > +{
> > +	char filename[20];
> > +	char *end;
> > +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> > +	int force = 0, yes = 0, version = 0, verbose = 0;
> > +	unsigned buf_idx = 0;
> > +	unsigned long len, raw_data;
> > +	__u8 data;
> > +	__u8 *buf;
> > +	__u16 flags;
> > +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> > +	struct i2c_rdwr_ioctl_data rdwr;
> > +	enum parse_state state = PARSE_GET_DESC;
> 
> That's a lot of local variables. I think some of them could be declared
> inside the state machine. Did you try moving the state machine to a
> separate function? Maybe if would improve readability.

I reduced the scope of a few variables. TBH I don't feel like putting the state
machine into a seperate function right now. It does have overhead and it feels
a bit like an academic exercise to me. And I had my share of academic excercise
these days with freeing the buffers (see later) ;)

> > +			len = strtoul(arg_ptr, &end, 0);
> > +			if (len > 65535) {
> 
> 0xffff would be easier to understand IMHO. Also you must check that
> arg_ptr != end, i.e. you managed to parse anything. Otherwise a faulty
> argument like w@0x50 will be parsed successfully (silently assuming len
> = 0) and odds are that the following argument won't be parsed
> successfully. In that case the error message will point the user to the
> wrong argument.

Great catch, thanks!

> 
> > +				fprintf(stderr, "Error: Length invalid\n");
> > +				goto err_out;
> > +			}
> > +
> > +			arg_ptr = end;
> > +			if (*arg_ptr) {
> > +				if (*arg_ptr++ != '@') {
> > +					fprintf(stderr, "Error: No '@' after length\n");
> > +			if (len) {
> > +				buf = malloc(len);
> > +				if (!buf) {
> > +					fprintf(stderr, "Error: No memory for buffer\n");
> > +					goto err_out;
> > +				}
> > +				memset(buf, 0, len);
> > +				msgs[nmsgs].buf = buf;
> > +			}
> 
> Else msgs[nmsgs].buf should be set to NULL. Otherwise you're passing a
> random address value to the kernel.

I can't do the else branch as you suggested. If I hit an error path before this
if-block, the pointer is still uninitialized and the cleanup loop will try to
free this dangling pointer. So, I need to do it earlier which is why I had the
extra loop initializing all messages in the next patch. I tried clearing the
pointer at the beginning of the switch-statement. But that caused problems,
too, when nmsgs == 0 as mentioned in the next paragraph. Then we skip the
parsing while-loop and ALL pointers are dangling.

> 
> > +
> > +	if (state != PARSE_GET_DESC || nmsgs == 0) {
> 
> I'd be surprised if nmsgs == 0 can actually happen. You checked that at
> least one parameter was left to parse on the command line before
> entering the state machine. If you couldn't parse it then it must have
> triggered some error earlier and you'll never reach this test.

It can. 'i2ctransfer 0'

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

I still think this makes the code less readable and unnecessarily big [1],
but I assume Jean insists on it :) So, here is an add-on patch to squash.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

[1] http://www.gnu.org/software/libc/manual/html_node/Freeing-after-Malloc.html#Freeing-after-Malloc

"There is no point in freeing blocks at the end of a program, because
all of the program’s space is given back to the system when the process
terminates."
---
 tools/i2ctransfer.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
index 27f4d7a..418e303 100644
--- a/tools/i2ctransfer.c
+++ b/tools/i2ctransfer.c
@@ -127,7 +127,7 @@ int main(int argc, char *argv[])
 {
 	char filename[20];
 	char *end;
-	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
+	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
 	int force = 0, yes = 0, version = 0, verbose = 0;
 	unsigned buf_idx = 0;
 	unsigned long len, raw_data;
@@ -138,6 +138,9 @@ int main(int argc, char *argv[])
 	struct i2c_rdwr_ioctl_data rdwr;
 	enum parse_state state = PARSE_GET_DESC;
 
+	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
+		msgs[i].buf = NULL;
+
 	/* handle (optional) arg_idx first */
 	while (arg_idx < argc && argv[arg_idx][0] = '-') {
 		switch (argv[arg_idx][1]) {
@@ -178,7 +181,7 @@ int main(int argc, char *argv[])
 		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
 			fprintf(stderr, "Error: Too many messages (max: %d)\n",
 				I2C_RDRW_IOCTL_MAX_MSGS);
-			exit(1);
+			goto err_out;
 		}
 
 		switch (state) {
@@ -190,20 +193,20 @@ int main(int argc, char *argv[])
 			case 'w': break;
 			default:
 				fprintf(stderr, "Error: Invalid direction\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 
 			len = strtoul(arg_ptr, &end, 0);
 			if (len > 65535) {
 				fprintf(stderr, "Error: Length invalid\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 
 			arg_ptr = end;
 			if (*arg_ptr) {
 				if (*arg_ptr++ != '@') {
 					fprintf(stderr, "Error: No '@' after length\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 
 				/* We skip 10-bit support for now. If we want it, it
@@ -213,16 +216,16 @@ int main(int argc, char *argv[])
 
 				address = parse_i2c_address(arg_ptr);
 				if (address < 0)
-					goto err_out;
+					goto err_out_with_arg;
 
 				if (!force && set_slave_addr(file, address, 0))
-					goto err_out;
+					goto err_out_with_arg;
 
 			} else {
 				/* Reuse last address if possible */
 				if (address < 0) {
 					fprintf(stderr, "Error: No address given\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 			}
 
@@ -234,7 +237,7 @@ int main(int argc, char *argv[])
 				buf = malloc(len);
 				if (!buf) {
 					fprintf(stderr, "Error: No memory for buffer\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 				memset(buf, 0, len);
 				msgs[nmsgs].buf = buf;
@@ -253,7 +256,7 @@ int main(int argc, char *argv[])
 			raw_data = strtoul(arg_ptr, &end, 0);
 			if (raw_data > 255) {
 				fprintf(stderr, "Error: Data byte invalid\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 			data = raw_data;
 			len = msgs[nmsgs].len;
@@ -270,7 +273,7 @@ int main(int argc, char *argv[])
 				case '=': break;
 				default:
 					fprintf(stderr, "Error: Invalid data byte suffix\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 			}
 
@@ -283,7 +286,7 @@ int main(int argc, char *argv[])
 
 		default:
 			fprintf(stderr, "Error: Unnkown state in state machine!\n");
-			goto err_out;
+			goto err_out_with_arg;
 		}
 
 		arg_idx++;
@@ -291,18 +294,18 @@ int main(int argc, char *argv[])
 
 	if (state != PARSE_GET_DESC || nmsgs = 0) {
 		fprintf(stderr, "Error: Incomplete message\n");
-		exit(1);
+		goto err_out;
 	}
 
 	if (!yes && !confirm(filename, msgs, nmsgs))
-		exit(0);
+		goto out;
 
 	rdwr.msgs = msgs;
 	rdwr.nmsgs = nmsgs;
 	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
 	if (nmsgs_sent < 0) {
 		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
-		exit(errno);
+		goto err_out;
 	} else if (nmsgs_sent < nmsgs) {
 		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
 	}
@@ -311,10 +314,17 @@ int main(int argc, char *argv[])
 
 	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
 
-	/* let Linux free malloced memory on termination */
+out:
+	for (i = 0; i <= nmsgs; i++)
+		free(msgs[i].buf);
+
 	exit(0);
 
-err_out:
+err_out_with_arg:
 	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
+err_out:
+	for (i = 0; i <= nmsgs; i++)
+		free(msgs[i].buf);
+
 	exit(1);
 }
-- 
2.1.4

Re: [PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Jean Delvare]

Hi Wolfram,

On Fri, 19 Jun 2015 12:40:32 +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I still think this makes the code less readable and unnecessarily big [1],
> but I assume Jean insists on it :) So, here is an add-on patch to squash.

Oh yeah. I'd also love if you could close the i2c device file before
leaving, even in error cases ;-)

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> [1] http://www.gnu.org/software/libc/manual/html_node/Freeing-after-Malloc.html#Freeing-after-Malloc
> 
> "There is no point in freeing blocks at the end of a program, because
> all of the program’s space is given back to the system when the process
> terminates."

Yeah, like the GNU folks are right on everything. Just see their
recommended coding style... :D

I know that the memory would be freed anyway. But I think there is
value in consistency. Also freeing the memory documents the memory
allocation model as a nice side effect. And avoids bad surprises when
one copies code from a command line tool to a GUI tool or a daemon. And
it lets you run the code under valgrind.

So I see the cost but I still believe that the benefits outweigh that
cost.

> ---
>  tools/i2ctransfer.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> index 27f4d7a..418e303 100644
> --- a/tools/i2ctransfer.c
> +++ b/tools/i2ctransfer.c
> @@ -127,7 +127,7 @@ int main(int argc, char *argv[])
>  {
>  	char filename[20];
>  	char *end;
> -	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
>  	int force = 0, yes = 0, version = 0, verbose = 0;
>  	unsigned buf_idx = 0;
>  	unsigned long len, raw_data;
> @@ -138,6 +138,9 @@ int main(int argc, char *argv[])
>  	struct i2c_rdwr_ioctl_data rdwr;
>  	enum parse_state state = PARSE_GET_DESC;
>  
> +	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
> +		msgs[i].buf = NULL;
> +

If you explicitly set "buf = NULL" for zero-length messages in the
state machine as recommended in my review of the previous patch, this
is no longer needed.

>  	/* handle (optional) arg_idx first */
>  	while (arg_idx < argc && argv[arg_idx][0] = '-') {
>  		switch (argv[arg_idx][1]) {
> @@ -178,7 +181,7 @@ int main(int argc, char *argv[])
>  		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
>  			fprintf(stderr, "Error: Too many messages (max: %d)\n",
>  				I2C_RDRW_IOCTL_MAX_MSGS);
> -			exit(1);
> +			goto err_out;
>  		}
>  
>  		switch (state) {
> @@ -190,20 +193,20 @@ int main(int argc, char *argv[])
>  			case 'w': break;
>  			default:
>  				fprintf(stderr, "Error: Invalid direction\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  
>  			len = strtoul(arg_ptr, &end, 0);
>  			if (len > 65535) {
>  				fprintf(stderr, "Error: Length invalid\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  
>  			arg_ptr = end;
>  			if (*arg_ptr) {
>  				if (*arg_ptr++ != '@') {
>  					fprintf(stderr, "Error: No '@' after length\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  
>  				/* We skip 10-bit support for now. If we want it, it
> @@ -213,16 +216,16 @@ int main(int argc, char *argv[])
>  
>  				address = parse_i2c_address(arg_ptr);
>  				if (address < 0)
> -					goto err_out;
> +					goto err_out_with_arg;
>  
>  				if (!force && set_slave_addr(file, address, 0))
> -					goto err_out;
> +					goto err_out_with_arg;
>  
>  			} else {
>  				/* Reuse last address if possible */
>  				if (address < 0) {
>  					fprintf(stderr, "Error: No address given\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  			}
>  
> @@ -234,7 +237,7 @@ int main(int argc, char *argv[])
>  				buf = malloc(len);
>  				if (!buf) {
>  					fprintf(stderr, "Error: No memory for buffer\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  				memset(buf, 0, len);
>  				msgs[nmsgs].buf = buf;
> @@ -253,7 +256,7 @@ int main(int argc, char *argv[])
>  			raw_data = strtoul(arg_ptr, &end, 0);
>  			if (raw_data > 255) {
>  				fprintf(stderr, "Error: Data byte invalid\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  			data = raw_data;
>  			len = msgs[nmsgs].len;
> @@ -270,7 +273,7 @@ int main(int argc, char *argv[])
>  				case '=': break;
>  				default:
>  					fprintf(stderr, "Error: Invalid data byte suffix\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  			}
>  
> @@ -283,7 +286,7 @@ int main(int argc, char *argv[])
>  
>  		default:
>  			fprintf(stderr, "Error: Unnkown state in state machine!\n");
> -			goto err_out;
> +			goto err_out_with_arg;

I'd stick to err_out in this case. As this isn't supposed to happen,
you have no idea if printing argv[arg_idx] is relevant or not. And it
is likely to confuse the user.

>  		}
>  
>  		arg_idx++;
> @@ -291,18 +294,18 @@ int main(int argc, char *argv[])
>  
>  	if (state != PARSE_GET_DESC || nmsgs = 0) {
>  		fprintf(stderr, "Error: Incomplete message\n");
> -		exit(1);
> +		goto err_out;
>  	}
>  
>  	if (!yes && !confirm(filename, msgs, nmsgs))
> -		exit(0);
> +		goto out;
>  
>  	rdwr.msgs = msgs;
>  	rdwr.nmsgs = nmsgs;
>  	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
>  	if (nmsgs_sent < 0) {
>  		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
> -		exit(errno);
> +		goto err_out;
>  	} else if (nmsgs_sent < nmsgs) {
>  		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
>  	}
> @@ -311,10 +314,17 @@ int main(int argc, char *argv[])
>  
>  	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
>  
> -	/* let Linux free malloced memory on termination */
> +out:

One space before labels please, so as to not break "diff -p".

> +	for (i = 0; i <= nmsgs; i++)
> +		free(msgs[i].buf);

It would be <, not <=.

Another approach is:

	for (; nmsgs >= 0; nmsgs--)
		free(msgs[nmsgs].buf);

which avoids introducing another loop variable.

> +
>  	exit(0);
>  
> -err_out:
> +err_out_with_arg:
>  	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> +err_out:
> +	for (i = 0; i <= nmsgs; i++)
> +		free(msgs[i].buf);
> +
>  	exit(1);
>  }

Thanks for doing that.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]


> > +	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
> > +		msgs[i].buf = NULL;
> > +
> 
> If you explicitly set "buf = NULL" for zero-length messages in the
> state machine as recommended in my review of the previous patch, this
> is no longer needed.

It is as stated in my previous mail.

We should really play safe here. If we optimize too much, the cleanup
loops become fragile (I tried some options) and one easily misses a
case. Cleaning up is hard, we know that from Linux drivers.

(For me, this is still a good example how letting the OS free memory can
actually prevent bugs. But I give in already ;))


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]