Patchwork [V4,09/10] irda-utils: fix for read-only rootfs

login
register
mail settings
Submitter Qi.Chen@windriver.com
Date Aug. 7, 2013, 8:08 a.m.
Message ID <3ea48b1c837fd67e81f367ff6943cae9f7fbeb33.1375862591.git.Qi.Chen@windriver.com>
Download mbox | patch
Permalink /patch/55243/
State New
Headers show

Comments

Qi.Chen@windriver.com - Aug. 7, 2013, 8:08 a.m.
From: Chen Qi <Qi.Chen@windriver.com>

The init script for irda writes configuration items to /etc/sysconfig/irda
if that file is not available in system. But it's actually not necessary,
the behavior doesn't change whether the init script writes to the file or not.

Considering it issues error messages in case of a read-only rootfs, I delete
the writing process.

[YOCTO #4103]
[YOCTO #4886]

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 .../irda-utils/irda-utils-0.9.18/init              |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
Chris Larson - Aug. 7, 2013, 2:18 p.m.
On Wed, Aug 7, 2013 at 1:08 AM, <Qi.Chen@windriver.com> wrote:

> From: Chen Qi <Qi.Chen@windriver.com>
>
> The init script for irda writes configuration items to /etc/sysconfig/irda
> if that file is not available in system. But it's actually not necessary,
> the behavior doesn't change whether the init script writes to the file or
> not.
>
> Considering it issues error messages in case of a read-only rootfs, I
> delete
> the writing process.
>
> [YOCTO #4103]
> [YOCTO #4886]
>
> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
>

This is a start, but is incomplete, from what I can tell. As mentioned in
the other thread, the startup script shouldn't be using /etc/sysconfig/ at
all, in any form. We use /etc/default/ for our startup script config files,
not /etc/sysconfig/. Also, the purpose of this block was clearly to
implement a default configuration, yet the recipe isn't altered to ship a
default configuration to provide equivalent functionality.
Qi.Chen@windriver.com - Aug. 8, 2013, 2:10 a.m.
On 08/07/2013 10:18 PM, Chris Larson wrote:
>
> On Wed, Aug 7, 2013 at 1:08 AM, <Qi.Chen@windriver.com 
> <mailto:Qi.Chen@windriver.com>> wrote:
>
>     From: Chen Qi <Qi.Chen@windriver.com <mailto:Qi.Chen@windriver.com>>
>
>     The init script for irda writes configuration items to
>     /etc/sysconfig/irda
>     if that file is not available in system. But it's actually not
>     necessary,
>     the behavior doesn't change whether the init script writes to the
>     file or not.
>
>     Considering it issues error messages in case of a read-only
>     rootfs, I delete
>     the writing process.
>
>     [YOCTO #4103]
>     [YOCTO #4886]
>
>     Signed-off-by: Chen Qi <Qi.Chen@windriver.com
>     <mailto:Qi.Chen@windriver.com>>
>
>
> This is a start, but is incomplete, from what I can tell. As mentioned 
> in the other thread, the startup script shouldn't be using 
> /etc/sysconfig/ at all, in any form. We use /etc/default/ for our 
> startup script config files, not /etc/sysconfig/.
Yeah, I agree with you.
But when I checked the irda source code, I saw that it actually could 
ship its own init script (irda-utils-xxx/etc/irda.rc). And I think our 
init script is derived from this one. In its own init script, 
/etc/sysconfig is used.
(Of course, I still think /etc/default is a better location for its 
configuration file.)


> Also, the purpose of this block was clearly to implement a default 
> configuration, yet the recipe isn't altered to ship a default 
> configuration to provide equivalent functionality.
In its source code, there's a default configuration file, and we don't 
use that. I think there might be a reason.
So I'm not sure about this one.

If you have a patch to fix the irda issue properly, send it out and I'll 
rebase my remote branch and drop this one.

Best Regards,
Chen Qi


> -- 
> Christopher Larson
> clarson at kergoth dot com
> Founder - BitBake, OpenEmbedded, OpenZaurus
> Maintainer - Tslib
> Senior Software Engineer, Mentor Graphics
Khem Raj - Aug. 8, 2013, 2:23 a.m.
On Wednesday, August 7, 2013, ChenQi <Qi.Chen@windriver.com> wrote:
> On 08/07/2013 10:18 PM, Chris Larson wrote:
>
> On Wed, Aug 7, 2013 at 1:08 AM, <Qi.Chen@windriver.com> wrote:
>>
>> From: Chen Qi <Qi.Chen@windriver.com>
>>
>> The init script for irda writes configuration items to
/etc/sysconfig/irda
>> if that file is not available in system. But it's actually not necessary,
>> the behavior doesn't change whether the init script writes to the file
or not.
>>
>> Considering it issues error messages in case of a read-only rootfs, I
delete
>> the writing process.
>>
>> [YOCTO #4103]
>> [YOCTO #4886]
>>
>> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
>
> This is a start, but is incomplete, from what I can tell. As mentioned in
the other thread, the startup script shouldn't be using /etc/sysconfig/ at
all, in any form. We use /etc/default/ for our startup script config files,
not /etc/sysconfig/.
>
> Yeah, I agree with you.
> But when I checked the irda source code, I saw that it actually could
ship its own init script (irda-utils-xxx/etc/irda.rc). And I think our init
script is derived from this one. In its own init script, /etc/sysconfig is
used.
> (Of course, I still think /etc/default is a better location for its
configuration file.)
>
>
> Also, the purpose of this block was clearly to implement a default
configuration, yet the recipe isn't altered to ship a default configuration
to provide equivalent functionality.
>
> In its source code, there's a default configuration file, and we don't
use that. I think there might be a reason.
> So I'm not sure about this one.
>
> If you have a patch to fix the irda issue properly, send it out and I'll
rebase my remote branch and drop this one.

Whether Chris has a patch or not this patch in its form should be dropped
since there is a better way to do it

>
> Best Regards,
> Chen Qi
>
>
> --
> Christopher Larson
> clarson at kergoth dot com
> Founder - BitBake, OpenEmbedded, OpenZaurus
> Maintainer - Tslib
> Senior Software Engineer, Mentor Graphics
>
Qi.Chen@windriver.com - Aug. 8, 2013, 2:29 a.m.
On 08/08/2013 10:23 AM, Khem Raj wrote:
>
>
> On Wednesday, August 7, 2013, ChenQi <Qi.Chen@windriver.com 
> <mailto:Qi.Chen@windriver.com>> wrote:
> > On 08/07/2013 10:18 PM, Chris Larson wrote:
> >
> > On Wed, Aug 7, 2013 at 1:08 AM, <Qi.Chen@windriver.com 
> <mailto:Qi.Chen@windriver.com>> wrote:
> >>
> >> From: Chen Qi <Qi.Chen@windriver.com <mailto:Qi.Chen@windriver.com>>
> >>
> >> The init script for irda writes configuration items to 
> /etc/sysconfig/irda
> >> if that file is not available in system. But it's actually not 
> necessary,
> >> the behavior doesn't change whether the init script writes to the 
> file or not.
> >>
> >> Considering it issues error messages in case of a read-only rootfs, 
> I delete
> >> the writing process.
> >>
> >> [YOCTO #4103]
> >> [YOCTO #4886]
> >>
> >> Signed-off-by: Chen Qi <Qi.Chen@windriver.com 
> <mailto:Qi.Chen@windriver.com>>
> >
> > This is a start, but is incomplete, from what I can tell. As 
> mentioned in the other thread, the startup script shouldn't be using 
> /etc/sysconfig/ at all, in any form. We use /etc/default/ for our 
> startup script config files, not /etc/sysconfig/.
> >
> > Yeah, I agree with you.
> > But when I checked the irda source code, I saw that it actually 
> could ship its own init script (irda-utils-xxx/etc/irda.rc). And I 
> think our init script is derived from this one. In its own init 
> script, /etc/sysconfig is used.
> > (Of course, I still think /etc/default is a better location for its 
> configuration file.)
> >
> >
> > Also, the purpose of this block was clearly to implement a default 
> configuration, yet the recipe isn't altered to ship a default 
> configuration to provide equivalent functionality.
> >
> > In its source code, there's a default configuration file, and we 
> don't use that. I think there might be a reason.
> > So I'm not sure about this one.
> >
> > If you have a patch to fix the irda issue properly, send it out and 
> I'll rebase my remote branch and drop this one.
>
> Whether Chris has a patch or not this patch in its form should be 
> dropped since there is a better way to do it
>

Shipping a default configuration file or using /etc/default as the 
configuration directory? Or something else?

//Chen Qi

> >
> > Best Regards,
> > Chen Qi
> >
> >
> > --
> > Christopher Larson
> > clarson at kergoth dot com
> > Founder - BitBake, OpenEmbedded, OpenZaurus
> > Maintainer - Tslib
> > Senior Software Engineer, Mentor Graphics
> >

Patch

diff --git a/meta/recipes-connectivity/irda-utils/irda-utils-0.9.18/init b/meta/recipes-connectivity/irda-utils/irda-utils-0.9.18/init
index 63750f1..14efb94 100755
--- a/meta/recipes-connectivity/irda-utils/irda-utils-0.9.18/init
+++ b/meta/recipes-connectivity/irda-utils/irda-utils-0.9.18/init
@@ -13,7 +13,6 @@  module_id() {
 }
 
 if [ ! -f /etc/sysconfig/irda ]; then
-
     case `module_id` in
 	"HP iPAQ H2200" | "HP iPAQ HX4700" | "HTC Universal")
 	    IRDA=yes
@@ -28,18 +27,10 @@  if [ ! -f /etc/sysconfig/irda ]; then
 	    DISCOVERY=
 	    ;;
     esac
-
-    mkdir -p /etc/sysconfig
-    echo "IRDA=$IRDA" > /etc/sysconfig/irda
-    if [ $IRDA = "yes" ]; then
-	echo "DEVICE=$DEVICE" >> /etc/sysconfig/irda
-	echo "DONGLE=$DONGLE" >> /etc/sysconfig/irda
-	echo "DISCOVERY=$DISCOVERY" >> /etc/sysconfig/irda
-    fi
+else
+    . /etc/sysconfig/irda
 fi
 
-. /etc/sysconfig/irda
-
 # Check that irda is up.
 [ ${IRDA} = "no" ] && exit 0