On Thu, Feb 10, 2005 at 12:57:12AM +0100, Francois Romieu wrote:
> Janitoring - error handling during attach
> - av7110_arm_sync(): small helper to factor out some code;
> - av7110_attach() does not check the status code returned by all the
> functions is uses;
> - balance the error path in av7110_attach and have it easy to check.
> Please check it;
> - if everything is correctly balanced, device_initialized is not needed
> anymore in struct av7110;
> - av7110_detach(): no need to cast a void * pointer;
> - av7110_detach(): die #ifdef, die !
> - change the returned value of av7110_av_exit() as it can't fail;
> - change the returned value of av7110_ca_init() as it can fail. Removed
> extraneous casts while are it.
Very good, but the patch doesn't apply (4 out of 14 hunks FAILED
for av7110.c). Could you please send an updated patch?
> - ttpci_eeprom_parse_mac(&av7110->i2c_adap, av7110->dvb_adapter->proposed_mac);
> + ret = ttpci_eeprom_parse_mac(&av7110->i2c_adap,
> + av7110->dvb_adapter->proposed_mac);
> + if (ret < 0)
> + goto err_i2c_del_3;
Not good. I don't think a wrong eeprom checksum should keep
people from using the card.
> + for (i = 0; i < 2; i++) {
> + struct ipack *ipack = av7110->ipack + i;
> +
> + ret = av7110_ipack_init(ipack, IPACKS, play[i]);
> + if (ret < 0) {
> + while (i--)
> + av7110_ipack_free(ipack);
> + goto out;
> + }
> + ipack->data = av7110;
> + }
av7110_ipack_free() would free the wrong ipack. Also, IMHO the loop
is superflous since we'll never have more than two ipacks.
> static int ci_ll_init(struct dvb_ringbuffer *cirbuf, struct dvb_ringbuffer *ciwbuf, int size)
> {
> + /* TODO: is it legal to not check the value returned by vmalloc ? */
> dvb_ringbuffer_init(cirbuf, vmalloc(size), size);
> dvb_ringbuffer_init(ciwbuf, vmalloc(size), size);
For sure the vmalloc() return value must be checked.
Johannes
