Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

Eddy Petrișor
X-Debbugs-Cc: [hidden email]

Package: laptop-mode-tools
Version: 1.61-2
Followup-For: Bug #673818

I was looking over the fix for this bug, but it seems to me the fix changes the
behaviour of laptop-mode.

Previously the /usr/sbin/laptop_mode script looked for files with the .conf
suffix, but now every file in the /etc/laptop-mode/conf.d and /etc/laptop-
mode/conf.d/board-specific directories.

I use the previous type of behaviour to disable specific scripts by renaming
them to somehting not matching the pattern (e.g. bluetooth.conf.disabled), and
I suspect others did, too.

With the change, those scripts would be reenabled on upgrade.



Please see below a proposal for the fix that does not have the
described problem, nor the warning that caused this BR:

--- laptop_mode.sid 2013-08-11 12:30:23.000000000 +0300
+++ laptop_mode 2013-08-11 12:28:10.000000000 +0300
@@ -234,7 +234,7 @@
     test -d /etc/laptop-mode/conf.d/board-specific &&
CONF_DIR="$CONF_DIR /etc/laptop-mode/conf.d/board-specific"

     for PER_DIR in $CONF_DIR; do
- for CONF in $PER_DIR/*; do
+ for CONF in $(ls $PER_DIR/* 2>/dev/null); do
     if [ -r "$CONF" ] ; then
     . "$CONF"
     #Handle individual module debug settings


This also has the advantage of not trying to print wildcard matching patterns.


-- System Information:
Debian Release: 7.1
  APT prefers stable
  APT policy: (500, 'stable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 3.10.0-heidi (SMP w/2 CPU cores)
Locale: LANG=ro_RO.utf8, LC_CTYPE=ro_RO.utf8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages laptop-mode-tools depends on:
ii  lsb-base    4.1+Debian8+deb7u1
ii  psmisc      22.19-1+deb7u1
ii  util-linux  2.20.1-5.3

Versions of packages laptop-mode-tools recommends:
pn  ethtool         <none>
ii  hdparm          9.39-1+b1
ii  net-tools       1.60-24.2
ii  sdparm          1.07-1
ii  udev            175-7.2
ii  wireless-tools  30~pre9-8

Versions of packages laptop-mode-tools suggests:
ii  acpid  1:2.0.16-1+deb7u1
ii  hal    0.5.14-8

-- no debconf information


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

Ritesh Raj Sarraf-3
On Sunday 11 August 2013 03:04 PM, Eddy Petrișor wrote:

> --- laptop_mode.sid 2013-08-11 12:30:23.000000000 +0300
> +++ laptop_mode 2013-08-11 12:28:10.000000000 +0300
> @@ -234,7 +234,7 @@
>      test -d /etc/laptop-mode/conf.d/board-specific &&
> CONF_DIR="$CONF_DIR /etc/laptop-mode/conf.d/board-specific"
>
>      for PER_DIR in $CONF_DIR; do
> - for CONF in $PER_DIR/*; do
> + for CONF in $(ls $PER_DIR/* 2>/dev/null); do
>      if [ -r "$CONF" ] ; then
>      . "$CONF"
>      #Handle individual module debug settings

This is already fixes in my upstream branch. It will be part of the next
release.

BTW, won't just *.conf solve the problem. The current fix in my branch is:

diff --git a/usr/sbin/laptop_mode b/usr/sbin/laptop_mode
index 71beab2..070f1de 100755
--- a/usr/sbin/laptop_mode
+++ b/usr/sbin/laptop_mode
@@ -234,7 +234,7 @@ lmt_load_config ()
     test -d /etc/laptop-mode/conf.d/board-specific &&
CONF_DIR="$CONF_DIR /etc/laptop-mode/conf.d/board-specific
 
     for PER_DIR in $CONF_DIR; do
-           for CONF in $PER_DIR/*; do
+           for CONF in $PER_DIR/*.conf; do
                    if [ -r "$CONF" ] ; then
                            . "$CONF"
                            #Handle individual module debug settings


--
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."



signature.asc (915 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

Eddy Petrișor
2013/8/11 Ritesh Raj Sarraf <[hidden email]>:
> This is already fixes in my upstream branch. It will be part of the next
> release.
>
> BTW, won't just *.conf solve the problem.

Yes, I realised just after sending the mail I quoted the wrong patch. I sent the correct patch in a follow up email:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=673818#24

> The current fix in my branch is:
>
[..]
>      for PER_DIR in $CONF_DIR; do
> -           for CONF in $PER_DIR/*; do
> +           for CONF in $PER_DIR/*.conf; do
>                     if [ -r "$CONF" ] ; then


The fix in the repo still has the original problem, because if the /etc/laptop-mode/conf.d or /etc/laptop-mode/conf.d/board-specific directories exists, but contains no .conf files, the script will generate the ugly warning initially reported since the shell does not expand the wildcard.


OTOH, the form below (and in the second patch) will not generate it because the expansion of the ls command is an empty string, so $CONF is never set to an invalid file:

for CONF in $(ls $PER_DIR/*.conf 2>/dev/null); do



Patch is here:
http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=24;filename=673818.patch;att=1;bug=673818


--
Regards,
EddyP
=============================================
The universe is not required to be in perfect harmony with human ambition. - Carl Sagan
Reply | Threaded
Open this post in threaded view
|

Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

Ritesh Raj Sarraf-3
On Sunday 11 August 2013 05:37 PM, Eddy Petrișor wrote:
> The fix in the repo still has the original problem, because if the
> /etc/laptop-mode/conf.d or /etc/laptop-mode/conf.d/board-specific
> directories exists, but contains no .conf files, the script will
> generate the ugly warning initially reported since the shell does not
> expand the wildcard.

I am not able to determine why the current fix will fail. Please see
below testcase.


rrs@zan:/etc/laptop-mode$ ls conf.d/
ac97-powersave.conf         configuration-file-control.conf
exec-commands.conf         nmi-watchdog.conf            
start-stop-programs.conf  wireless-iwl-power.conf
auto-hibernate.conf         cpufreq.conf                    
hal-polling.conf           pcie-aspm.conf              
terminal-blanking.conf    wireless-power.conf
battery-level-polling.conf  dpms-standby.conf              
intel-hda-powersave.conf   runtime-pm.conf              
usb-autosuspend.conf
bluetooth.conf              eee-superhe.conf                
intel-sata-powermgmt.conf  sched-mc-power-savings.conf   video-out.conf
board-specific/             ethernet.conf                  
lcd-brightness.conf        sched-smt-power-savings.conf
wireless-ipw-power.conf

rrs@zan:/etc/laptop-mode$ ls conf.d/board-specific/

rrs@zan:/etc/laptop-mode$ for x in conf.d/*.conf; do test -r $x &&  echo
$x; done
conf.d/ac97-powersave.conf
conf.d/auto-hibernate.conf
conf.d/battery-level-polling.conf
conf.d/bluetooth.conf
conf.d/configuration-file-control.conf
conf.d/cpufreq.conf
conf.d/dpms-standby.conf
conf.d/eee-superhe.conf
conf.d/ethernet.conf
conf.d/exec-commands.conf
conf.d/hal-polling.conf
conf.d/intel-hda-powersave.conf
conf.d/intel-sata-powermgmt.conf
conf.d/lcd-brightness.conf
conf.d/nmi-watchdog.conf
conf.d/pcie-aspm.conf
conf.d/runtime-pm.conf
conf.d/sched-mc-power-savings.conf
conf.d/sched-smt-power-savings.conf
conf.d/start-stop-programs.conf
conf.d/terminal-blanking.conf
conf.d/usb-autosuspend.conf
conf.d/video-out.conf
conf.d/wireless-ipw-power.conf
conf.d/wireless-iwl-power.conf
conf.d/wireless-power.conf

rrs@zan:/etc/laptop-mode$ for x in conf.d/board-specific/*.conf; do test
-r $x && echo $x; done
rrs@zan:/etc/laptop-mode$


--
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."



signature.asc (915 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

Eddy Petrișor
2013/8/11 Ritesh Raj Sarraf <[hidden email]>

>
> On Sunday 11 August 2013 05:37 PM, Eddy Petrișor wrote:
> > The fix in the repo still has the original problem, because if the
> > /etc/laptop-mode/conf.d or /etc/laptop-mode/conf.d/board-specific
> > directories exists, but contains no .conf files, the script will
> > generate the ugly warning initially reported since the shell does not
> > expand the wildcard.
>
> I am not able to determine why the current fix will fail. Please see
> below testcase.
>
You failed to reproduce the else branch in your test. The attached
test script displays the issue:

heidi:/home/eddy/usr/src/bugs/673818# mkdir
/etc/laptop-mode/conf.d/board-specific
heidi:/home/eddy/usr/src/bugs/673818# ls /etc/laptop-mode/conf.d/board-specific
heidi:/home/eddy/usr/src/bugs/673818# ./test
expansion
MSG Warning: Configuration file
/etc/laptop-mode/conf.d/board-specific/*.conf is not readable,
skipping.
using ls


It is basically the same issue as reported in the bug.


--
Regards,
EddyP
=============================================
The universe is not required to be in perfect harmony with human
ambition. - Carl Sagan

test (996 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#673818: laptop-mode-tools: ugly warning related to board-specific configuration - fix changes behaviour

Ritesh Raj Sarraf-3
On Sunday 11 August 2013 06:25 PM, Eddy Petrișor wrote:
You failed to reproduce the else branch in your test. The attached
test script displays the issue:

heidi:/home/eddy/usr/src/bugs/673818# mkdir
/etc/laptop-mode/conf.d/board-specific
heidi:/home/eddy/usr/src/bugs/673818# ls /etc/laptop-mode/conf.d/board-specific

Okay!!! Now I recollect why you are seeing this issue. This issue was fixed in 1.63, which is currently in Sid/Testing.
http://ftp-master.metadata.debian.org/changelogs/main/l/laptop-mode-tools/unstable_changelog

The conf.d/board-specific/ settings were added to accommodate vendors who'd like to override LMT's default values.

Since 1.63, we do not ship the board-specific/ sub-folder, because that is the duty of hw vendors. With that assumption, I do not see this as a bug.


Your patch is okay, but adds a subshell in it. :-(
LMT already is crawling in terms of performance. Its runtime is around 7-8 secs.



Now coming back to your first email:

===
Previously the /usr/sbin/laptop_mode script looked for files with the .conf
suffix, but now every file in the /etc/laptop-mode/conf.d and /etc/laptop-
mode/conf.d/board-specific directories.
===
Like I mentioned in one of the previous mails, I think this is fixed in commit: 6f81613ebd2ef7eba88ea2edd596f9441be2c294


I guess it is time I tag and push 1.64.
-- 
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."

signature.asc (915 bytes) Download Attachment