Re: [PATCH V3 05/19] OMAP3+: voltage: use IS_ERR_OR_NULL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Russell King - ARM Linux wrote, on 03/06/2011 01:48 PM:
On Sun, Mar 06, 2011 at 08:15:06AM +0530, Nishanth Menon wrote:
my_dumb_func(){
	struct voltagedomain *vdata = NULL;
	if (cpu_is_omap3630()_) {
		vdata = omap_voltage_domain_lookup("mpu")
	}
	/* forgot to add other cpu types or a else case */
	/* do other things */
	me_volt=omap_voltage_get_nom_volt(vdata);
	/* do things with me_volt */
}

Sorry, but i think the check, even if seems superfluous is sane IMHO.
even if the above code worked on 3630, it'd fail on o4/3430 without the
check, it can even crash. and given that we've all seen our fair share
of developers who develop for one platform without consideration that
the rest of the world uses others as well... I do feel cases like above
example might infact be a reality.

But normal practice is to check the return value from functions before
it's used.  So:

  my_dumb_func(){
	struct voltagedomain *vdata = NULL;
	if (cpu_is_omap3630()) {
		vdata = omap_voltage_domain_lookup("mpu")
	}
	/* forgot to add other cpu types or a else case */

+	if (!vdata)
+		return some error;

	/* do other things */
	me_volt=omap_voltage_get_nom_volt(vdata);
	/* do things with me_volt */
  }

is the right way to deal with this.  Pushing the primary error checking
down into sub-functions is stupid - where does it stop?  Do you check
me_volt for errors?  Do you get functions which use me_volt to check for
errors from that too?

It's a silly idea.  Put the primary error checking in the function which
gets the return value.  Don't bury it in sub-functions.

I agree with you on this - sub functions esp static ones should able to trust the parameters passed to it by callers. for the moment, I suggest we drop this patch from this series - it has no functional impact to the overall goal which I was attempting to achieve. Cleanups of the voltage, smartreflex layers are an ongoing activity currently and should be takenup as part of it.

--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux