> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Wednesday, February 18, 2015 2:24 AM > To: Jake Oshins > Cc: rafael.j.wysocki@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; KY Srinivasan; > linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx > Subject: Re: [PATCH 2/3] drivers:hv Convert VMBus and its descendants to PnP > > On Tue, Feb 17, 2015 at 11:41:50AM -0800, Jake Oshins wrote: > > > > + ret = pnp_add_descendant(child_device_obj->pnp_dev); > > if (ret) > > + goto register_exit; > > + > > + added = TRUE; > > + > > + ret = pnp_activate_dev(child_device_obj->pnp_dev); > > + > > +register_exit: > > + > > + if (ret) { > > + if (added) > > + pnp_remove_descendant(child_device_obj->pnp_dev); > > + free_pnp_descendant(child_device_obj->pnp_dev); > > pr_err("Unable to register child device\n"); > > - else > > + } else > > pr_debug("child device %s registered\n", > > - dev_name(&child_device_obj->device)); > > + dev_name(&child_device_obj->pnp_dev->dev)); > > > > return ret; > > } > > This error handling is horribly ugly and has a bug. Please, read my > essay on how to do error handling properly. > > https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ > > 1) Don't do "One Err" style error handling. > 2) Don't twist the success path and the error paths together. > 3) Don't use the "added" variable. > 4) Don't initialize "ret" at the start of the function. > 5) Don't forget to undo pnp_descendant_memory_option() > > It should look something like: > > return 0; > > err_remove_desc: > pnp_remove_descendant(child_device_obj->pnp_dev); > err_remove_option: > if (bytes) > remove_whatever(); > err_free_desc: > free_pnp_descendant(child_device_obj->pnp_dev); > > return ret; > > regards, > dan carpenter Will do. Thanks for the review. -- Jake Oshins _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
