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

[Will Woods]

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
+
+def read_cmdline(f):
+    args = OrderedDict()
+    for arg in readfile(f).split():
+        k,e,v = arg.partition("=")
+        args[k] = v
+    return args
+
+proc_cmdline = read_cmdline("/proc/cmdline")
+
 # Here are the kickstart commands we care about:

class Method(commands.method.F14_Method):
@@ -79,7 +96,7 @@ class Network(commands.network.F16_Network):
         # write ifcfg for all listed devices
         ksnet_to_ifcfg(net)
         # anaconda tradition: bring up the first device listed, and no others
-        if len(self.network) == 1:
+        if len(self.network) == 1 and "BOOTIF" not in proc_cmdline:
             netline = ksnet_to_dracut(args, lineno, net, bootdev=True)
             return netline

@@ -215,13 +232,6 @@ def ksnet_to_dracut(args, lineno, net, bootdev=False):

return " ".join(line)

-def readfile(f):
-    try:
-        val = open(f).readline().strip()
-    except IOError:
-        val = None
-    return val
-
 def ksnet_to_ifcfg(net, filename=None):
     '''Write an ifcfg file for the given kickstart network config'''
     dev = net.device

[PATCH 2/2] parse-kickstart: improve logging

[Will Woods]

Emit logging messages when 'rd.debug' or 'rd.info' are passed.
---
 dracut/parse-kickstart | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/dracut/parse-kickstart b/dracut/parse-kickstart
index d81dc72..8cd9394 100755
--- a/dracut/parse-kickstart
+++ b/dracut/parse-kickstart
@@ -125,7 +125,6 @@ class DracutHandler(handlerclass):
         command = self.commands[cmd]
         if hasattr(command, "dracut_args"):
             log.debug("kickstart line %u: handling %s", lineno, cmd)
-            line = " ".join(args)
             self.output.append(command.dracut_args(args, lineno, obj))
         return obj

@@ -137,12 +136,16 @@ class KmsgFormatter(logging.Formatter):
         elif record.levelno <= logging.WARNING: tag = "<28>"
         else: tag = "<24>"
         return tag + logging.Formatter.format(self, record)
-def init_logger():
+def init_logger(level=logging.WARNING):
+    if 'rd.debug' in proc_cmdline:
+        level = logging.DEBUG
+    elif 'rd.info' in proc_cmdline:
+        level = logging.INFO
     logfmt = "%(name)s %(levelname)s: %(message)s"
     stderr = logging.StreamHandler()
     stderr.setFormatter(logging.Formatter(logfmt))
     logger = logging.getLogger('parse-kickstart')
-    logger.setLevel(logging.WARNING)
+    logger.setLevel(level)
     logger.addHandler(stderr)
     try:
         kmsg = logging.FileHandler("/dev/kmsg", "w")
@@ -280,6 +283,7 @@ def ksnet_to_ifcfg(net, filename=None):
     # TODO: handle essid/wepkey/wpakey (maybe inside anaconda)

try:
+        log.info("writing ifcfg for %s", dev)
         outf = open(filename, "w")
         outf.write('# Generated by parse-kickstart\n')
         for k,v in ifcfg.items():
@@ -294,6 +298,7 @@ def process_kickstart(ksfile):
     handler = DracutHandler()
     handler.ksdevice = os.environ.get('ksdevice')
     parser = KickstartParser(handler)
+    log.info("processing kickstart file %s", ksfile)
     processed_file = preprocessKickstart(ksfile)
     try:
         parser.readKickstart(processed_file)
@@ -301,6 +306,7 @@ def process_kickstart(ksfile):
         log.error(str(e))
     with open("/tmp/ks.info", "a") as f:
         f.write('parsed_kickstart="%s"\n' % processed_file)
+    log.info("finished parsing kickstart")
     return processed_file, handler.output

if __name__ == '__main__':

[Brian C. Lane]

Ack to both of these

[Vratislav Podzimek]

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. :)

[Will Woods]

On Tue, 2012-07-31 at 10:08 +0200, Vratislav Podzimek wrote:
> I don't like this patch for two reasons:
> 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.

No, your understanding is correct. This is another place where current
upstream behavior collides with Anaconda Tradition:

Anaconda Tradition:
  If 'ksdevice=bootif' is used, use that device for kickstart.
  Otherwise, ignore BOOTIF=.

Dracut Behavior:
  Use the device specified by BOOTIF to set up the network via dhcp.
  Ignore any other ip= lines, and crash if they're found.

Probably the right thing to do is to *default* to doing dhcp if BOOTIF
is found, but let kickstart 'network' commands and other 'ip' options
override it.

Dracut says:

# Don't mix BOOTIF=macaddr from pxelinux and ip= lines
  getarg ip= >/dev/null && getarg BOOTIF= >/dev/null && \
      die "Mixing BOOTIF and ip= lines is dangerous"

But I have no idea why this would be "dangerous". If I can't find a good
justification then we should patch dracut so we can tell it to ignore
BOOTIF if we've got kickstart config we want to use.
> 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.

..oddly, the same file also has:

try:
            thismac = readfile("/sys/class/net/%s/address" % netif)
        except IOError:
            pass

So obviously I got mixed up somewhere.

The point of readfile() was that it should return the contents of the
file, or nothing if the file is missing/empty.

Sort of like "$(cat $file 2>/dev/null)" - we don't care if the file is
missing or not.

So I'm changing it to reflect that.

New patches will be along in a bit.

-w

[Radek Vykydal]

On 07/31/2012 12:30 AM, 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.

Being at this, could you explain how the ip= is used then? From looking at
dracut code I think in run_kickstart follwing parse-kickstart network
udev events are re-triggered which calls ifup.sh using new ip= lines
from kickstart? Does it restart an already activated device with new 
(from ks)
configuration?

As Vratislav and you, I think BOOTIF shouldn't override
ks network configuration of a device. If anything, it should
be used to identify device specified as:
network --device=bootif

Thanks,

Radek