Take II: [master/rhel7-branch] Auto-load ks.cfg if OEMDRV volume available. (#1057271)

[Samantha N. Bueno]

This checks if there are any OEMDRV labelled storage volumes available--
if yes, load a ks.cfg file from that.

This should only happen if a user has not manually specified a ks=
option themselves.

Additionally, this checks for a boot arg inst.ks=none, to explicitly
disable autoloading of ks.cfg from any detected OEMDRV volumes.

Resolves: rhbz#1057271
---
 dracut/kickstart-genrules.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/dracut/kickstart-genrules.sh b/dracut/kickstart-genrules.sh
index 3b83d6c..b5823f9 100755
--- a/dracut/kickstart-genrules.sh
+++ b/dracut/kickstart-genrules.sh
@@ -24,4 +24,10 @@ case "${kickstart%%:*}" in
     bd) # bd:<dev>:<path> - biospart (TODO... if anyone uses this anymore)
         warn "inst.ks: can't get kickstart - biospart (bd:) isn't supported yet"
     ;;
+    "")
+        if [ -z "$kickstart" -a $(getarg ks= inst.ks=) != "none" ]; then
+            when_diskdev_appears $(disk_to_dev_path LABEL=OEMDRV) \
+                fetch-kickstart-disk $env{DEVNAME} "/ks.cfg"
+        fi
+    ;;
 esac

[Vratislav Podzimek]

On Wed, 2015-07-22 at 15:36 -0400, Samantha N. Bueno wrote:
> This checks if there are any OEMDRV labelled storage volumes available--
> if yes, load a ks.cfg file from that.
>
> This should only happen if a user has not manually specified a ks=
> option themselves.
>
> Additionally, this checks for a boot arg inst.ks=none, to explicitly
> disable autoloading of ks.cfg from any detected OEMDRV volumes.
> Resolves: rhbz#1057271
>
> dracut/kickstart-genrules.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/dracut/kickstart-genrules.sh b/dracut/kickstart-genrules.sh
> index 3b83d6c..b5823f9 100755
> --- a/dracut/kickstart-genrules.sh
> +++ b/dracut/kickstart-genrules.sh
> @@ -24,4 +24,10 @@ case "${kickstart%%:*}" in
>      bd) # bd:<dev>:<path> - biospart (TODO... if anyone uses this anymore)
>          warn "inst.ks: can't get kickstart - biospart (bd:) isn't supported 
> yet"
>      ;;
> "")
>    if [ -z "$kickstart" -a $(getarg ks= inst.ks=) != "none" ]; then
>        when_diskdev_appears $(disk_to_dev_path LABEL=OEMDRV) \
>            fetch-kickstart-disk \$env{DEVNAME} "/ks.cfg"
>    fi
> ;;
>
> esac

Looks good to me.

[Adam Williamson]

On Wed, 2015-07-22 at 15:36 -0400, Samantha N. Bueno wrote:
> This checks if there are any OEMDRV labelled storage volumes 
> available--
> if yes, load a ks.cfg file from that.
>
> This should only happen if a user has not manually specified a ks=
> option themselves.
>
> Additionally, this checks for a boot arg inst.ks=none, to explicitly
> disable autoloading of ks.cfg from any detected OEMDRV volumes.

I think there's an error in this, possibly when there is no 'ks=' or
'inst.ks=' at all; when you boot with an updates.img at present there
seems to be a delay to the boot process, and you can see this error
while it's stuck:

//lib/dracut/hooks/pre-trigger/50-kickstart-genrules.sh: line 34: [:
too many arguments

which is the test that was added by this commit. There aren't any
obvious consequences, but it still looks like a bug.

The error does still pop up, I think, when you boot without
inst.updates - it's just not visible for very long, so easy to miss. I
don't know what the cause of the boot delay when booting with
inst.updates is, it just seems to sit there doing nothing for a while
before deciding to go ahead and download the updates image.

[Vratislav Podzimek]

On Wed, 2015-07-22 at 15:36 -0400, Samantha N. Bueno wrote:
> This checks if there are any OEMDRV labelled storage volumes available--
> if yes, load a ks.cfg file from that.
>
> This should only happen if a user has not manually specified a ks=
> option themselves.
>
> Additionally, this checks for a boot arg inst.ks=none, to explicitly
> disable autoloading of ks.cfg from any detected OEMDRV volumes.
> Resolves: rhbz#1057271
>
> dracut/kickstart-genrules.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/dracut/kickstart-genrules.sh b/dracut/kickstart-genrules.sh
> index 3b83d6c..b5823f9 100755
> --- a/dracut/kickstart-genrules.sh
> +++ b/dracut/kickstart-genrules.sh
> @@ -24,4 +24,10 @@ case "${kickstart%%:*}" in
>      bd) # bd:<dev>:<path> - biospart (TODO... if anyone uses this anymore)
>          warn "inst.ks: can't get kickstart - biospart (bd:) isn't supported yet"
>      ;;
> "")
>    if [ -z "$kickstart" -a $(getarg ks= inst.ks=) != "none" ]; then

Per Adam's report, the second part of this condition should probably
look like:

x$(getarg ks= inst.ks=) != "xnone"

but I thought that getarg handles that and returns "none" exactly in
those cases that could cause such issues. If it returns an empty string,
this could be reduced to:

if [ -z "$kickstart" -a -z "$(getarg ks= inst.ks=)"]; then

[Samantha N. Bueno]

On Wed, Jul 29, 2015 at 09:18:37AM +0200, Vratislav Podzimek wrote:
> On Wed, 2015-07-22 at 15:36 -0400, Samantha N. Bueno wrote:
> > This checks if there are any OEMDRV labelled storage volumes available--
> > if yes, load a ks.cfg file from that.
> >
> > This should only happen if a user has not manually specified a ks=
> > option themselves.
> >
> > Additionally, this checks for a boot arg inst.ks=none, to explicitly
> > disable autoloading of ks.cfg from any detected OEMDRV volumes.
> > Resolves: rhbz#1057271
> >
> > dracut/kickstart-genrules.sh | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/dracut/kickstart-genrules.sh b/dracut/kickstart-genrules.sh
> > index 3b83d6c..b5823f9 100755
> > --- a/dracut/kickstart-genrules.sh
> > +++ b/dracut/kickstart-genrules.sh
> > @@ -24,4 +24,10 @@ case "${kickstart%%:*}" in
> >      bd) # bd:<dev>:<path> - biospart (TODO... if anyone uses this anymore)
> >          warn "inst.ks: can't get kickstart - biospart (bd:) isn't supported yet"
> >      ;;
> > "")
> >    if [ -z "$kickstart" -a $(getarg ks= inst.ks=) != "none" ]; then
>
> Per Adam's report, the second part of this condition should probably
> look like:
>
> x$(getarg ks= inst.ks=) != "xnone"
>
> but I thought that getarg handles that and returns "none" exactly in
> those cases that could cause such issues. If it returns an empty string,
> this could be reduced to:

Right, I don't think getarg returns "none", just an empty string.
> if [ -z "$kickstart" -a -z "$(getarg ks= inst.ks=)"]; then

Ok. Well it's too late and I already pushed this to master and
rhel7-branch, but I can push the above one-liner to both as a follow-up.
I'll post it as a new patch for review, so that it's clear to everyone.

Thanks for the catch, dudes.

Samantha