[PATCH] handlers: add filesystem handler

[roland.g...@weidmueller.com]

From: Roland Gaudig <roland...@weidmueller.com>

Signed-off-by: Roland Gaudig <roland...@weidmueller.com>
---

This patch introduces a handler for creating file systems on spare
partitions. It checks if the desired filesystem exists. In case it
doesn't the file system will be created. In case there is already
a filesystem of the desired type on that partition, nothing will be
changed.

doc/source/handlers.rst | 20 ++++++
doc/source/sw-description.rst | 5 +-
handlers/Config.in | 7 ++
handlers/Makefile | 1 +
handlers/filesystem_handler.c | 120 ++++++++++++++++++++++++++++++++++
5 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 handlers/filesystem_handler.c

diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
index 07fb6c2..f9b4fbc 100644
--- a/doc/source/handlers.rst
+++ b/doc/source/handlers.rst
@@ -857,3 +857,23 @@ found on the device. It is a partition handler and it runs before any image is i
"18e12df1-d8e1-4283-8727-37727eb4261d"];
}
});
+
+Filesystem Handler
+------------------
+
+This handler checks if the device already has a filesystem of the specified type.
+If the filesystem does not yet exist, it will be created.
+
+::
+
+ scripts: (
+ {
+ device = "/dev/mmcblk0p1";
+ type = "filesystem";
+ filesystem = "vfat";
+ },
+ {
+ device = "/dev/mmcblk0p2";
+ type = "filesystem";
+ filesystem = "ext2";
+ })
diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index fd8bd11..98001d9 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -1242,8 +1242,11 @@ There are 4 main sections inside sw-description:
| | | | rootfs will be used. |
+-------------+----------+------------+---------------------------------------+
| filesystem | string | files | indicates the filesystem type where |
- | | | | the file must be installed. Only |
+ | | | scripts | the file must be installed. Only |
| | | | used if "device" attribute is set. |
+ | | | | In combination witht the filesystem |
+ | | | | handler it indicates the filesystem |
+ | | | | which shall be created. |
+-------------+----------+------------+---------------------------------------+
| path | string | files | For files: indicates the path |
| | | | (absolute) where the file must be |
diff --git a/handlers/Config.in b/handlers/Config.in
index 8571c0c..a646610 100644
--- a/handlers/Config.in
+++ b/handlers/Config.in
@@ -99,6 +99,13 @@ config DISKPART
help
Handler to partition a disk, eMMC or SD

+config FILESYSTEM
+ bool "filesystem handler for creating file systems on empty partitions"
+ default n
+ help
+ The filesystem handler checks if a partition already contains the
+ specified filesystem. If not, the file system will be created.
+
comment "diskpart support needs libfdisk"
depends on !HAVE_LIBFDISK

diff --git a/handlers/Makefile b/handlers/Makefile
index 1449139..c0d78b6 100644
--- a/handlers/Makefile
+++ b/handlers/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_ARCHIVE) += archive_handler.o
obj-$(CONFIG_BOOTLOADERHANDLER) += boot_handler.o
obj-$(CONFIG_CFI) += flash_handler.o
obj-$(CONFIG_DISKPART) += diskpart_handler.o
+obj-$(CONFIG_FILESYSTEM) += filesystem_handler.o
obj-$(CONFIG_UNIQUEUUID) += uniqueuuid_handler.o
obj-$(CONFIG_CFIHAMMING1)+= flash_hamming1_handler.o
obj-$(CONFIG_LUASCRIPTHANDLER) += lua_scripthandler.o
diff --git a/handlers/filesystem_handler.c b/handlers/filesystem_handler.c
new file mode 100644
index 0000000..f7ba3ee
--- /dev/null
+++ b/handlers/filesystem_handler.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2021 Weidmueller Interface GmbH & Co. KG
+ * Roland Gaudig <roland...@weidmueller.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "swupdate.h"
+#include "handler.h"
+#include "pctl.h"
+#include "util.h"
+
+void filesystem_handler_init(void);
+
+
+static int filesystem(struct img_type *img,
+ void __attribute__ ((__unused__)) *data)
+{
+ char *lbltype = img->filesystem;
+ int ret = 0;
+ char command[SWUPDATE_GENERAL_STRING_SIZE];
+ int len;
+ int exit_code;
+
+ if (!data)
+ return -EINVAL;
+
+ script_fn scriptfn = *(script_fn *)data;
+
+ /*
+ * Run only in case of pre-install
+ */
+ if (scriptfn != PREINSTALL)
+ return 0;
+
+ if (!strlen(img->device)) {
+ ERROR("File system handler requires setting \"device\" attribute");
+ return -EINVAL;
+ }
+
+ if (!lbltype) {
+ ERROR("File system handler requires setting \"filesystem\" attribute");
+ return -EINVAL;
+ }
+
+ /* Check if filesystem exists */
+ len = snprintf(command, SWUPDATE_GENERAL_STRING_SIZE,
+ "fsck.%s -n %s", lbltype, img->device);
+ if (len < 0) {
+ ERROR("File snprintf");
+ return -1;
+ }
+
+ if (len > (SWUPDATE_GENERAL_STRING_SIZE - 1)) {
+ ERROR("Path name to device is to long");
+ return -1;
+ }
+
+ TRACE("Check if %s file system exists on %s ...", lbltype, img->device);
+ exit_code = run_system_cmd(command);
+
+ if (exit_code == -1) {
+ ERROR("Could not run %s", command);
+ return -1;
+ }
+
+ if (exit_code == 127) {
+ ERROR("fsck.%s not found, %s filesystem not supported", lbltype, lbltype);
+ return -1;
+ }
+
+ if (exit_code == 0) {
+ TRACE("Found %s file system on %s", lbltype, img->device);
+ return 0;
+ }
+
+ if (exit_code > 0) {
+ /* File system does not exist, create new file system */
+ TRACE("Creating new %s file system on %s", lbltype, img->device);
+ len = snprintf(command, SWUPDATE_GENERAL_STRING_SIZE,
+ "mkfs.%s %s", lbltype, img->device);
+
+ if (len < 0) {
+ ERROR("File snprintf");
+ return -1;
+ }
+
+ if (len > (SWUPDATE_GENERAL_STRING_SIZE - 1)) {
+ ERROR("Path name to device is to long");
+ return -1;
+ }
+
+ exit_code = run_system_cmd(command);
+
+ if (exit_code) {
+ if (exit_code == -1) {
+ ERROR("File could not run %s", command);
+ return -1;
+ } else {
+ ERROR("Couldn't create %s filesystem on %s",
+ lbltype, img->device);
+ return -1;
+ }
+ }
+ }
+
+ return ret;
+}
+
+
+__attribute__((constructor))
+void filesystem_handler_init(void)
+{
+ register_handler("filesystem", filesystem,
+ SCRIPT_HANDLER | NO_DATA_HANDLER, NULL);
+}
--
2.25.1

[Stefano Babic]

Hi Roland,

On 24.02.21 13:55, roland.g...@weidmueller.com wrote:
> From: Roland Gaudig <roland...@weidmueller.com>
>
> Signed-off-by: Roland Gaudig <roland...@weidmueller.com>
> ---
>
> This patch introduces a handler for creating file systems on spare
> partitions. It checks if the desired filesystem exists. In case it
> doesn't the file system will be created. In case there is already
> a filesystem of the desired type on that partition, nothing will be
> changed.

I have thought myself such as extension, but I won't do in the tricky
way, that is just executing an external command.

You have implemented as SCRIPT_HANDLER. IMHO this has some limitations,
also because the script handler is called after and too late if an
artefact is streamed. IMHO this feature is related to the partitions,
and I see this as an extension of the diskpart handler, like a property
as "format = <filesystem type>".

Partiton handlers are always called before installing something, and
this allows also to format a partition and then install as files a
tarball - what it is not guaranteed here.

The second aspect is that this handler is a wrapper of run_system_cmd(),
that at the end fork the process and execute an exteranl command. This
is not aligned with the rest of code, because SWUpdate does not rely on
external commands with the exception of shell scripts and pre- / post-
update command, with the goal to be as much as possible self contained,
and to avoid that an attacker can compromize an external tool and
SWUpdate relies on it. Of course, it cannot be completely self
contained, but it should remain as much as possible.

So my idea for this feature is to link the libext2fs library (for extX)
and execute the formatting inside the same process of the handler
without forking to the shell. Take into account that I have taken the
same decision (surely with more effort) for disk partitioning, linking
to libfdisk instead of simply calling parted, sfdisk and company.

What do you think ?

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
=====================================================================

[Stefano Babic]

Hello Stefano,

On Wed, Feb 24, 2021 at 06:48:10PM +0100, Stefano Babic wrote:
> On 24.02.21 13:55, roland.g...@weidmueller.com wrote:
> > From: Roland Gaudig <roland...@weidmueller.com>
> >
> > Signed-off-by: Roland Gaudig <roland...@weidmueller.com>
> > ---
> >
> > This patch introduces a handler for creating file systems on spare
> > partitions. It checks if the desired filesystem exists. In case it
> > doesn't the file system will be created. In case there is already
> > a filesystem of the desired type on that partition, nothing will be
> > changed.
>
> I have thought myself such as extension, but I won't do in the tricky
> way, that is just executing an external command.

On the first glance executing an external command seemed to be the most
obvious way for me. But I observed some slight differences between
different versions of mkfs, which might lead to tricky situations.

> You have implemented as SCRIPT_HANDLER. IMHO this has some limitations,
> also because the script handler is called after and too late if an
> artefact is streamed. IMHO this feature is related to the partitions,
> and I see this as an extension of the diskpart handler, like a property
> as "format = <filesystem type>".

Thematically it would really better fit to be an extension of the
diskpart handler. As I wanted to alter only one partition, the
SCRIPT_HANDLER seemed to be a rather convenient way, as I just had to
specify the device file for the partition being altered.

If I have understood the diskpart handler correctly I, always would have
to specify all partitions with their locations and sizes. This seems to
be rather inconvenient when I intend to create only just one file system.

Nice would be if I just could specify it in a way like this:

device = "/dev/mmcblk1p1";
format = "vfat";

> Partiton handlers are always called before installing something, and
> this allows also to format a partition and then install as files a
> tarball - what it is not guaranteed here.

That is an important aspect.

> The second aspect is that this handler is a wrapper of run_system_cmd(),
> that at the end fork the process and execute an exteranl command. This
> is not aligned with the rest of code, because SWUpdate does not rely on
> external commands with the exception of shell scripts and pre- / post-
> update command, with the goal to be as much as possible self contained,
> and to avoid that an attacker can compromize an external tool and
> SWUpdate relies on it. Of course, it cannot be completely self
> contained, but it should remain as much as possible.

That is a reasonable design decision.

> So my idea for this feature is to link the libext2fs library (for extX)
> and execute the formatting inside the same process of the handler
> without forking to the shell. Take into account that I have taken the
> same decision (surely with more effort) for disk partitioning, linking
> to libfdisk instead of simply calling parted, sfdisk and company.

That is a possible way to go. But our current need is a FAT file system
partition. One possible solution would be to use code from dosfstools, but
it uses GPL-3 or later, whereas Swupdate uses GPL-2 or later. Another
FAT file system library we found is this FatFs library:

http://elm-chan.org/fsw/ff/00index_e.html

Its license is liberal, but its text reads quite strange.

Maybe you have another idea for creating a FAT file system?

Best regards,
Roland Gaudig

[Stefano Babic]

Hi Roland,

On 25.02.21 11:29, Roland Gaudig wrote:
> Hello Stefano,
>
> On Wed, Feb 24, 2021 at 06:48:10PM +0100, Stefano Babic wrote:

>> On 24.02.21 13:55, roland.g...@weidmueller.com wrote:
>>> From: Roland Gaudig <roland...@weidmueller.com>
>>>
>>> Signed-off-by: Roland Gaudig <roland...@weidmueller.com>
>>> ---
>>>
>>> This patch introduces a handler for creating file systems on spare
>>> partitions. It checks if the desired filesystem exists. In case it
>>> doesn't the file system will be created. In case there is already
>>> a filesystem of the desired type on that partition, nothing will be
>>> changed.
>>
>> I have thought myself such as extension, but I won't do in the tricky
>> way, that is just executing an external command.
>

> On the first glance executing an external command seemed to be the most
> obvious way for me. But I observed some slight differences between
> different versions of mkfs, which might lead to tricky situations.

Ok

>
>> You have implemented as SCRIPT_HANDLER. IMHO this has some limitations,
>> also because the script handler is called after and too late if an
>> artefact is streamed. IMHO this feature is related to the partitions,
>> and I see this as an extension of the diskpart handler, like a property
>> as "format = <filesystem type>".
>

> Thematically it would really better fit to be an extension of the
> diskpart handler. As I wanted to alter only one partition, the
> SCRIPT_HANDLER seemed to be a rather convenient way, as I just had to
> specify the device file for the partition being altered.
>
> If I have understood the diskpart handler correctly I, always would have
> to specify all partitions with their locations and sizes. This seems to
> be rather inconvenient when I intend to create only just one file system.

Anyway, the diskpart handler provides a read-modify-write of the
partitions. If the partition table does not change, nothing is written
on the disk.

But I could also think that the handler regiters itself also with a
seconf handler that just formats.

__attribute__((constructor))
void diskformat_handler(void)
{
register_handler("diskformat", diskformat,
PARTITION_HANDLER | NO_DATA_HANDLER,
NULL);
}

if this can help.

>
> Nice would be if I just could specify it in a way like this:
>
> device = "/dev/mmcblk1p1";
> format = "vfat";
>

>> Partiton handlers are always called before installing something, and
>> this allows also to format a partition and then install as files a
>> tarball - what it is not guaranteed here.
>

> That is an important aspect.

Right.

>
>> The second aspect is that this handler is a wrapper of run_system_cmd(),
>> that at the end fork the process and execute an exteranl command. This
>> is not aligned with the rest of code, because SWUpdate does not rely on
>> external commands with the exception of shell scripts and pre- / post-
>> update command, with the goal to be as much as possible self contained,
>> and to avoid that an attacker can compromize an external tool and
>> SWUpdate relies on it. Of course, it cannot be completely self
>> contained, but it should remain as much as possible.
>

> That is a reasonable design decision.
>

>> So my idea for this feature is to link the libext2fs library (for extX)
>> and execute the formatting inside the same process of the handler
>> without forking to the shell. Take into account that I have taken the
>> same decision (surely with more effort) for disk partitioning, linking
>> to libfdisk instead of simply calling parted, sfdisk and company.
>

> That is a possible way to go. But our current need is a FAT file system
> partition. One possible solution would be to use code from dosfstools, but
> it uses GPL-3 or later, whereas Swupdate uses GPL-2 or later.

I know, GPL-3 is a now way here.

> Another
> FAT file system library we found is this FatFs library:
>
> http://elm-chan.org/fsw/ff/00index_e.html
>
> Its license is liberal, but its text reads quite strange.

It looks to me it provides access to FAT files, but it cannot format.


Maybe taking the code from FreeRTOS (MIT) ?

>
> Maybe you have another idea for creating a FAT file system?

Using external mkfs.vfat, a C wrapper as in your patch is not necessary.
You get already this feature using Lua. You write a Lua handler, this
registers itself as "PARTITION" handler and in Lua code you execute
mkfs.vfat exactly as you do in C code (but with more flexibility). You
get automatically all information from sw-description (SWUpdate will
still parse then for you) and you can access them as image.device,
image.format, etc.

Anyway, I remain interested for an inbound solution.

Best regards,
Stefano Babic