[PATCH] update-rc.d.bbclass: check that init script exists before running it

[PATCH] update-rc.d.bbclass: check that init script exists before running it

[Markus Lehtonen]

Check that the init script that is going to be called in the prerm()
script really exists. There might be a packaging bug or the script
might've been removed already earlier in prerm().

[YOCTO #10299]

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
---
 meta/classes/update-rc.d.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/update-rc.d.bbclass b/meta/classes/update-rc.d.bbclass
index dfef2a2..24da547 100644
--- a/meta/classes/update-rc.d.bbclass
+++ b/meta/classes/update-rc.d.bbclass
@@ -37,7 +37,7 @@ fi
 }
 
 updatercd_prerm() {
-if [ -z "$D" ]; then
+if [ -z "$D" -a -f "${INIT_D_DIR}/${INITSCRIPT_NAME}" ]; then
 	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
 fi
 }
-- 
2.6.6

Re: [PATCH] update-rc.d.bbclass: check that init script exists before running it

[Andreas Oberritter]

Hello Markus,

On 05.10.2016 16:11, Markus Lehtonen wrote:
> Check that the init script that is going to be called in the prerm()
> script really exists. There might be a packaging bug or the script
> might've been removed already earlier in prerm().

isn't it called prerm in the first place because it's not supposed to
remove any packaged files?

And if there's a packaging bug, we should IMO better add a sanity check
there and abort the build.

Regards,
Andreas



> 
> [YOCTO #10299]
> 
> Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
> ---
>  meta/classes/update-rc.d.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/update-rc.d.bbclass b/meta/classes/update-rc.d.bbclass
> index dfef2a2..24da547 100644
> --- a/meta/classes/update-rc.d.bbclass
> +++ b/meta/classes/update-rc.d.bbclass
> @@ -37,7 +37,7 @@ fi
>  }
>  
>  updatercd_prerm() {
> -if [ -z "$D" ]; then
> +if [ -z "$D" -a -f "${INIT_D_DIR}/${INITSCRIPT_NAME}" ]; then
>  	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
>  fi
>  }
> 

Re: [PATCH] update-rc.d.bbclass: check that init script exists before running it

[Markus Lehtonen]

On Wed, 2016-10-05 at 16:51 +0200, Andreas Oberritter wrote:
> Hello Markus,
> 
> On 05.10.2016 16:11, Markus Lehtonen wrote:
> > Check that the init script that is going to be called in the prerm()
> > script really exists. There might be a packaging bug or the script
> > might've been removed already earlier in prerm().
> 
> isn't it called prerm in the first place because it's not supposed to
> remove any packaged files?

In the case of this bug it does not remove any packaged files. Update
-alternatives removes a symlink (created by itself)


> And if there's a packaging bug, we should IMO better add a sanity check
> there and abort the build.

First of all, this does not fix a build-time, but a run-time problem. And,
I think that the pre post etc scripts should basically never fail.


[YOCTO #10299]
> > 
> > Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>
> > ---
> >  meta/classes/update-rc.d.bbclass | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meta/classes/update-rc.d.bbclass b/meta/classes/update
> > -rc.d.bbclass
> > index dfef2a2..24da547 100644
> > --- a/meta/classes/update-rc.d.bbclass
> > +++ b/meta/classes/update-rc.d.bbclass
> > @@ -37,7 +37,7 @@ fi
> >  }
> >  
> >  updatercd_prerm() {
> > -if [ -z "$D" ]; then
> > +if [ -z "$D" -a -f "${INIT_D_DIR}/${INITSCRIPT_NAME}" ]; then
> >  	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> >  fi
> >  }
> > 
> 

Re: [PATCH] update-rc.d.bbclass: check that init script exists before running it

[Phil Blundell]

On Thu, 2016-10-06 at 11:49 +0300, Markus Lehtonen wrote:
> On Wed, 2016-10-05 at 16:51 +0200, Andreas Oberritter wrote:
> > Hello Markus,
> > 
> > On 05.10.2016 16:11, Markus Lehtonen wrote:
> > > Check that the init script that is going to be called in the
> > > prerm()
> > > script really exists. There might be a packaging bug or the
> > > script
> > > might've been removed already earlier in prerm().
> > 
> > isn't it called prerm in the first place because it's not supposed
> > to
> > remove any packaged files?
> 
> In the case of this bug it does not remove any packaged files. Update
> -alternatives removes a symlink (created by itself)

That arguably is a bug in u-a, which probably ought to be removing the
symlink in postrm not prerm.  If it removes the symlink too early and
prevents u-rc.d from running "stop" on it then you may end up with
dangling daemon processes still running after the package has been
uninstalled.  But...

> I think that the pre post etc scripts should basically never fail.

... this is essentially true, and having u-rc.d's own prerm fail
because of a bug in u-a definitely isn't going to improve the
situation.  So I think your patch is a good one.

p.

> 

Re: [PATCH] update-rc.d.bbclass: check that init script exists before running it

[Markus Lehtonen]

On Thu, 2016-10-06 at 11:12 +0100, Phil Blundell wrote:
> On Thu, 2016-10-06 at 11:49 +0300, Markus Lehtonen wrote:
> > On Wed, 2016-10-05 at 16:51 +0200, Andreas Oberritter wrote:
> > > Hello Markus,
> > > 
> > > On 05.10.2016 16:11, Markus Lehtonen wrote:
> > > > Check that the init script that is going to be called in the
> > > > prerm()
> > > > script really exists. There might be a packaging bug or the
> > > > script
> > > > might've been removed already earlier in prerm().
> > > 
> > > isn't it called prerm in the first place because it's not supposed
> > > to
> > > remove any packaged files?
> > 
> > In the case of this bug it does not remove any packaged files. Update
> > -alternatives removes a symlink (created by itself)
> 
> That arguably is a bug in u-a, which probably ought to be removing the
> symlink in postrm not prerm.  

It was moved to prerm earlier to fix some other problems:
http://git.openembedded.org/openembedded-core/commit/meta/classes/update-al
ternatives.bbclass?id=2a5484a90513b58c829a916bfe5268a0fde3512a

So I think moving it back and forth between prerm and postrm doesn't get us
anywhere :)


Thanks,
  Markus



> If it removes the symlink too early and
> prevents u-rc.d from running "stop" on it then you may end up with
> dangling daemon processes still running after the package has been
> uninstalled.  But...
> 
> > I think that the pre post etc scripts should basically never fail.
> 
> ... this is essentially true, and having u-rc.d's own prerm fail
> because of a bug in u-a definitely isn't going to improve the
> situation.  So I think your patch is a good one.
> 
> p.
> 
> > 

Re: [PATCH] update-rc.d.bbclass: check that init script exists before running it

[Phil Blundell]

On Thu, 2016-10-06 at 16:46 +0300, Markus Lehtonen wrote:
> 
It was moved to prerm earlier to fix some other problems:
> http://git.openembedded.org/openembedded-core/commit/meta/classes/upd
> ate-al
> ternatives.bbclass?id=2a5484a90513b58c829a916bfe5268a0fde3512a

The commit message for that checkin is a bit vague about exactly what
problem it was solving.  It mentions that it "fixes deinstalling
alternatives for programs needed by the postrm script, e.g. /bin/sh"
but it's rather hard to see how that actually works.  If postrm
requires /bin/sh and prerm has deleted it then it's not obvious how
this is an improvement.

> So I think moving it back and forth between prerm and postrm doesn't
> get us
> anywhere :)

That is a fair point, though I am not entirely convinced that the
commit you mentioned above is the correct solution to whatever problem
it thinks it's solving.

As it stands, with both things in prerm, you will get essentially
random behaviour depending on the order in which the two classes (u-
rc.d and u-a) are inherited by the recipe.  If u-a comes first then u-
rc.d will not be able to run the "stop" script (and the services will
not be stopped on package uninstallation), and vice versa.

So maybe these two classes just need to be taught to play together
better.

p.

Re: [PATCH] update-rc.d.bbclass: check that init script exists before running it

[Andreas Oberritter]

On 06.10.2016 17:12, Phil Blundell wrote:
> On Thu, 2016-10-06 at 16:46 +0300, Markus Lehtonen wrote:
>>
> It was moved to prerm earlier to fix some other problems:
>> http://git.openembedded.org/openembedded-core/commit/meta/classes/upd
>> ate-al
>> ternatives.bbclass?id=2a5484a90513b58c829a916bfe5268a0fde3512a
> 
> The commit message for that checkin is a bit vague about exactly what
> problem it was solving.  It mentions that it "fixes deinstalling
> alternatives for programs needed by the postrm script, e.g. /bin/sh"
> but it's rather hard to see how that actually works.  If postrm
> requires /bin/sh and prerm has deleted it then it's not obvious how
> this is an improvement.

Sorry for the bad commit message back then.

If you had both busybox and bash installed and had /bin/sh pointing to
bash, then uninstalling bash failed, because the postrm script was
started with a dangling symlink of /bin/sh.

The commit cited above fixes it, because when the prerm script finishes,
/bin/sh will point to busybox, thus letting the postrm script of bash
succeed.

> 
>> So I think moving it back and forth between prerm and postrm doesn't
>> get us
>> anywhere :)
> 
> That is a fair point, though I am not entirely convinced that the
> commit you mentioned above is the correct solution to whatever problem
> it thinks it's solving.
> 
> As it stands, with both things in prerm, you will get essentially
> random behaviour depending on the order in which the two classes (u-
> rc.d and u-a) are inherited by the recipe.  If u-a comes first then u-
> rc.d will not be able to run the "stop" script (and the services will
> not be stopped on package uninstallation), and vice versa.
> 
> So maybe these two classes just need to be taught to play together
> better.

I think having update-alternatives manage an initscript is already a bug
on its own. I haven't seen such a thing in debian or anywhere else
before. Update-alternatives usually creates symlinks for programs which
can be installed side-by-side. If two system services provide the same
functionality, I guess the packages should better conflict with each
other or at least have different initscript names (like apache2 and
lighttpd for example), so they can both be started with adjusted
configurations (e.g. using different tcp ports).

Regards,
Andreas