[Debian-eeepc-devel] package eeepc-acpi-scripts: inefficiencies and inconsistencies

[Cristian Ionescu-Idbohrn]

On Mon, 5 Apr 2010, Damyan Ivanov wrote:

> -=| Cristian Ionescu-Idbohrn, Mon, Apr 05, 2010 at 10:56:20AM +0200 |=-
> >
> > Should I post patches to this list, or shall we take that off list?
>
> List is fine.
>
> > Do you like big or small patches?
>
> Small, self-contained.
>
> > All tested or even untested?  I don't think I can test all, as the
> > hardware I own does not have all "features".
> > And/or bugs :)
>
> Test the ones you can. Be careful with the rest.

Alright.

The attached pach cleans up whitespace, adjusts indentation, removes some
unnecessary quoting, declares a local variable, rewords a comment and
removes another.  Not too controversial, I would hope :)

Observations:

* 'functions.sh' uses '/bin/ps' in 'detect_x_display', but the package
  doesn't depend on 'procps'

* 'functions.sh' uses '/bin/fgconsole' in 'detect_x_display', but the
  package doesn't depend on 'console-tools' (yes, the package depends on
  'acpi-support-base', which depends on 'console-tools', but is that a
  reliable dependency?

* most functions do _not_ validate arguments before using them; that opens
  for subtle bugs being more difficult to locate

* I'd like to suggest a function to be used in conjunction with error
  handling, using '/usr/bin/logger' instead of echo; something along these
  lines:

	SELF="${0##*/}[$$]"
	logit() {
		[ $# -gt 1 ] || {
			echo "$SELF: logit: missing argument(s)" >&2
			exit 1
		}
		logger -t $SELF -p $1 -- $*
	}

* it's difficult to distinguish between global and (undeclared) local
  variables; what about upper case _only_ for global variables and lower
  case for the local ones (see f.ex. function 'detect_x_display':

	user=$_user
	home=$_home

  are 'user' and 'home' global or local variables?)

* functions that update global/the caller's variables open a can of worms,
  IMO; that should be avoided; telling the function the variable name is a
  better way (TM) to do it; something like:

	func() {
		# $1 - name of the variable to be updated
		[ $# -ge 1 ] ||
			echo "$SELF: func: missing argument(s)" >&2
			exit 1
		}
		...
		eval $1=...
	}

* rfkill _does_ return error messages:

	# rfkill list otto
	Bogus list argument 'otto'.
	$ echo $?
	0
	# rfkill list ""
	Bogus list argument ''.
	# echo $?
	0

  but not error status; I propose filing a bug against rfkill

* is there a good reason for running some of the programs using the whole
  path (/usr/bin/gnome-screensaver-command, /usr/bin/xtrlock, etc.) but
  not others (cut, who, getent, dcop, etc.)


Cheers,

-- 
Cristian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: functions.sh.1.patch
Type: text/x-diff
Size: 4849 bytes
Desc: 
URL: <http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/attachments/20100405/9bed5064/attachment.patch>