| Message ID | 1434710432-4182-3-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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.
> > + 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 ;))
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); }