Hi Wolfram, On Fri, 19 Jun 2015 12:40:32 +0200, Wolfram Sang wrote: > From: Wolfram Sang <wsa+rene...@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+rene...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
