[PATCH 1/2] dracut: fix crash with PXE ipappend and ks=file:..

Vratislav Podzimek vpodzime at redhat.com
Tue Jul 31 08:08:42 UTC 2012


I don't like this patch for two reasons:

1) As I understand the (new) behaviour, using 'ipappend 2' will cause
that dracut will start the BOOTIF interface (for early networking)
instead of the device specified in the kickstart, right? I think this is
not the right behaviour, because for example our (Anaconda) PXE targets
here in Brno all contain 'ipappend 2' and it only make sense because it
is just an additional information for the installer. This does not mean
that we want to use BOOTIF interface for networking. The old behaviour
in loader was that this interface was used only when '--device=bootif'
was used in kickstart or as a default when no device was specified.
If my understanding is wrong, could you please explain the change? As I
see it, dracut should be patched to allow both "BOOTIF=..." and "ip=..."
options.

2) See comments below.

On Mon, 2012-07-30 at 18:30 -0400, Will Woods wrote:
> If you're using pxelinux with 'ipappend 2', it will add a "BOOTIF=..."
> argument to your boot args.
> 
> parse-kickstart will rewrite your first 'network' line to a "ip=..."
> argument for dracut.
> 
> But dracut refuses to allow "ip=..." and "BOOTIF=..." arguments to mix,
> so this makes it die with the message:
> 
>   FATAL: Mixing BOOTIF and ip= lines is dangerous
> 
> So: parse-kickstart should check the boot arguments, and not bother
> generating an "ip=..." line if "BOOTIF" is already there.
> ---
>  dracut/parse-kickstart | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/dracut/parse-kickstart b/dracut/parse-kickstart
> index 3b6b861..d81dc72 100755
> --- a/dracut/parse-kickstart
> +++ b/dracut/parse-kickstart
> @@ -23,6 +23,23 @@ from collections import OrderedDict
>  # Default logging: none
>  log = logging.getLogger('parse-kickstart').addHandler(logging.NullHandler())
>  
> +# So we can examine commandline args
> +def readfile(f):
> +    try:
> +        val = open(f).readline().strip()
> +    except IOError:
> +        val = None
> +    return val
Returning None is not a good way of an error indication, because it
results in "None type has no attribute..." tracebacks. You have to check
the returned value every time you use such function. Raising a
descriptive exception is much better way.

> +
> +def read_cmdline(f):
> +    args = OrderedDict()
> +    for arg in readfile(f).split():
And here comes the usage with no check. I don't expect there will ever
be a problem while reading "/proc/cmdline", but this function works with
any file, right?

> +        k,e,v = arg.partition("=")
> +        args[k] = v
> +    return args
> +
> +proc_cmdline = read_cmdline("/proc/cmdline")
Using exception instead of returning None would still allow omitting
try-except block here for the reason I've explained in the previous
comment. However, I don't see 3 additional lines as a tragedy. :)

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list