Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()

[Dan Carpenter]

The "brd->nasync = 0;" was wrong, yes, but my main complaint was that
you are writing complicated error handling.  This v2 patch makes the
error handling even more complicated.  If dgap_tty_init() fails it
should free the things it allocates itself, instead of the caller
handling errors for it.

It's not actually that hard.  The only error handling we need to do in
dgap_tty_init() is if the kzalloc() fails:

  1374          /*
  1375           * Allocate channel memory that might not have been allocated
  1376           * when the driver was first loaded.
  1377           */
  1378          for (i = 0; i < brd->nasync; i++) {
  1379                  if (!brd->channels[i]) {
  1380                          brd->channels[i] =
  1381                                  kzalloc(sizeof(struct channel_t), GFP_KERNEL);
  1382                          if (!brd->channels[i])
  1383                                  return -ENOMEM;

Instead of returning directly here we should free the previous
allocations.

  1384                  }
  1385          }

The code is confusing because which ones did we allocate and which ones
were already non-NULL at the start of the function?  In other words
the "if (!brd->channels[i]) {" test?

The answer is that the comment and the test seem to be wrong they were
all NULL at the start of the function.  Just add a:

free_chan:
	while (--i >= 0) {
		kfree(brd->channels[i]);
		brd->channels[i] = NULL;
	}
	return ret;

Actually, for these I would introduce an "int chan" variable just for
that loop instead of "i" which we re-use.

So then we remove the call to dgap_tty_uninit() from
dgap_firmware_load().

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/