I reviewed the patch. Thinking aloud.. * hdparm_is_on_battery doesn't check whether on_ac_power actually exists (it's only recommended), but since it explicitly tests for $? == 1, and "command not found" yields 127, this should be okay. It means that if powermgmt-base is not installed, we default to the current behaviour, which makes sense.
* The new hdparm_apm_option_for_disk() function has the same structure as in /lib/udev/hdparm, except it only looks out for some particular parameters. If this is accepted in Debian, this probably needs some refactoring. In anticipation of this, could hdparm-functions perhaps be installed into /lib, not /usr/lib/? Then the udev script could use it as well. * hdparm_apm_option_for_disk() could do with a comment describing parameters and return value, and mention that the result will go to stdout * There seems to be an inconsistency between the manpage (saying that the default value of apm is 255), and the code (which defaults to 254). In general I'm in favour of this, since it's pretty much a bug fix. The code duplication is okay for now since it (a) avoids structural changes in existing code, and (b) it does not make sense to work on this until Debian agrees to that change in general. -- pm-utils doesn't reload hdparm.conf after a suspend https://bugs.launchpad.net/bugs/238555 You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
