[OE-core] [PATCH v2 1/2] classes/uboot-extlinux-config: Add class

[Ulf Magnusson]

Hello,

On Mon, Oct 3, 2016 at 2:17 PM, Fabio Berton
<fabio.berton at ossystems.com.br> wrote:
> This class allow the extlinux.conf generation for U-Boot use.
> The U-Boot support for it is given to allow the Generic Distribution
> Configuration specification use by OpenEmbedded-based products.
>
> This class can be inherited by u-boot recipes to create extlinux.conf
> and boot using menu options.
>
> U-boot with extlinux support is machine dependent, so to use this class
> you need to set UBOOT_EXTLINUX to 1 in machine configuration file and
> also set root= kernel cmdline UBOOT_EXTLINUX_ROOT. This variable is used
> to pass root kernel cmdline, e.g:
>
> UBOOT_EXTLINUX_ROOT = "root=/dev/mmcblk2p2"
>
> Signed-off-by: Fabio Berton <fabio.berton at ossystems.com.br>
> Signed-off-by: Otavio Salvador <otavio at ossystems.com.br>
> ---
>  meta/classes/uboot-extlinux-config.bbclass | 130 +++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
>  create mode 100644 meta/classes/uboot-extlinux-config.bbclass
>
> diff --git a/meta/classes/uboot-extlinux-config.bbclass b/meta/classes/uboot-extlinux-config.bbclass
> new file mode 100644
> index 0000000..a4dc0c0
> --- /dev/null
> +++ b/meta/classes/uboot-extlinux-config.bbclass
> @@ -0,0 +1,130 @@
> +# uboot-extlinux-config.bbclass
> +#
> +# This class allow the extlinux.conf generation for U-Boot use. The
> +# U-Boot support for it is given to allow the Generic Distribution
> +# Configuration specification use by OpenEmbedded-based products.
> +#
> +# External variables:
> +#
> +# UBOOT_EXTLINUX_CONFIG            - Configuration file default set to source dir.
> +# UBOOT_EXTLINUX_CONSOLE           - Set to "console=ttyX" to change kernel boot
> +#                                    default console.
> +# UBOOT_EXTLINUX_LABELS            - A list of targets for the automatic config.
> +# UBOOT_EXTLINUX_KERNEL_ARGS       - Add additional kernel arguments.
> +# UBOOT_EXTLINUX_KERNEL_IMAGE      - Kernel image name.
> +# UBOOT_EXTLINUX_FDTDIR            - Device tree directory.
> +# UBOOT_EXTLINUX_INITRD            - Indicates a list of filesystem images to
> +#                                    concatenate and use as an initrd (optional).
> +# UBOOT_EXTLINUX_MENU_DESCRIPTION  - Name to use as description.
> +# UBOOT_EXTLINUX_ROOT              - Root kernel cmdline.
> +#
> +# If there's only one label system will boot automatically and menu won't be
> +# created. If you want to use more than one labels, e.g linux and alternate,
> +# use overrides to set menu description, console and others variables.
> +#
> +# Ex:
> +#
> +# UBOOT_EXTLINUX_LABELS ??= "default fallback"
> +#
> +# UBOOT_EXTLINUX_KERNEL_IMAGE_default ??= "../zImage"
> +# UBOOT_EXTLINUX_MENU_DESCRIPTION_default ??= "Linux Default"
> +#
> +# UBOOT_EXTLINUX_KERNEL_IMAGE_fallback ??= "../zImage-fallback"
> +# UBOOT_EXTLINUX_MENU_DESCRIPTION_fallback ??= "Linux Fallback"
> +#
> +# Results:
> +#
> +# menu title Select the boot mode
> +# LABEL Linux Default
> +#   KERNEL ../zImage
> +#   FDTDIR ../
> +#   APPEND root=/dev/mmcblk2p2 rootwait rw console=${console}
> +# LABEL Linux Fallback
> +#   KERNEL ../zImage-fallback
> +#   FDTDIR ../
> +#   APPEND root=/dev/mmcblk2p2 rootwait rw console=${console}
> +#
> +# Copyright (C) 2016, O.S. Systems Software LTDA.  All Rights Reserved
> +# Released under the MIT license (see packages/COPYING)
> +#
> +# The kernel has an internal default console, which you can override with
> +# a console=...some_tty...
> +UBOOT_EXTLINUX_CONSOLE ??= "console=${console}"
> +UBOOT_EXTLINUX_CONFIG ??= "${S}/extlinux.conf"
> +UBOOT_EXTLINUX_LABELS ??= "linux"
> +UBOOT_EXTLINUX_FDTDIR ??= "../"
> +UBOOT_EXTLINUX_KERNEL_IMAGE ??= "../${KERNEL_IMAGETYPE}"
> +UBOOT_EXTLINUX_KERNEL_ARGS ??= "rootwait rw"
> +UBOOT_EXTLINUX_MENU_DESCRIPTION_linux ??= "${DISTRO_NAME}"
> +
> +python create_extlinux_config() {
> +    if d.getVar("UBOOT_EXTLINUX", False) == "1":

Is there a reason why False is passed here? Passing True would make
the code more general, as the value of the flag could come from
another variable, e.g.

  UBOOT_EXTLINUX = "${SOME_FLAG_HELPER_VAR}"

, or from a function, e.g.

  UBOOT_EXTLINUX = "${@needs_uboot_extlinux()}"

As a minor style note, the indentation of the function could be
decreased by doing

  if d.getVar("UBOOT_EXTLINUX", True) != "1":
      return
  ...

That might avoid excessive indentation if you use 'with' as mentioned below.

> +        workdir = d.getVar('WORKDIR', True)
> +        if not workdir:
> +            bb.error("WORKDIR not defined, unable to package")
> +            return

Could be simplified to the following to make it clear that 'workdir'
is never referenced afterwards (I know there's some existing code like
the above though):

  if not d.getVar('WORKDIR', True):
      ...

> +
> +        labels = d.getVar('UBOOT_EXTLINUX_LABELS', True)
> +        if not labels:
> +            bb.fatal(1, "UBOOT_EXTLINUX_LABELS not defined, nothing to do")
> +            return

'return' is redundant after bb.fatal(). bb.fatal() throws an exception
(BBHandledException, though that's internalish), so the 'return' will
never be reached.

> +
> +        if labels == []:
> +            bb.fatal(1, "No labels, nothing to do")
> +            return

Redundant 'return'.

> +
> +        cfile = d.getVar('UBOOT_EXTLINUX_CONFIG', True)
> +        if not cfile:
> +            bb.fatal('Unable to read UBOOT_EXTLINUX_CONFIG')
> +
> +        try:
> +            cfgfile = open(cfile, 'w')
> +        except OSError:
> +            bb.fatal('Unable to open %s' % (cfile))
> +
> +        cfgfile.write('# Generic Distro Configuration file generated by OpenEmbedded\n')
> +
> +        if len(labels.split()) > 1:
> +            cfgfile.write('menu title Select the boot mode\n')
> +
> +        for label in labels.split():
> +            localdata = bb.data.createCopy(d)
> +
> +            overrides = localdata.getVar('OVERRIDES', True)
> +            if not overrides:
> +                bb.fatal('OVERRIDES not defined')
> +
> +            localdata.setVar('OVERRIDES', label + ':' + overrides)
> +            bb.data.update_data(localdata)
> +
> +            extlinux_console = localdata.getVar('UBOOT_EXTLINUX_CONSOLE', True)
> +
> +            menu_description = localdata.getVar('UBOOT_EXTLINUX_MENU_DESCRIPTION', True)
> +            if not menu_description:
> +                menu_description = label
> +
> +            root = localdata.getVar('UBOOT_EXTLINUX_ROOT', True)
> +            if not root:
> +                bb.fatal('UBOOT_EXTLINUX_ROOT not defined')
> +
> +            kernel_image = localdata.getVar('UBOOT_EXTLINUX_KERNEL_IMAGE', True)
> +            fdtdir = localdata.getVar('UBOOT_EXTLINUX_FDTDIR', True)
> +            if fdtdir:
> +                cfgfile.write('LABEL %s\n\tKERNEL %s\n\tFDTDIR %s\n' %
> +                             (menu_description, kernel_image, fdtdir))
> +            else:
> +                cfgfile.write('LABEL %s\n\tKERNEL %s\n' % (menu_description, kernel_image))
> +
> +            kernel_args = localdata.getVar('UBOOT_EXTLINUX_KERNEL_ARGS', True)
> +
> +            initrd = localdata.getVar('UBOOT_EXTLINUX_INITRD', True)
> +            if initrd:
> +                cfgfile.write('\tINITRD %s\n'% initrd)
> +
> +            kernel_args = root + " " + kernel_args
> +            cfgfile.write('\tAPPEND %s %s\n' % (kernel_args, extlinux_console))
> +
> +        cfgfile.close()

Using Python's 'with' syntax would be a better way to handle the file.
That way, the file is guaranteed to be close()d immediately even if
the function exits via bb.fatal().

It is possible to use 'with' even after opening the file by the way.
Just do the following:

  with cfgfile:
      ...

> +}
> +
> +do_install[prefuncs] += "create_extlinux_config"
> --
> 2.1.4
>
> --

I'm not familiar with the functionality itself, so hard to say
anything constructive there.

Cheers,
Ulf