[PATCH] initial support for grubenv

[Maciej Pijanowski]

Signed-off-by: Maciej Pijanowski <maciej.p...@3mdeb.com>
---
Following patch provides initial support for grubenv modification in similar
fashion as for U-Boot. Environmental block handling is basically almost 1:1 ported
from GRUB source code. We've been successfully using it for a few months by now
with VirtualBox (GRUB2 EFI) and would like to attempt to upstream.

At the moment GRUB config option is added in handler section. Although it is
not really a handler - it can only set variables parsed from sw-description
file.

Possible fields of improvements:
* Error handling and printing (usage of swupdate macros?).
* Atomicity of grubenv modification. In previous messages we thought about
invoking mv system call. In GRUB source code, content of grubenv is read into
buffer and modified inside buffer. When buffer modification succeeds, then an
actual grubenv file gets written using fwrite. I guess this it not enough
fot swupdate requirements?
* Introduce bootloader abstraction - bootloader menu in menuconfig that allows
to pick GRUB / U-Boot etc.
* GRUB actual handler - setting multiple environmental variables using single
file as in case of U-Boot handler script - is this desired?
* Unsetting (deleting) variable from grubenv. At the moment this is not
implemented. In case of U-Boot, variable is being unset when no value is given.
In many grub.cfg configuration files, setting empty value to a variable
seems to be quite common practice. Implementing unsetting variables policy
as in case of U-Boot (no value = unset) may limit usage of grub.cfg. Maybe
use another keyword or mechanism for unset operation?


Makefile | 13 +-
corelib/installer.c | 33 +++++
examples/config/defconfig | 82 +++++++++++
grub/Makefile | 1 +
grub/grub_env.c | 344 ++++++++++++++++++++++++++++++++++++++++++++++
handlers/Config.in | 14 ++
include/grub_env.h | 46 +++++++
include/swupdate.h | 1 +
parser/parser.c | 50 +++++++
9 files changed, 583 insertions(+), 1 deletion(-)
create mode 100644 examples/config/defconfig
create mode 100644 grub/Makefile
create mode 100644 grub/grub_env.c
create mode 100644 include/grub_env.h

diff --git a/Makefile b/Makefile
index 307ae50d28aa..0f1c6caf6ad2 100644
--- a/Makefile
+++ b/Makefile
@@ -4,6 +4,10 @@ SUBLEVEL = 0
EXTRAVERSION =
NAME =

+INSTALL = install
+PREFIX ?= /usr/local
+BINDIR ?= $(PREFIX)/bin
+
# *DOCUMENTATION*
# To see a list of typical targets execute "make help"
# More info can be located in ./README
@@ -344,7 +348,7 @@ include $(srctree)/Makefile.flags
all: swupdate progress

objs-y := core handlers
-libs-y := archival corelib ipc mongoose parser suricatta
+libs-y := archival corelib ipc mongoose parser suricatta grub
client-y := progress_client

swupdate-dirs := $(objs-y) $(libs-y)
@@ -517,6 +521,13 @@ endif # skip-makefile
PHONY += FORCE
FORCE:

+# install
+#
+PHONY += install
+install:
+ $(INSTALL) -d ${BINDIR}/
+ $(INSTALL) -m 0755 swupdate ${BINDIR}/
+
# Documentation
# run Makefile in doc directory

diff --git a/corelib/installer.c b/corelib/installer.c
index fc5afb473758..266a76461840 100644
--- a/corelib/installer.c
+++ b/corelib/installer.c
@@ -44,6 +44,10 @@
#include "fw_env.h"
#include "progress.h"

+#ifdef CONFIG_GRUB
+#include "grub_env.h"
+#endif /* CONFIG_GRUB */
+
static int isImageInstalled(struct swver *sw_ver_list,
struct img_type *img)
{
@@ -208,6 +212,27 @@ static int update_uboot_env(void)
return ret;
}

+#ifdef CONFIG_GRUB
+static int update_grub_env(struct swupdate_cfg *cfg)
+{
+ int ret = 0;
+
+ struct dict_entry *grubvar;
+ TRACE("Updating GRUB environment");
+
+ LIST_FOREACH(grubvar, &cfg->grub, next) {
+ if (!grubvar->varname || !grubvar->value)
+ continue;
+ ret = grub_set_variable(grubvar->varname, grubvar->value);
+ if (ret < 0) {
+ ERROR("Error updating GRUB environment");
+ break;
+ }
+ }
+ return ret;
+}
+#endif /* CONFIG_GRUB */
+
int install_single_image(struct img_type *img)
{
struct installer_handler *hnd;
@@ -270,6 +295,14 @@ int install_images(struct swupdate_cfg *sw, int fdsw, int fromfile)
return ret;
}

+#ifdef CONFIG_GRUB
+ /* Update GRUB environment */
+ ret = update_grub_env(sw);
+ if (ret) {
+ return ret;
+ }
+#endif /* CONFIG_GRUB */
+
LIST_FOREACH(img, &sw->images, next) {

/*
diff --git a/examples/config/defconfig b/examples/config/defconfig
new file mode 100644
index 000000000000..a330c8a3f9dd
--- /dev/null
+++ b/examples/config/defconfig
@@ -0,0 +1,82 @@
+#
+# Automatically generated file; DO NOT EDIT.
+# Swupdate Configuration
+#
+CONFIG_HAVE_DOT_CONFIG=y
+
+#
+# Swupdate Settings
+#
+
+#
+# General Configuration
+#
+CONFIG_SCRIPTS=y
+# CONFIG_HW_COMPATIBILITY is not set
+CONFIG_SW_VERSIONS_FILE="/etc/sw-versions"
+# CONFIG_MTD is not set
+CONFIG_LUA=y
+CONFIG_LUAPKG="lua50"
+# CONFIG_FEATURE_SYSLOG is not set
+
+#
+# Build Options
+#
+# CONFIG_STATIC is not set
+CONFIG_CROSS_COMPILE=""
+CONFIG_SYSROOT=""
+CONFIG_EXTRA_CFLAGS=""
+CONFIG_EXTRA_LDFLAGS=""
+CONFIG_EXTRA_LDLIBS=""
+
+#
+# Debugging Options
+#
+# CONFIG_DEBUG is not set
+# CONFIG_WERROR is not set
+# CONFIG_NOCLEANUP is not set
+CONFIG_DOWNLOAD=y
+# CONFIG_HASH_VERIFY is not set
+# CONFIG_SIGNED_IMAGES is not set
+# CONFIG_ENCRYPTED_IMAGES is not set
+# CONFIG_SURICATTA is not set
+
+#
+# Suricatta
+#
+
+#
+# Server
+#
+CONFIG_SURICATTA_SERVER_NONE=y
+
+#
+# hawkBit support needs libcurl and CONFIG_JSON=y
+#
+# CONFIG_WEBSERVER is not set
+
+#
+# Archival Features
+#
+CONFIG_GUNZIP=y
+
+#
+# Parser Features
+#
+CONFIG_LIBCONFIG=y
+CONFIG_LIBCONFIGROOT=""
+# CONFIG_JSON is not set
+# CONFIG_LUAEXTERNAL is not set
+# CONFIG_SETSWDESCRIPTION is not set
+
+#
+# Image Handlers
+#
+CONFIG_RAW=y
+# CONFIG_LUASCRIPTHANDLER is not set
+CONFIG_SHELLSCRIPTHANDLER=y
+CONFIG_ARCHIVE=y
+# CONFIG_REMOTE_HANDLER is not set
+# CONFIG_UBOOT is not set
+CONFIG_GRUB=y
+CONFIG_GRUB_ENV="/boot/efi/EFI/BOOT/grub/grubenv"
diff --git a/grub/Makefile b/grub/Makefile
new file mode 100644
index 000000000000..5018fd093001
--- /dev/null
+++ b/grub/Makefile
@@ -0,0 +1 @@
+lib-$(CONFIG_GRUB) += grub_env.o
diff --git a/grub/grub_env.c b/grub/grub_env.c
new file mode 100644
index 000000000000..38570e47071c
--- /dev/null
+++ b/grub/grub_env.c
@@ -0,0 +1,344 @@
+/* grub-editenv.c - tool to edit environment block. */
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2008,2009,2010 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "grub_env.h"
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+/* internal functions prototypes */
+static grub_envblk_t grub_envblk_open(char *buf, size_t size);
+static int grub_envblk_set(grub_envblk_t envblk, char *name, char *value);
+/* unsetting variable is not used at the moment */
+static int grub_envblk_delete(grub_envblk_t envblk, char *name);
+static void grub_envblk_close(grub_envblk_t envblk);
+
+static grub_envblk_t grub_open_envblk_file(void);
+static int grub_write_envblk(grub_envblk_t envblk);
+
+static grub_envblk_t
+grub_envblk_open(char *buf, size_t size)
+{
+ grub_envblk_t envblk;
+
+ if (size < sizeof(GRUB_ENVBLK_SIGNATURE)
+ || memcmp(buf, GRUB_ENVBLK_SIGNATURE,
+ sizeof(GRUB_ENVBLK_SIGNATURE) - 1)) {
+ fprintf(stderr, "GRUB: invalid environment block\n");
+ return NULL;
+ }
+
+ envblk = malloc(sizeof(*envblk));
+ if (envblk) {
+ envblk->buf = buf;
+ envblk->size = size;
+ }
+
+ return envblk;
+}
+
+static void
+grub_envblk_close(grub_envblk_t envblk)
+{
+ free(envblk->buf);
+ free(envblk);
+}
+
+static int
+escaped_value_len(const char *value)
+{
+ int n = 0;
+ char *p;
+
+ for (p = (char *)value; *p; p++) {
+ if (*p == '\\' || *p == '\n')
+ n += 2;
+ else
+ n++;
+ }
+
+ return n;
+}
+
+static char
+*find_next_line(char *p, const char *pend)
+{
+ while (p < pend) {
+ if (*p == '\\')
+ p += 2;
+ else if (*p == '\n')
+ break;
+ else
+ p++;
+ }
+
+ return p + 1;
+}
+
+static int
+grub_envblk_set(grub_envblk_t envblk, char *name, char *value)
+{
+ char *p, *pend;
+ char *space; // free space (chars) left in grubenv
+ int found = 0;
+ int nl; // length of name
+ int vl; // length of value
+ int i;
+
+ nl = strlen(name);
+ vl = escaped_value_len(value);
+ /*set pointer at the begging of the first variable name */
+ p = envblk->buf + sizeof(GRUB_ENVBLK_SIGNATURE) - 1;
+ pend = envblk->buf + envblk->size;
+
+ /* First, look at free space. Empty byte is saved as '#' */
+ for (space = pend - 1; *space == '#'; space--);
+
+ if (*space != '\n')
+ /* Broken. */
+ return -1;
+
+ space++;
+
+ while (p + nl + 1 < space) {
+ if (memcmp(p, name, nl) == 0 && p[nl] == '=') {
+ int len;
+
+ /* Found the same name. */
+ /* set pointer at corresponding value */
+ p += nl + 1;
+
+ /* Check the length of the current value. */
+ len = 0;
+ while (p + len < pend && p[len] != '\n') {
+ if (p[len] == '\\')
+ len += 2;
+ else
+ len++;
+ }
+
+ if (p + len >= pend)
+ /* Broken. */
+ return -1;
+
+ if (pend - space < vl - len)
+ /* No space. */
+ return -1;
+
+ if (vl < len) {
+ /* Move the following characters backward, and fill the new
+ space with harmless characters. */
+ memmove(p + vl, p + len, pend - (p + len));
+ memset(space + len - vl, '#', len - vl);
+ } else
+ /* Move the following characters forward. */
+ memmove(p + vl, p + len, pend - (p + vl));
+
+ found = 1;
+ break;
+ }
+
+ p = find_next_line(p, pend);
+ }
+
+ if (!found) {
+ /* Append a new variable. */
+
+ if (pend - space < nl + 1 + vl + 1)
+ /* No space. */
+ return -1;
+
+ memcpy(space, name, nl);
+ p = space + nl;
+ *p++ = '=';
+ }
+
+ /* Write the value. */
+ for (i = 0; value[i]; i++) {
+ if (value[i] == '\\' || value[i] == '\n')
+ *p++ = '\\';
+
+ *p++ = value[i];
+ }
+
+ *p = '\n';
+ return 0;
+}
+
+/* unsetting variable is not used at the moment */
+static int
+grub_envblk_delete(grub_envblk_t envblk, char *name)
+{
+ char *p, *pend;
+ int nl; /* length of name */
+
+ nl = strlen(name);
+ p = envblk->buf + sizeof(GRUB_ENVBLK_SIGNATURE) - 1;
+ pend = envblk->buf + envblk->size;
+
+ while (p + nl + 1 < pend) {
+ if (memcmp(p, name, nl) == 0 && p[nl] == '=') {
+ /* Found */
+ int len = nl + 1;
+
+ while (p + len < pend) {
+ if (p[len] == '\n')
+ break;
+ else if (p[len] == '\\')
+ len += 2;
+ else
+ len++;
+ }
+
+ if (p + len >= pend)
+ /* Broken. */
+ return -1;
+
+ len++;
+ memmove(p, p + len, pend - (p + len));
+ memset(pend - len, '#', len);
+ break;
+ }
+
+ p = find_next_line(p, pend);
+ }
+ return 0;
+}
+
+static grub_envblk_t
+grub_open_envblk_file(void)
+{
+ FILE *fp;
+ char *buf;
+ size_t size;
+ grub_envblk_t envblk;
+
+ fp = fopen(GRUB_ENVBLK_PATH, "rb");
+ if (!fp) {
+ /* should we create env file if it's missing ?
+ *
+ * grub_create_envblk_file(name);
+ * fp = grub_util_fopen (name, "rb");
+ */
+ fprintf(stderr, "GRUB: %s file is missing\n", GRUB_ENVBLK_PATH);
+ return NULL;
+ }
+
+ if (fseek(fp, 0, SEEK_END) < 0) {
+ fprintf(stderr, "GRUB: cannot seek %s\n", GRUB_ENVBLK_PATH);
+ return NULL;
+ }
+
+ size = (size_t)ftell(fp);
+
+ if (fseek(fp, 0, SEEK_SET) < 0) {
+ fprintf(stderr, "GRUB: cannot seek %s\n", GRUB_ENVBLK_PATH);
+ return NULL;
+ }
+
+ buf = malloc(size);
+ if (!buf) {
+ fprintf(stderr, "GRUB: No memory: malloc failed\n");
+ return NULL;
+ }
+
+ if (fread(buf, 1, size, fp) != size) {
+ fprintf(stderr, "GRUB: cannot read %s\n", GRUB_ENVBLK_PATH);
+ return NULL;
+ }
+
+ fclose(fp);
+
+ envblk = grub_envblk_open(buf, size);
+ if (!envblk) {
+ fprintf(stderr, "GRUB: invalid environment block\n");
+ return NULL;
+ }
+
+ return envblk;
+}
+
+static int
+grub_write_envblk(grub_envblk_t envblk)
+{
+ FILE *fp;
+
+ int ret = 0;
+ fp = fopen(GRUB_ENVBLK_PATH, "wb");
+ if (!fp) {
+ fprintf(stderr, "GRUB: cannot open %s\n", GRUB_ENVBLK_PATH);
+ return -1;
+ }
+
+ if (fwrite(grub_envblk_buffer(envblk), 1, grub_envblk_size(envblk), fp)
+ != grub_envblk_size(envblk)) {
+ fprintf(stderr, "GRUB: cannot write to %s\n", GRUB_ENVBLK_PATH);
+ return -1;
+ }
+
+ fflush(fp);
+ fsync(fileno(fp));
+ ret = fclose(fp);
+
+ return ret;
+}
+
+int
+grub_set_variable(char *name, char *value)
+{
+ int ret = 0;
+ grub_envblk_t envblk;
+ envblk = grub_open_envblk_file();
+ if (!envblk) {
+ fprintf(stderr, "GRUB: failed to open envblk file\n");
+ return -1;
+ }
+
+ if (grub_envblk_set(envblk, name, value)) {
+ fprintf(stderr, "GRUB: environment block too small\n");
+ return -1;
+ }
+
+ ret = grub_write_envblk(envblk);
+ grub_envblk_close(envblk);
+ return ret;
+}
+
+/* unsetting variable is not used at the moment */
+int
+grub_unset_variable(char *name)
+{
+ int ret = 0;
+ grub_envblk_t envblk;
+ envblk = grub_open_envblk_file();
+ if (!envblk) {
+ fprintf(stderr, "GRUB: failed to open envblk file\n");
+ return -1;
+ }
+
+ if (grub_envblk_delete(envblk, name)) {
+ fprintf(stderr, "GRUB: failed to delete variable: %s\n", name);
+ return ret;
+ }
+
+ ret = grub_write_envblk(envblk);
+ grub_envblk_close(envblk);
+ return ret;
+}
diff --git a/handlers/Config.in b/handlers/Config.in
index a05c1fc1ef9a..ea782e8dd400 100644
--- a/handlers/Config.in
+++ b/handlers/Config.in
@@ -148,4 +148,18 @@ config UBOOT_FWENV
in the tools directory. It tells where the U-Boot
environment is saved.

+config GRUB
+ bool "grub"
+ default n
+ help
+ Enable it to change GRUB environment
+ during the installation process.
+
+config GRUB_ENV
+ string "GRUB Environment block file path"
+ depends on GRUB
+ default "/boot/efi/EFI/BOOT/grub/grubenv"
+ help
+ Provide path to grub environment block file
+
endmenu
diff --git a/include/grub_env.h b/include/grub_env.h
new file mode 100644
index 000000000000..97b96e807559
--- /dev/null
+++ b/include/grub_env.h
@@ -0,0 +1,46 @@
+#include <stdlib.h>
+
+#define GRUB_ENVBLK_SIGNATURE "# GRUB Environment Block\n"
+#define GRUB_DEFAULT_ENVBLK_PATH "/boot/efi/EFI/BOOT/grub/grubenv"
+
+#ifdef CONFIG_GRUB_ENV
+#define GRUB_ENVBLK_PATH CONFIG_GRUB_ENV
+#else /* CONFIG_GRUB_ENV */
+#define GRUB_ENVBLK_PATH GRUB_DEFAULT_ENVBLK_PATH
+#endif /* CONFIG_GRUB_ENV */
+
+struct grub_envblk
+{
+ char *buf;
+ size_t size;
+};
+
+
+/* U-Boot handler allows to set multiple env variables by listing them in a
+ * file. It has not been implemented for GRUB since we did not need this.
+ * Setting single variables inside sw-desciption was enough. Should it be
+ * implemented? */
+/* int grub_parse_script(char *name); */
+typedef struct grub_envblk *grub_envblk_t;
+
+/* only 'set' and 'unset' are callable from external */
+int grub_set_variable(char *name, char *value);
+/* unset is not used at the moment */
+/* In case of U-Boot, variable is being unset when no value is given.
+ * In many grub.cfg configuration files, setting empty value to a variable
+ * seems to be quite common practice. Implementing unsetting variables policy
+ * as in case of U-Boot (no value = unset) may limit usage of grub.cfg
+ */
+int grub_unset_variable(char *name);
+
+static inline char *
+grub_envblk_buffer(const grub_envblk_t envblk)
+{
+ return envblk->buf;
+}
+
+static inline size_t
+grub_envblk_size(const grub_envblk_t envblk)
+{
+ return envblk->size;
+}
diff --git a/include/swupdate.h b/include/swupdate.h
index bff757ebff7a..b21f4793ee78 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -123,6 +123,7 @@ struct swupdate_cfg {
struct imglist partitions;
struct imglist scripts;
struct dictlist uboot;
+ struct dictlist grub;
void *dgst; /* Structure for signed images */
struct swupdate_global_cfg globals;
};
diff --git a/parser/parser.c b/parser/parser.c
index 33a496085441..4e635b8cccc3 100644
--- a/parser/parser.c
+++ b/parser/parser.c
@@ -349,6 +349,50 @@ static void parse_uboot(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
}
}

+#ifdef CONFIG_GRUB
+static void parse_grub(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
+{
+ void *setting, *elem;
+ int count, i;
+ char name[32];
+ char value[255];
+
+ setting = find_node(p, cfg, "grub", swcfg);
+
+ if (setting == NULL)
+ return;
+
+ count = get_array_length(p, setting);
+ for(i = (count - 1); i >= 0; --i) {
+ elem = get_elem_from_idx(p, setting, i);
+
+ if (!elem)
+ continue;
+
+ /*
+ * Check for mandatory field
+ */
+ if(!(exist_field_string(p, elem, "name"))) {
+ TRACE("GRUB entry without variable name field, skipping..");
+ continue;
+ }
+
+ /*
+ * Call directly get_field_string with size 0
+ * to let allocate the place for the strings
+ */
+ GET_FIELD_STRING(p, elem, "name", name);
+ GET_FIELD_STRING(p, elem, "value", value);
+ dict_set_value(&swcfg->grub, name, value);
+
+ TRACE("GRUB var: %s = %s\n",
+ name,
+ dict_get_value(&swcfg->grub, name));
+
+ }
+}
+#endif /* config_grub */
+
static void parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
{
void *setting, *elem;
@@ -496,6 +540,9 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
parse_images(p, cfg, swcfg);
parse_scripts(p, cfg, swcfg);
parse_uboot(p, cfg, swcfg);
+#ifdef CONFIG_GRUB
+ parse_grub(p, cfg, swcfg);
+#endif /* config_grub */
parse_files(p, cfg, swcfg);

/*
@@ -507,6 +554,9 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
if (LIST_EMPTY(&swcfg->images) &&
LIST_EMPTY(&swcfg->partitions) &&
LIST_EMPTY(&swcfg->scripts) &&
+#ifdef CONFIG_GRUB
+ LIST_EMPTY(&swcfg->grub) &&
+#endif /* config_grub */
LIST_EMPTY(&swcfg->uboot)) {
ERROR("Found nothing to install\n");
return -1;
--
2.7.4

[Stefano Babic]

Hi Maciej,

thanks for your work.

First at all, there is a quite big licensing issue. Your grub-editenv.c
is licensed under GPLv3, and this conflicts with SWUpdate (GPLv2).

Having a file GPLv3 makes the project GPLv3. This means that
manufacturers must provide with whatever information or data is
necessary to install modified software on the device. This is a big
issue on embedded device, and one of the reasons why a lot of projects
(including kernel) are GPLv2 (or LGPL).

I cannot merge a GPLv3 file into the project and rather this file should
be rewritten - however, reading / writing a 1024 byte block is not very
difficult, and even if disturbing because there is already a working
code, it is something that can be reasonably done.

On 21/03/2017 12:47, Maciej Pijanowski wrote:
> Signed-off-by: Maciej Pijanowski <maciej.p...@3mdeb.com>
> ---
> Following patch provides initial support for grubenv modification in similar
> fashion as for U-Boot. Environmental block handling is basically almost 1:1 ported
> from GRUB source code. We've been successfully using it for a few months by now
> with VirtualBox (GRUB2 EFI) and would like to attempt to upstream.
>
> At the moment GRUB config option is added in handler section. Although it is
> not really a handler - it can only set variables parsed from sw-description
> file.
>
> Possible fields of improvements:
> * Error handling and printing (usage of swupdate macros?).

They should be used - SWUpdate's macros are now quite more, because they
allow to forward an exception to the installer if raised from another
process. It is the centralized way to collect traces / exceptions.

> * Atomicity of grubenv modification. In previous messages we thought about
> invoking mv system call. In GRUB source code, content of grubenv is read into
> buffer and modified inside buffer. When buffer modification succeeds, then an
> actual grubenv file gets written using fwrite. I guess this it not enough
> fot swupdate requirements?

We must avoid to brick the device. I explain this with an example: think
there is a power-cut when the fwrite() is called. The grubenv is in most
cases truncated to zero after reboot or corrupted. This can lead that
the device cannot boot.

I have imagined another scenario, not very different.

- your function modifies grubenv, but it writes to a different file. So
we have grubenv and grubenv.new.
- when fwrite completes, there is the "mv" system call that is at least
atomic:grubenv.new --> grubenv.

We do not reach the security of U-Boot with the two copies, but we
reduce the risks. To have the same safety as U-Boot, Grub should manage
itself two copies of the environment, but this feature is as far as I
know not (yet ?) implemented.

> * Introduce bootloader abstraction - bootloader menu in menuconfig that allows
> to pick GRUB / U-Boot etc.

Absolutely agree - this is what I am already thinking about. We need
some general API in SWUpdate that do not depend on the chosen bootloader
(write_boot_env, read_boot_env,..).

> * GRUB actual handler - setting multiple environmental variables using single
> file as in case of U-Boot handler script - is this desired?

Reason for that is to have an atomic switch. I built this for U-Boot to
guarantee that all variables are changed in a single shot like a
transaction. In fact, the new environment is written after setting all
variables, and not after setting a single variable. This means that does
not happen that just a part of variables are set - this can be dangerous
because it can brick again the device.

> * Unsetting (deleting) variable from grubenv. At the moment this is not
> implemented. In case of U-Boot, variable is being unset when no value is given.
> In many grub.cfg configuration files, setting empty value to a variable
> seems to be quite common practice. Implementing unsetting variables policy
> as in case of U-Boot (no value = unset) may limit usage of grub.cfg. Maybe
> use another keyword or mechanism for unset operation?

Understood - I agree that this can be done by follow up patches.

The update_uboot_env() is just used in case of local install.
Nevertheless, your function shows that an abstraction for the bootloader
is really needed. We should have something like "update_boot_env(...)",
independently if we have U-Boot or Grub or...

> int install_single_image(struct img_type *img)
> {
> struct installer_handler *hnd;
> @@ -270,6 +295,14 @@ int install_images(struct swupdate_cfg *sw, int fdsw, int fromfile)
> return ret;
> }
>
> +#ifdef CONFIG_GRUB
> + /* Update GRUB environment */
> + ret = update_grub_env(sw);
> + if (ret) {
> + return ret;
> + }
> +#endif /* CONFIG_GRUB */
> +

This should not be here, and install_single_image() is called also in
case of streaming. This means that update_grub_env() is called for each
artifacts, and this is wrong.

As remark: I agree to add configuration file (maybe it makes sense in a
separate patch). I have starting adding a tracis configuration file to
the project, and having multiple configs allows us to automatically test
the builds in several conditions.

> diff --git a/grub/Makefile b/grub/Makefile
> new file mode 100644
> index 000000000000..5018fd093001
> --- /dev/null
> +++ b/grub/Makefile
> @@ -0,0 +1 @@
> +lib-$(CONFIG_GRUB) += grub_env.o
> diff --git a/grub/grub_env.c b/grub/grub_env.c
> new file mode 100644
> index 000000000000..38570e47071c
> --- /dev/null
> +++ b/grub/grub_env.c

I propose to add a "bootloader" directory instead of "grub", where we
can put all relevant functions for U-Boot, Grub,...

> @@ -0,0 +1,344 @@
> +/* grub-editenv.c - tool to edit environment block. */
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2008,2009,2010 Free Software Foundation, Inc.
> + *
> + * GRUB is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or

^-- rather this is a big issue

So I will not to go on reviewing this file, that must be rewritten :-(

What about to merge them, changing the names ? We do not need both lists
at the same time.

Same here - this is a copy&paste from the U-Boot entry. But if we have a
single list for bootloader environment, we factorize the code.

What we can also do is to allow another name for the section - I will
let "u-boot" for compatibility reason, but we can add "bootloader", too.
The parse_bootloader() (current parse_uboot) will point to "bootloader"
or as fallback to "u-boot", if "bootloader" is not found.

> +}
> +#endif /* config_grub */
> +
> static void parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
> {
> void *setting, *elem;
> @@ -496,6 +540,9 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
> parse_images(p, cfg, swcfg);
> parse_scripts(p, cfg, swcfg);
> parse_uboot(p, cfg, swcfg);
> +#ifdef CONFIG_GRUB
> + parse_grub(p, cfg, swcfg);
> +#endif /* config_grub */
> parse_files(p, cfg, swcfg);
>
> /*
> @@ -507,6 +554,9 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
> if (LIST_EMPTY(&swcfg->images) &&
> LIST_EMPTY(&swcfg->partitions) &&
> LIST_EMPTY(&swcfg->scripts) &&
> +#ifdef CONFIG_GRUB
> + LIST_EMPTY(&swcfg->grub) &&
> +#endif /* config_grub */
> LIST_EMPTY(&swcfg->uboot)) {
> ERROR("Found nothing to install\n");
> return -1;
>

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Maciej Pijanowski]

On 03/21/2017 02:50 PM, Stefano Babic wrote:

Hi Maciej,

Hi Stefano,

thanks for your work.

First at all, there is a quite big licensing issue. Your grub-editenv.c
is licensed under GPLv3, and this conflicts with SWUpdate (GPLv2).

Having a file GPLv3 makes the project GPLv3. This means that
manufacturers must provide with whatever information or data is
necessary to install modified software on the device. This is a big
issue on embedded device, and one of the reasons why a lot of projects
(including kernel) are GPLv2 (or LGPL).

I cannot merge a GPLv3 file into the project and rather this file should
be rewritten - however, reading / writing a 1024 byte block is not very
difficult, and even if disturbing because there is already a working
code, it is something that can be reasonably done.

Thanks for pointing that out. Sorry for not paying attention to this matter enough.
I will try to rewrite this in upcoming time. Then we can go back to further discussion.

I'll try addressing this in an upcoming implementation.

-- 
Maciej Pijanowski
Embedded Systems Engineer
http://3mdeb.com | @3mdeb_com