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

[Wolfram Sang <wsa@the-dreams.de>]

[-- 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 --]