[2/3] handlers: rdiff handler for applying librsync's rdiff patches

The rdiff handler adds support for applying binary
delta patches generated by librsync's rdiff tool,
see http://librsync.sourcefrog.net

Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 Makefile.flags           |   4 +
 doc/source/handlers.rst  |  54 +++++
 doc/source/swupdate.rst  |   1 +
 handlers/Config.in       |   7 +
 handlers/Makefile        |   1 +
 handlers/rdiff_handler.c | 413 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 480 insertions(+)
 create mode 100644 handlers/rdiff_handler.c

Hi Christian,

On 09/11/18 16:51, Christian Storm wrote:
> The rdiff handler adds support for applying binary
> delta patches generated by librsync's rdiff tool,
> see http://librsync.sourcefrog.net

I would like to test and try myself the patchset, so I am not yet going
to a full review. I think that the requirements are not fully
established, I mean:

which is the required version for librsync ? Distros are coming with
0.9.7. There is an old OE recipe for the same version, too, but not
merged in any current layer.
If  there is not yet a layer with recipe, I agree to put it into
meta-swupdate.

Anyway, last librsync release is 2.0.2. I am expecting to use a quite
recent version for it.

Do we have some values about the saving factor by using librsync ? IMHO
this can help in case Software is continuosly update, that is moving
from a version to the next one. But I guess that there is no advantage
or even a disadvantage by moving from an arbitrary version. For the last
one, I have imagined a pretty more complicated setup. Do you have some
values, for example between a -krogoth release and a -sumo ? Or
generally, how much can we save ?

I would like to understand which is the way to generate and deploy the
delta. The thing is that only the device knows which is the running
version, and just the build system knows the new one. The build server
could generate any diff between any former release to the last one, but
it looks like quite expensive. The result is then a set of SWUs, instead
of a single SWU. Let's say, this was done in some way.

Now we have a set of SWUs and the device should pick up the correct one.
How is this done ? In case of Hawkbit, we could have a set of Software
Modules, but they are all parts of the distro and we have not (yet ?) a
mechanism on the device to choose which one is downloaded.

In case of other interfaces, it is even more complictaed because this is
let to the operator and cannot be done automatically. Let's think to the
integrated Webserver: the device is in this case not the active
operator, and it expects to get the right SWU. It can just reject or
accept the installation. The operator must then choose the right SWU,
the one that contains the diff between the running version and the new one.

So can you start to explain these requirements ? Even if we think that
we want to add a single use case, we should then pretty document and
explain this in documentation.

The next point I want to discuss is the application of this in case of
compressed images. I guess the delta apply before compressing the
images, right ? After compression I feel we have bad chances to get
small deltas.

That means, after reading the example:

- we build a filesystem, let's say a "my-image-domething.[filesystem]".

- we run rdiff of it ==> new image type in OE ? A class in
meta-swupdate, maybe ?
I guess it is quite difficult without hacks to run this in OE, because a
build must have the knowledge of previous builds (with different layer
setup, maybe).

- result can be compressed, too ==> Swupdate decompress it before
running the handler.

Back to the original thread by Paul, can we apply this to the kernel ?
Kernel is already compressed as result of the "compile" task. And image
format can vary, we could have a vmlinux (uncompressed), a zImage, or a
uImage (both compressed) or a fitImage. Is it appliable to the kernel ?

> 
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>  Makefile.flags           |   4 +
>  doc/source/handlers.rst  |  54 +++++
>  doc/source/swupdate.rst  |   1 +
>  handlers/Config.in       |   7 +
>  handlers/Makefile        |   1 +
>  handlers/rdiff_handler.c | 413 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 480 insertions(+)
>  create mode 100644 handlers/rdiff_handler.c
> 
> diff --git a/Makefile.flags b/Makefile.flags
> index b7b7389..82365a5 100644
> --- a/Makefile.flags
> +++ b/Makefile.flags
> @@ -165,6 +165,10 @@ ifeq ($(CONFIG_GUNZIP),y)
>  LDLIBS += z
>  endif
>  
> +ifeq ($(CONFIG_RDIFFHANDLER),y)
> +LDLIBS += rsync
> +endif
> +
>  ifeq ($(CONFIG_REMOTE_HANDLER),y)
>  LDLIBS += zmq
>  endif
> diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
> index 97c4d49..f9c3a74 100644
> --- a/doc/source/handlers.rst
> +++ b/doc/source/handlers.rst
> @@ -350,6 +350,60 @@ the SWU forwarder:
>  			};
>  		});
>  
> +
> +rdiff handler
> +-------------
> +
> +The rdiff handler adds support for applying binary delta patches generated by
> +`librsync's <http://librsync.sourcefrog.net/>`_ rdiff tool.
> +
> +First, create the signature of the original (base) file via
> +``rdiff signature <basefile> <signaturefile>``.
> +Then, create the delta file (i.e., patch) from the original base file to the target
> +file via ``rdiff delta <signaturefile> <targetfile> <deltafile>``.
> +The ``<deltafile>`` is the artifact to be applied via this handler on the device.
> +Essentially, it mimics running ``rdiff patch <basefile> <deltafile> <targetfile>``
> +on the device. Naturally for patches, the very same ``<basefile>`` has to be used
> +for creating as well as for applying the patch to.
> +
> +This handler registers itself for handling files and images.
> +An exemplary sw-description fragment for the files section is
> +
> +::
> +
> +    files: (
> +        {
> +            type = "rdiff"

If you use rdiffimage, maybe we have to set it as rdifffile.

> +            filename = "file.rdiff.delta";
> +            path = "/usr/bin/file";
> +        }
> +    );

Does it work with "installed-directly" ?

> +
> +
> +Note that the file referenced to by ``path`` serves as ``<basefile>`` and
> +gets replaced by a temporary file serving as ``<targetfile>`` while the rdiff
> +patch processing.
> +
> +An exemplary sw-description fragment for the images section is
> +
> +::
> +
> +    images: (
> +        {
> +            type = "rdiffimage";
> +            filename = "image.rdiff.delta";
> +            device = "/dev/mmcblk0p2";
> +            properties: {
> +                rdiffbase = ["/dev/mmcblk0p1"];
> +            };
> +        }
> +    );
> +
> +
> +Here, the property ``rdiffbase`` qualifies the ``<basefile>`` while the ``device``
> +attribute designates the ``<targetfile>``.
> +
> +
>  ucfw handler
>  ------------
>  
> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
> index 75fc4fb..7bbfd28 100644
> --- a/doc/source/swupdate.rst
> +++ b/doc/source/swupdate.rst
> @@ -214,6 +214,7 @@ There are only a few libraries that are required to compile SWUpdate.
>  - libz, libcrypto are always linked.
>  - libconfig: it is used by the default parser.
>  - libarchive (optional) for archive handler
> +- librsync (optional) for support to apply rdiff patches
>  - libjson (optional) for JSON parser and Hawkbit
>  - libubootenv (optional) if support for U-Boot is enabled
>  - libebgenv (optional) if support for EFI Boot Guard is enabled
> diff --git a/handlers/Config.in b/handlers/Config.in
> index 12a50b4..3e4e077 100644
> --- a/handlers/Config.in
> +++ b/handlers/Config.in
> @@ -99,6 +99,13 @@ config RAW
>  	  This is a simple handler that simply copies
>  	  into the destination.
>  
> +config RDIFFHANDLER
> +	bool "rdiff"
> +	default n
> +	help
> +	  Add support for applying librsync's rdiff patches,
> +	  see http://librsync.sourcefrog.net/
> +	
>  config LUASCRIPTHANDLER
>  	bool "Lua Script"
>  	depends on LUA
> diff --git a/handlers/Makefile b/handlers/Makefile
> index 8db9e41..b75c3ba 100644
> --- a/handlers/Makefile
> +++ b/handlers/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_SHELLSCRIPTHANDLER) += shell_scripthandler.o
>  obj-$(CONFIG_SWUFORWARDER_HANDLER) += swuforward_handler.o
>  obj-$(CONFIG_UBIVOL)	+= ubivol_handler.o
>  obj-$(CONFIG_UCFWHANDLER)	+= ucfw_handler.o
> +obj-$(CONFIG_RDIFFHANDLER) += rdiff_handler.o
> diff --git a/handlers/rdiff_handler.c b/handlers/rdiff_handler.c
> new file mode 100644
> index 0000000..a607613
> --- /dev/null
> +++ b/handlers/rdiff_handler.c
> @@ -0,0 +1,413 @@
> +/*
> + * Author: Christian Storm
> + * Copyright (C) 2018, Siemens AG
> + *
> + * SPDX-License-Identifier:     GPL-2.0-or-later
> + */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <stdbool.h>
> +#include <librsync.h>
> +#if defined(__linux__)
> +#include <sys/sendfile.h>
> +#endif
> +#if defined(__FreeBSD__)
> +#include <sys/param.h>
> +#endif
> +#include "swupdate.h"
> +#include "handler.h"
> +#include "util.h"
> +
> +/* Use rdiff's default inbuf and outbuf size of 64K */
> +#define RDIFF_BUFFER_SIZE 64 * 1024
> +
> +#define ASSERT(expr, failret) \
> +	if (expr) { \
> +	} else { \
> +		ERROR("Assertion violated: %s.", #expr); \
> +		return failret; \
> +	}
> +

do we need this ? what about "assert" in most code ?

> +void rdiff_file_handler(void);
> +void rdiff_image_handler(void);
> +
> +struct rdiff_t
> +{
> +	rs_job_t *job;
> +	rs_buffers_t buffers;
> +
> +	FILE *dest_file;
> +	FILE *base_file;
> +
> +	char *inbuf;
> +	char *outbuf;
> +
> +	long long cpio_input_len;
> +
> +	uint8_t type;
> +};
> +
> +static void rdiff_log(rs_loglevel level, char const *msg)
> +{
> +	int loglevelmap[] =
> +	{
> +		[RS_LOG_EMERG]   = ERRORLEVEL,
> +		[RS_LOG_ALERT]   = ERRORLEVEL,
> +		[RS_LOG_CRIT]    = ERRORLEVEL,
> +		[RS_LOG_ERR]     = ERRORLEVEL,
> +		[RS_LOG_WARNING] = WARNLEVEL,
> +		[RS_LOG_NOTICE]  = INFOLEVEL,
> +		[RS_LOG_INFO]    = INFOLEVEL,
> +		[RS_LOG_DEBUG]   = TRACELEVEL
> +	};
> +	*strchrnul(msg, '\n') = '\0';
> +	swupdate_notify(RUN, "%s", loglevelmap[level], msg);
> +}
> +
> +static rs_result base_file_read_cb(void *fp, rs_long_t pos, size_t *len, void **buf)
> +{
> +	FILE *f = (FILE *)fp;
> +
> +	if (fseek(f, pos, SEEK_SET) != 0) {
> +		ERROR("Error seeking rdiff base file: %s", strerror(errno));
> +		return RS_IO_ERROR;
> +	}
> +
> +	int ret = fread(*buf, 1, *len, f);
> +	if (ret == -1) {
> +		ERROR("Error reading rdiff base file: %s", strerror(errno));
> +		return RS_IO_ERROR;
> +	}
> +	if (ret == 0) {
> +		ERROR("Unexpected EOF on rdiff base file.");
> +		return RS_INPUT_ENDED;
> +	}
> +	*len = ret;
> +
> +	return RS_DONE;
> +}
> +
> +static rs_result fill_inbuffer(struct rdiff_t *rdiff_state, const void *buf, unsigned int len)
> +{
> +	rs_buffers_t *buffers = &rdiff_state->buffers;
> +
> +	if (buffers->eof_in == true) {
> +		TRACE("EOF on rdiff chunk input, not reading more data.");
> +		return RS_DONE;
> +	}
> +
> +	if (buffers->avail_in == 0) {
> +		/* No more buffered input data pending, get some... */
> +		ASSERT(len <= RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +		buffers->next_in = rdiff_state->inbuf;
> +		buffers->avail_in = len;
> +		(void)memcpy(rdiff_state->inbuf, buf, len);
> +	} else {
> +		/* There's more input, append it to input buffer. */
> +		char *target = buffers->next_in + buffers->avail_in;
> +		buffers->avail_in += len;
> +		ASSERT(target - buffers->next_in <= RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +		ASSERT(buffers->next_in >= rdiff_state->inbuf, RS_IO_ERROR);
> +		ASSERT(buffers->next_in <= rdiff_state->inbuf + RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +		(void)memcpy(target, buf, len);
> +	}
> +	rdiff_state->cpio_input_len -= len;
> +	buffers->eof_in = rdiff_state->cpio_input_len == 0 ? true : false;
> +	return RS_DONE;
> +}
> +
> +static rs_result drain_outbuffer(struct rdiff_t *rdiff_state)
> +{
> +	rs_buffers_t *buffers = &rdiff_state->buffers;
> +
> +	int len = buffers->next_out - rdiff_state->outbuf;
> +	ASSERT(len <= RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +	ASSERT(buffers->next_out >= rdiff_state->outbuf, RS_IO_ERROR);
> +	ASSERT(buffers->next_out <= rdiff_state->outbuf + RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +
> +	writeimage destfiledrain = copy_write;
> +#if defined(__FreeBSD__)
> +	if (rdiff_state->type == IMAGE_HANDLER) {
> +		destfiledrain = copy_write_padded;
> +		if (len % 512 != 0) {
> +			WARN("Output data is not 512 byte aligned!");
> +		}
> +	}
> +#endif
> +	if (len > 0) {
> +		buffers->next_out = rdiff_state->outbuf;
> +		buffers->avail_out = RDIFF_BUFFER_SIZE;
> +		int dest_file_fd = fileno(rdiff_state->dest_file);
> +		if (destfiledrain(&dest_file_fd, buffers->next_out, len) != 0) {
> +			return RS_IO_ERROR;
> +		}
> +	} else {
> +		DEBUG("No rdiff chunk data to drain.");
> +	}
> +	return RS_DONE;
> +}
> +
> +static int apply_rdiff_chunk_cb(void *out, const void *buf, unsigned int len)
> +{
> +	struct rdiff_t *rdiff_state = (struct rdiff_t *)out;
> +	rs_buffers_t *buffers = &rdiff_state->buffers;
> +
> +	if (buffers->next_out == NULL) {
> +		ASSERT(buffers->avail_out == 0, -1);
> +		buffers->next_out = rdiff_state->outbuf;
> +		buffers->avail_out = RDIFF_BUFFER_SIZE;
> +	}
> +
> +	if (fill_inbuffer(rdiff_state, buf, len) != RS_DONE) {
> +		return -1;
> +	}
> +
> +	rs_result result = RS_RUNNING;
> +	while (true) {
> +		result = rs_job_iter(rdiff_state->job, buffers);
> +		if (result != RS_DONE && result != RS_BLOCKED) {
> +			ERROR("Error processing rdiff chunk: %s", rs_strerror(result));
> +			return -1;
> +		}
> +		if (drain_outbuffer(rdiff_state) != RS_DONE) {
> +			return -1;
> +		}
> +		if (result == RS_BLOCKED && buffers->eof_in == true) {
> +			TRACE("rdiff chunk processing blocked for output buffer draining, "
> +				  "flushing output buffer.");
> +			continue;
> +		}
> +		if (result == RS_BLOCKED && buffers->eof_in == false) {
> +			TRACE("rdiff chunk processing blocked for input, "
> +				  "getting more chunk data.");
> +			break;
> +		}
> +		if (result == RS_DONE && buffers->eof_in == true) {
> +			TRACE("rdiff processing done.");
> +			break;
> +		}
> +		if (result == RS_DONE && buffers->eof_in == false) {
> +			WARN("rdiff processing done but input EOF not seen yet?");
> +			break;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int apply_rdiff_patch(struct img_type *img,
> +							 void __attribute__((__unused__)) * data)
> +{
> +	int ret = 0;
> +
> +	struct rdiff_t rdiff_state = {};
> +	rdiff_state.type =
> +	    strcmp(img->type, "rdiffimage") == 0 ? IMAGE_HANDLER : FILE_HANDLER;
> +
> +	char *mountpoint = NULL;
> +	bool use_mount = (strlen(img->device) && strlen(img->filesystem)) ? true : false;
> +
> +	char *base_file_filename = NULL;
> +	char *dest_file_filename = NULL;
> +
> +	if (rdiff_state.type == IMAGE_HANDLER) {
> +		if (img->seek) {
> +			/*
> +			 * img->seek mandates copyfile()'s out parameter to be a fd, it
> +			 * isn't. So, the seek option is invalid for the rdiff handler.
> +			 * */

Not clear, as  rdiff_state.dest_file is a fd, and a seek() on this fd
should be possible. What am I missing ?

> +			ERROR("Option 'seek' is not supported for rdiff.");
> +			return -1;
> +		}
> +
> +		base_file_filename = dict_get_value(&img->properties, "rdiffbase");
> +		if (base_file_filename == NULL) {
> +			ERROR("Property 'rdiffbase' is missing in sw-description.");
> +			return -1;
> +		}
> +
> +		if ((rdiff_state.dest_file = fopen(img->device, "wb+")) == NULL) {
> +			ERROR("%s cannot be opened for writing: %s", img->device, strerror(errno));
> +			return -1;
> +		}
> +	}
> +	if (rdiff_state.type == FILE_HANDLER) {
> +		int fd;
> +
> +		base_file_filename = img->path;
> +
> +		if (strlen(img->path) == 0) {
> +			ERROR("Missing path attribute");
> +			return -1;
> +		}
> +
> +		if (asprintf(&dest_file_filename, "%srdiffpatch.XXXXXX", get_tmpdir()) == -1) {
> +			ERROR("Cannot allocate memory for temporary filename creation.");
> +			return -1;
> +		}
> +		if ((fd = mkstemp(dest_file_filename)) == -1) {
> +			ERROR("Cannot create temporary file %s: %s", dest_file_filename,
> +				  strerror(errno));
> +			return -1;
> +		}
> +
> +		if ((rdiff_state.dest_file = fdopen(fd, "wb+")) == NULL) {
> +			(void)close(fd);
> +			ERROR("%s cannot be opened for writing: %s", dest_file_filename,
> +				  strerror(errno));
> +			return -1;
> +		}
> +
> +		if (use_mount) {
> +			mountpoint = alloca(strlen(get_tmpdir()) + strlen(DATADST_DIR_SUFFIX) + 1);
> +			sprintf(mountpoint, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX);
> +
> +			if (swupdate_mount(img->device, mountpoint, img->filesystem) != 0) {
> +				ERROR("Device %s with filesystem %s cannot be mounted",
> +					  img->device, img->filesystem);
> +				ret = -1;
> +				goto cleanup;
> +			}
> +		}
> +	}
> +
> +	if ((rdiff_state.base_file = fopen(base_file_filename, "rb+")) == NULL) {
> +		ERROR("%s cannot be opened for reading: %s", base_file_filename, strerror(errno));
> +		ret = -1;
> +		goto cleanup;
> +	}
> +
> +	if (!(rdiff_state.inbuf = malloc(RDIFF_BUFFER_SIZE))) {
> +		ERROR("Cannot allocate memory for rdiff input buffer.");
> +		ret = -1;
> +		goto cleanup;
> +	}
> +
> +	if (!(rdiff_state.outbuf = malloc(RDIFF_BUFFER_SIZE))) {
> +		ERROR("Cannot allocate memory for rdiff output buffer.");
> +		ret = -1;
> +		goto cleanup;
> +	}
> +
> +	rdiff_state.cpio_input_len = img->size;
> +
> +	int loglevelmap[] =
> +	{
> +		[OFF]        = RS_LOG_ERR,
> +		[ERRORLEVEL] = RS_LOG_ERR,
> +		[WARNLEVEL]  = RS_LOG_WARNING,
> +		[INFOLEVEL]  = RS_LOG_INFO,
> +		[DEBUGLEVEL] = RS_LOG_DEBUG,
> +		[TRACELEVEL] = RS_LOG_DEBUG,
> +	};
> +	rs_trace_set_level(loglevelmap[loglevel]);
> +	rs_trace_to(rdiff_log);
> +
> +	rdiff_state.job = rs_patch_begin(base_file_read_cb, rdiff_state.base_file);
> +	ret = copyfile(img->fdin,
> +			&rdiff_state,
> +			img->size,
> +			(unsigned long *)&img->offset,
> +			img->seek,
> +			0, /* no skip */
> +			img->compressed,
> +			&img->checksum,
> +			img->sha256,
> +			img->is_encrypted,
> +			apply_rdiff_chunk_cb);
> +
> +	if (rdiff_state.type == FILE_HANDLER) {
> +		struct stat stat_dest_file;
> +		if (fstat(fileno(rdiff_state.dest_file), &stat_dest_file) == -1) {
> +			ERROR("Cannot fstat file %s: %s", dest_file_filename, strerror(errno));
> +			ret = -1;
> +			goto cleanup;
> +		}
> +
> +		/*
> +		 * Most often $TMPDIR -- in which dest_file resides -- is a different
> +		 * filesystem (probably tmpfs) than that base_file resides in. Hence,
> +		 * substituting base_file by dest_file cross-filesystem via renameat()
> +		 * won't work. If dest_file and base_file are indeed in the same
> +		 * filesystem, metadata (uid, gid, mode, xattrs, acl, ...) has to be
> +		 * preserved after renameat(). This isn't worth the effort as Linux's
> +		 * sendfile() is fast, so copy the content.
> +		 */
> +		rdiff_state.base_file = freopen(NULL, "wb", rdiff_state.base_file);
> +		rdiff_state.dest_file = freopen(NULL, "rb", rdiff_state.dest_file);
> +		if ((rdiff_state.base_file == NULL) || (rdiff_state.dest_file == NULL)) {
> +			ERROR("Cannot reopen %s or %s: %s", dest_file_filename,
> +				  base_file_filename, strerror(errno));
> +			ret = -1;
> +			goto cleanup;
> +		}

What about if the file does not exist in the destination directory and
it is a new file ?

> +
> +#if defined(__FreeBSD__)
> +		(void)stat_dest_file;
> +		char buf[DFLTPHYS];
> +		int r;
> +		while ((r = read(fileno(rdiff_state.dest_file), buf, DFLTPHYS)) > 0) {
> +			if (write(fileno(rdiff_state.base_file), buf, r) != r) {
> +				ERROR("Write to %s failed.", base_file_filename);
> +				ret = -1;
> +				break;
> +			}
> +		}
> +		if (r < 0) {
> +			ERROR("Read from to %s failed.", dest_file_filename);
> +			ret = -1;
> +		}
> +#else
> +		if (sendfile(fileno(rdiff_state.base_file), fileno(rdiff_state.dest_file),
> +					 NULL, stat_dest_file.st_size) == -1) {
> +			ERROR("Cannot copy from %s to %s: %s", dest_file_filename,
> +			      base_file_filename, strerror(errno));
> +			ret = -1;
> +			goto cleanup;
> +		}
> +#endif
> +	}
> +
> +cleanup:
> +	free(rdiff_state.inbuf);
> +	free(rdiff_state.outbuf);
> +	if (rdiff_state.job != NULL) {
> +		(void)rs_job_free(rdiff_state.job);
> +	}
> +	if (rdiff_state.base_file != NULL) {
> +		if (fclose(rdiff_state.base_file) == EOF) {
> +			ERROR("Error while closing rdiff base: %s", strerror(errno));
> +		}
> +	}
> +	if (rdiff_state.dest_file != NULL) {
> +		if (fclose(rdiff_state.dest_file) == EOF) {
> +			ERROR("Error while closing rdiff destination: %s",
> +			      strerror(errno));
> +		}
> +	}
> +	if (rdiff_state.type == FILE_HANDLER) {
> +		if (unlink(dest_file_filename) == -1) {
> +			ERROR("Cannot delete temporary file %s, please clean up manually: %s",
> +			      dest_file_filename, strerror(errno));
> +		}
> +		if (use_mount == true) {
> +			swupdate_umount(mountpoint);
> +		}
> +	}
> +	return ret;
> +}
> +
> +__attribute__((constructor))
> +void rdiff_image_handler(void)
> +{
> +	register_handler("rdiffimage", apply_rdiff_patch, IMAGE_HANDLER, NULL);
> +}
> +
> +__attribute__((constructor))
> +void rdiff_file_handler(void)
> +{
> +	register_handler("rdiff", apply_rdiff_patch, FILE_HANDLER, NULL);
> +}
> 

Best regards,
Stefano

Hi Stefano,

> > The rdiff handler adds support for applying binary
> > delta patches generated by librsync's rdiff tool,
> > see http://librsync.sourcefrog.net
> 
> I would like to test and try myself the patchset, so I am not yet going
> to a full review. I think that the requirements are not fully
> established, I mean:
> 
> which is the required version for librsync ? Distros are coming with
> 0.9.7. There is an old OE recipe for the same version, too, but not
> merged in any current layer.
> If  there is not yet a layer with recipe, I agree to put it into
> meta-swupdate.
> 
> Anyway, last librsync release is 2.0.2. I am expecting to use a quite
> recent version for it.

Version 0.9.7 gives you a compiler warning about a different type
expected for the rs_trace_fn_t type as the `int level` parameter was
changed to be an `enum`. That's not going to break things but looks
ugly, in CI at least. I have developed and tested against version 2.0.2.
Version 2.0.1 was released one year ago, 2.0.0 three years ago. Both
*should* work looking at the changelogs but may yield this ugly warning.
I could test for those versions to find a minimal working version.
So far, as I've tested with version 2.0.2, that's the one.


> Do we have some values about the saving factor by using librsync ? IMHO
> this can help in case Software is continuosly update, that is moving
> from a version to the next one. But I guess that there is no advantage
> or even a disadvantage by moving from an arbitrary version. For the last
> one, I have imagined a pretty more complicated setup. Do you have some
> values, for example between a -krogoth release and a -sumo ? Or
> generally, how much can we save ?

Well, that's heavily dependent on your setup and your mileage may vary
drastically. In my tests, I got from saving up to ~95% to a negative
saving, i.e., overhead, of about ~10% attributed to the rdiff metadata.

The latter was, e.g. with a patch from /dev/zero to a /dev/urandom-
filled file of 4MB in size. The same is true for the patch between two
/dev/urandom-filled files. It doesn't get much better with larger
specimen sizes. Naturally, this is where binary diffs don't shine
particularly well :)

Roughly the former was with two mkosi-generated ArchLinux full-disk
images of 1,5G size which resulted in a patch of 33 MB, i.e., 2% of the
original size. So, to give a baseline number, this is the number that
can be attributed just to image generation noise with using mkosi and
ArchLinux. Add to this, e.g., the ~7 MB plus overhead of openssl and
you'll get a patch of ~3-4% or ~50 MB size. Again, your mileage may vary.
In general, binary diffs do shine, if you have (a) reproducible
builds (i.e., a deterministic build system, stable output order, ...)
and (b) small changes such as when applying a security update to a
library on an otherwise unchanged root file system. Think of read-only
root filesystems that get updated this way, frequently.


> I would like to understand which is the way to generate and deploy the
> delta. The thing is that only the device knows which is the running
> version, and just the build system knows the new one. 

Well, if the backend/build system has no knowledge whatsoever of the
image currently running on the device, it cannot sensibly compute a diff
that fits the device. Under this assumption, binary diffs don't really
make sense, in particular if they're precomputed at the server's side as
it's the case for rdiff. Then, maybe "online binary diffs" like done by
casync make more sense... The big plus of rdiff is that *if* you know
what's running on the device and that's not changed since your last
deployment/update, you can compute the diff on the server's side using
"cloud compute resources" and simply ship & apply it without having the
device to commit any work like checksumming the device file as it's the
case for "online binary diff" tools like casync and the like.
A backend may know what's running on the device, e.g., by the device
updating properties on the backend side on each update or some other
(side-channel) means. Fact is, for this to be beneficial, the backend
has to have current knowledge of the device in order to compute the
diff at the backend's side.


> The build server could generate any diff between any former release to
> the last one, but it looks like quite expensive. The result is then a
> set of SWUs, instead of a single SWU. Let's say, this was done in some
> way.

Well, this is expensive nonsense, agreed :)


> Now we have a set of SWUs and the device should pick up the correct one.
> How is this done ? In case of Hawkbit, we could have a set of Software
> Modules, but they are all parts of the distro and we have not (yet ?) a
> mechanism on the device to choose which one is downloaded.
> 
> In case of other interfaces, it is even more complictaed because this is
> let to the operator and cannot be done automatically. Let's think to the
> integrated Webserver: the device is in this case not the active
> operator, and it expects to get the right SWU. It can just reject or
> accept the installation. The operator must then choose the right SWU,
> the one that contains the diff between the running version and the new one.
> 
> So can you start to explain these requirements ? Even if we think that
> we want to add a single use case, we should then pretty document and
> explain this in documentation.

Yes, true. The most prominent use case would be to have read-only root
filesystems on the device of which versions the backend has current
knowledge. Take, e.g., the openssl security update use case example
sketched above. For such an update, the backend can compute the diff at
the backend's side and have it applied on the device.


> The next point I want to discuss is the application of this in case of
> compressed images. I guess the delta apply before compressing the
> images, right ? After compression I feel we have bad chances to get
> small deltas.

Yes, you do the diff/delta generation on the "raw" output images. If you
do this after compression, you are closer to the /dev/urandom case.

> 
> That means, after reading the example:
> 
> - we build a filesystem, let's say a "my-image-domething.[filesystem]".
> 
> - we run rdiff of it ==> new image type in OE ? A class in
> meta-swupdate, maybe ?
> I guess it is quite difficult without hacks to run this in OE, because a
> build must have the knowledge of previous builds (with different layer
> setup, maybe).

There's naturally some turning point after which it doesn't make sense
to ship a diff but a full image. The closer you are to the previous
build(s), the greater your benefit of using diffs. So, the more OE can
do reproducible builds, the more you'll benefit. For now, you build as
always and compute the diff between the two (partition) artifacts. This
is likely a post-processing step on the resulting (uncompressed) images
after the OE chain. After having calculated the diff, you can decide
whether it's small enough and hence worth shipping the diff or whether
the turning point has passed and a full image delivery is more appropriate.


> - result can be compressed, too ==> Swupdate decompress it before
> running the handler.

Yes, that and decryption is for free as I intentionally use the
copyfile() machinery, exactly for this benefit. That said, one can also
compile librsync to compress the chunk data...


> Back to the original thread by Paul, can we apply this to the kernel ?
> Kernel is already compressed as result of the "compile" task. And image
> format can vary, we could have a vmlinux (uncompressed), a zImage, or a
> uImage (both compressed) or a fitImage. Is it appliable to the kernel ?

Yes, via the rdiff{,file} handler but the result isn't that mindblowing :) 
There's simply too much that changes in between two kernels: A diff from
my custom 4.14-76 kernel to my custom 4.18-16 kernel -- both bzImages
and both 12MB in size -- is 9,7MB. A 4.18-14 to 4.18-16 diff is 9,2MB.
Here, compression butchers our benefit as it's unstable in order.
Uncompressed, the kernels are 33MB in size and the diff is 19MB, i.e.,
~50%. That said, for patching single binaries only, one would be better
off using something like Chromium's Courgette:
https://www.chromium.org/developers/design-documents/software-updates-courgette
As said, to get the most out of rdiff binary patches, you need as much
reproducibility with stable output order as you can get. A
compiler+compression pass usually doesn't satisfy this. It really
depends on the mixture: Applied to a root filesystem with, say, just
one library changed, you get very decent results for rdiff patches.


> > [...]
> > +
> > +rdiff handler
> > +-------------
> > +
> > +The rdiff handler adds support for applying binary delta patches generated by
> > +`librsync's <http://librsync.sourcefrog.net/>`_ rdiff tool.
> > +
> > +First, create the signature of the original (base) file via
> > +``rdiff signature <basefile> <signaturefile>``.
> > +Then, create the delta file (i.e., patch) from the original base file to the target
> > +file via ``rdiff delta <signaturefile> <targetfile> <deltafile>``.
> > +The ``<deltafile>`` is the artifact to be applied via this handler on the device.
> > +Essentially, it mimics running ``rdiff patch <basefile> <deltafile> <targetfile>``
> > +on the device. Naturally for patches, the very same ``<basefile>`` has to be used
> > +for creating as well as for applying the patch to.
> > +
> > +This handler registers itself for handling files and images.
> > +An exemplary sw-description fragment for the files section is
> > +
> > +::
> > +
> > +    files: (
> > +        {
> > +            type = "rdiff"
> 
> If you use rdiffimage, maybe we have to set it as rdifffile.

I resisted because of the 3 'f's there, but OK, fine with me :)

> > [...]
> > +            filename = "file.rdiff.delta";
> > +            path = "/usr/bin/file";
> > +        }
> > +    );
> 
> Does it work with "installed-directly" ?

Hm, haven't tested it. As this handler relies on copyfile() to deliver
its input and thereby driving it, I don't see a reason why it should not
work in streaming mode but again, that's not yet tested.


> > [...]
> > +
> > +#define ASSERT(expr, failret) \
> > +	if (expr) { \
> > +	} else { \
> > +		ERROR("Assertion violated: %s.", #expr); \
> > +		return failret; \
> > +	}
> > +
> 
> do we need this ? what about "assert" in most code ?

Well, I don't want to abort the process in case an assertion doesn't
hold. Printing an error through SWUpdate's notification framework and
aborting the installation seemed sufficient to me, considering the use
case of SWUpdate running as daemon. I could replace this by a traditional
assert() and then have, e.g., systemd respawn SWUpdate on a failed
assert() abortion, granted. I don't have a strong opinion for one over
the other. What's your opinion on this?


> > [...]
> > +static int apply_rdiff_patch(struct img_type *img,
> > +							 void __attribute__((__unused__)) * data)
> > +{
> > +	int ret = 0;
> > +
> > +	struct rdiff_t rdiff_state = {};
> > +	rdiff_state.type =
> > +	    strcmp(img->type, "rdiffimage") == 0 ? IMAGE_HANDLER : FILE_HANDLER;
> > +
> > +	char *mountpoint = NULL;
> > +	bool use_mount = (strlen(img->device) && strlen(img->filesystem)) ? true : false;
> > +
> > +	char *base_file_filename = NULL;
> > +	char *dest_file_filename = NULL;
> > +
> > +	if (rdiff_state.type == IMAGE_HANDLER) {
> > +		if (img->seek) {
> > +			/*
> > +			 * img->seek mandates copyfile()'s out parameter to be a fd, it
> > +			 * isn't. So, the seek option is invalid for the rdiff handler.
> > +			 * */
> 
> Not clear, as  rdiff_state.dest_file is a fd, and a seek() on this fd
> should be possible. What am I missing ?

Yes, true, but.. as called by apply_rdiff_patch(), copyfile()'s out
parameter is not a file descriptor but &rdiff_state. The (seek) test in
copyfile() assumes it to be an fd, which is isn't. 
Of course, one can support seeking in the destination (device) file, but
I haven't imagined a good reason to do so since skipping over
unchanged content is handled quite well by the rdiff algorithm.
That said, we may of course change this and implement it...


> > +			ERROR("Option 'seek' is not supported for rdiff.");
> > +			return -1;
> > +		}
> > +
> > [...]
> > +	rdiff_state.job = rs_patch_begin(base_file_read_cb, rdiff_state.base_file);
> > +	ret = copyfile(img->fdin,
> > +			&rdiff_state,
> > +			img->size,
> > +			(unsigned long *)&img->offset,
> > +			img->seek,
> > +			0, /* no skip */
> > +			img->compressed,
> > +			&img->checksum,
> > +			img->sha256,
> > +			img->is_encrypted,
> > +			apply_rdiff_chunk_cb);
> > +
> > +	if (rdiff_state.type == FILE_HANDLER) {
> > +		struct stat stat_dest_file;
> > +		if (fstat(fileno(rdiff_state.dest_file), &stat_dest_file) == -1) {
> > +			ERROR("Cannot fstat file %s: %s", dest_file_filename, strerror(errno));
> > +			ret = -1;
> > +			goto cleanup;
> > +		}
> > +
> > +		/*
> > +		 * Most often $TMPDIR -- in which dest_file resides -- is a different
> > +		 * filesystem (probably tmpfs) than that base_file resides in. Hence,
> > +		 * substituting base_file by dest_file cross-filesystem via renameat()
> > +		 * won't work. If dest_file and base_file are indeed in the same
> > +		 * filesystem, metadata (uid, gid, mode, xattrs, acl, ...) has to be
> > +		 * preserved after renameat(). This isn't worth the effort as Linux's
> > +		 * sendfile() is fast, so copy the content.
> > +		 */
> > +		rdiff_state.base_file = freopen(NULL, "wb", rdiff_state.base_file);
> > +		rdiff_state.dest_file = freopen(NULL, "rb", rdiff_state.dest_file);
> > +		if ((rdiff_state.base_file == NULL) || (rdiff_state.dest_file == NULL)) {
> > +			ERROR("Cannot reopen %s or %s: %s", dest_file_filename,
> > +				  base_file_filename, strerror(errno));
> > +			ret = -1;
> > +			goto cleanup;
> > +		}
> 
> What about if the file does not exist in the destination directory and
> it is a new file ?

Good point, I've also thought about this. In this case, there's nothing
to apply a patch against, i.e., the patch creates the file and hence the
patch is naturally larger than the file itself due to rdiff metadata
overhead. Then, the binary diff sweet spot is missed by large and it
doesn't make sense to use patches but the file should be created by
installing it via the files:{} section first and then, possibly, be
later patched. For consistency, I could implement this as well, sure...



Kind regards,
   Christian

Hi Christian,

On 12/11/18 16:13, Christian Storm wrote:
> Hi Stefano,
> 
>>> The rdiff handler adds support for applying binary
>>> delta patches generated by librsync's rdiff tool,
>>> see http://librsync.sourcefrog.net
>>
>> I would like to test and try myself the patchset, so I am not yet going
>> to a full review. I think that the requirements are not fully
>> established, I mean:
>>
>> which is the required version for librsync ? Distros are coming with
>> 0.9.7. There is an old OE recipe for the same version, too, but not
>> merged in any current layer.
>> If  there is not yet a layer with recipe, I agree to put it into
>> meta-swupdate.
>>
>> Anyway, last librsync release is 2.0.2. I am expecting to use a quite
>> recent version for it.
> 
> Version 0.9.7 gives you a compiler warning about a different type
> expected for the rs_trace_fn_t type as the `int level` parameter was
> changed to be an `enum`. That's not going to break things but looks
> ugly, in CI at least. I have developed and tested against version 2.0.2.
> Version 2.0.1 was released one year ago, 2.0.0 three years ago. Both
> *should* work looking at the changelogs but may yield this ugly warning.
> I could test for those versions to find a minimal working version.
> So far, as I've tested with version 2.0.2, that's the one.

As we start to add a new feature, we can set that librsync > 2.0.2 is
needed, and former releases are at least untested.

I will take a look to integrate it in meta-swupdate.

> 
> 
>> Do we have some values about the saving factor by using librsync ? IMHO
>> this can help in case Software is continuosly update, that is moving
>> from a version to the next one. But I guess that there is no advantage
>> or even a disadvantage by moving from an arbitrary version. For the last
>> one, I have imagined a pretty more complicated setup. Do you have some
>> values, for example between a -krogoth release and a -sumo ? Or
>> generally, how much can we save ?
> 
> Well, that's heavily dependent on your setup and your mileage may vary
> drastically. In my tests, I got from saving up to ~95% to a negative
> saving, i.e., overhead, of about ~10% attributed to the rdiff metadata.
> 

Ok, this is also my expectations.

> The latter was, e.g. with a patch from /dev/zero to a /dev/urandom-
> filled file of 4MB in size. The same is true for the patch between two
> /dev/urandom-filled files. It doesn't get much better with larger
> specimen sizes. Naturally, this is where binary diffs don't shine
> particularly well :)

Right.

> 
> Roughly the former was with two mkosi-generated ArchLinux full-disk
> images of 1,5G size which resulted in a patch of 33 MB, i.e., 2% of the
> original size. So, to give a baseline number, this is the number that
> can be attributed just to image generation noise with using mkosi and
> ArchLinux. Add to this, e.g., the ~7 MB plus overhead of openssl and
> you'll get a patch of ~3-4% or ~50 MB size. Again, your mileage may vary.
> In general, binary diffs do shine, if you have (a) reproducible
> builds (i.e., a deterministic build system, stable output order, ...)
> and (b) small changes such as when applying a security update to a
> library on an otherwise unchanged root file system. Think of read-only
> root filesystems that get updated this way, frequently.

Agree. This should also explained into the documentation. The use case
where we get a real advantage is when we have a ro filesystem and we
want to fix a few files, like in case of security fixes, few libraries,
and so on. It does not want to try to perform a delta when two
filesystem differ substantially, for example between two Yocto releases.

> 
> 
>> I would like to understand which is the way to generate and deploy the
>> delta. The thing is that only the device knows which is the running
>> version, and just the build system knows the new one. 
> 
> Well, if the backend/build system has no knowledge whatsoever of the
> image currently running on the device, it cannot sensibly compute a diff
> that fits the device.

This is my assumption, too.

> Under this assumption, binary diffs don't really
> make sense, in particular if they're precomputed at the server's side as
> it's the case for rdiff. Then, maybe "online binary diffs" like done by
> casync make more sense...

This is *exactly* my conclusion when I took a look how to add binary
deltas to SWUpdate and at the end I land to think about top integrate
casync. Of course, casync requires some changes to let it couple with
SWUpdate. An advantage is that the server could be unaware of it, and
even it does not need to know which version of software is running on
the device.

As I have already checked deeper, IMHO adding a "casync-like handler"
requires also some further changes, because casync thinks to run with
full rights as root and this breaks privilege separation when accessing
network. Nevertheless, casync should more used as librsync is, that is a
library instead of tool. So some surrounding code / script to let casync
working is not enough, but I think is doable. I have proposed this as
draft to a couple of customers of mine, but there is not (yet ?) enough
interest to drive an implementation. The librsync approach is a more
simple approach that it is suitable for some use cases, as we have stated.


> The big plus of rdiff is that *if* you know
> what's running on the device and that's not changed since your last
> deployment/update, you can compute the diff on the server's side using
> "cloud compute resources" and simply ship & apply it without having the
> device to commit any work like checksumming the device file as it's the
> case for "online binary diff" tools like casync and the like.

Yes, agree. If we exactly know which is the version on the device and
the resulting release, we can compute offline the diff-SWU.

> A backend may know what's running on the device, e.g., by the device
> updating properties on the backend side on each update or some other
> (side-channel) means.

If you are thinking about Hawkbit, it has not yet this feature. The only
way for the device to inform the server is via the configData URL, but
the posted dictionary is not evaluated by Hawkbit core. A custom process
using the Hawkbit's management interface should be implemented to read
device data and to provide (or build) the SWU with the diff.
This can be more difficult if a secure boot and a verification of the
root filesystem is implemented. In fact, in the last case even the
deployment server that generates the diff must be able to sign the
images, not only the build server (that one running Yocto / Buildroot /..).

> Fact is, for this to be beneficial, the backend
> has to have current knowledge of the device in order to compute the
> diff at the backend's side.

Thanks for explanation - I vote to have this sentence in the documentation.

> 
> 
>> The build server could generate any diff between any former release to
>> the last one, but it looks like quite expensive. The result is then a
>> set of SWUs, instead of a single SWU. Let's say, this was done in some
>> way.
> 
> Well, this is expensive nonsense, agreed :)

Right.

> 
> 
>> Now we have a set of SWUs and the device should pick up the correct one.
>> How is this done ? In case of Hawkbit, we could have a set of Software
>> Modules, but they are all parts of the distro and we have not (yet ?) a
>> mechanism on the device to choose which one is downloaded.
>>
>> In case of other interfaces, it is even more complictaed because this is
>> let to the operator and cannot be done automatically. Let's think to the
>> integrated Webserver: the device is in this case not the active
>> operator, and it expects to get the right SWU. It can just reject or
>> accept the installation. The operator must then choose the right SWU,
>> the one that contains the diff between the running version and the new one.
>>
>> So can you start to explain these requirements ? Even if we think that
>> we want to add a single use case, we should then pretty document and
>> explain this in documentation.
> 
> Yes, true. The most prominent use case would be to have read-only root
> filesystems on the device of which versions the backend has current
> knowledge. 

ok

> Take, e.g., the openssl security update use case example
> sketched above. For such an update, the backend can compute the diff at
> the backend's side and have it applied on the device.

Thanks, use case is clear.

> 
> 
>> The next point I want to discuss is the application of this in case of
>> compressed images. I guess the delta apply before compressing the
>> images, right ? After compression I feel we have bad chances to get
>> small deltas.
> 
> Yes, you do the diff/delta generation on the "raw" output images. If you
> do this after compression, you are closer to the /dev/urandom case.

ok, got it.

> 
>>
>> That means, after reading the example:
>>
>> - we build a filesystem, let's say a "my-image-domething.[filesystem]".
>>
>> - we run rdiff of it ==> new image type in OE ? A class in
>> meta-swupdate, maybe ?
>> I guess it is quite difficult without hacks to run this in OE, because a
>> build must have the knowledge of previous builds (with different layer
>> setup, maybe).
> 
> There's naturally some turning point after which it doesn't make sense
> to ship a diff but a full image. The closer you are to the previous
> build(s), the greater your benefit of using diffs. So, the more OE can
> do reproducible builds, the more you'll benefit. For now, you build as
> always and compute the diff between the two (partition) artifacts. This
> is likely a post-processing step on the resulting (uncompressed) images
> after the OE chain.

Ok, this can be done. Let's say we generate a new format ".diff", and we
can have a "diff.gz", too.

> After having calculated the diff, you can decide
> whether it's small enough and hence worth shipping the diff or whether
> the turning point has passed and a full image delivery is more appropriate.

ok, it makes sense

> 
> 
>> - result can be compressed, too ==> Swupdate decompress it before
>> running the handler.
> 
> Yes, that and decryption is for free as I intentionally use the
> copyfile() machinery, exactly for this benefit.

Fully agree. Handler should use the copyfile(), because SWUpdate will
take care of most details and the whole pipeline (decompression /
decryption) is still valid.

> That said, one can also
> compile librsync to compress the chunk data...

No, this is not our use case.

> 
> 
>> Back to the original thread by Paul, can we apply this to the kernel ?
>> Kernel is already compressed as result of the "compile" task. And image
>> format can vary, we could have a vmlinux (uncompressed), a zImage, or a
>> uImage (both compressed) or a fitImage. Is it appliable to the kernel ?
> 
> Yes, via the rdiff{,file} handler but the result isn't that mindblowing :) 
> There's simply too much that changes in between two kernels: A diff from
> my custom 4.14-76 kernel to my custom 4.18-16 kernel -- both bzImages
> and both 12MB in size -- is 9,7MB. A 4.18-14 to 4.18-16 diff is 9,2MB.

I will say that it does not make sense to provide a diff.

> Here, compression butchers our benefit as it's unstable in order.
> Uncompressed, the kernels are 33MB in size and the diff is 19MB, i.e.,
> ~50%. That said, for patching single binaries only, one would be better
> off using something like Chromium's Courgette:
> https://www.chromium.org/developers/design-documents/software-updates-courgette

I know this, but this is derived from bsdiff to better make diff on ELF
files. We cannot use it on any kind of files.

> As said, to get the most out of rdiff binary patches, you need as much
> reproducibility with stable output order as you can get. A
> compiler+compression pass usually doesn't satisfy this. It really
> depends on the mixture: Applied to a root filesystem with, say, just
> one library changed, you get very decent results for rdiff patches.

Ok

> 
> 
>>> [...]
>>> +
>>> +rdiff handler
>>> +-------------
>>> +
>>> +The rdiff handler adds support for applying binary delta patches generated by
>>> +`librsync's <http://librsync.sourcefrog.net/>`_ rdiff tool.
>>> +
>>> +First, create the signature of the original (base) file via
>>> +``rdiff signature <basefile> <signaturefile>``.
>>> +Then, create the delta file (i.e., patch) from the original base file to the target
>>> +file via ``rdiff delta <signaturefile> <targetfile> <deltafile>``.
>>> +The ``<deltafile>`` is the artifact to be applied via this handler on the device.
>>> +Essentially, it mimics running ``rdiff patch <basefile> <deltafile> <targetfile>``
>>> +on the device. Naturally for patches, the very same ``<basefile>`` has to be used
>>> +for creating as well as for applying the patch to.
>>> +
>>> +This handler registers itself for handling files and images.
>>> +An exemplary sw-description fragment for the files section is
>>> +
>>> +::
>>> +
>>> +    files: (
>>> +        {
>>> +            type = "rdiff"
>>
>> If you use rdiffimage, maybe we have to set it as rdifffile.
> 
> I resisted because of the 3 'f's there, but OK, fine with me :)

I do not like the 3 'f' too, but we have "rawfile". I think adding
"file" makes names consistent.

> 
>>> [...]
>>> +            filename = "file.rdiff.delta";
>>> +            path = "/usr/bin/file";
>>> +        }
>>> +    );
>>
>> Does it work with "installed-directly" ?
> 
> Hm, haven't tested it. As this handler relies on copyfile() to deliver
> its input and thereby driving it, I don't see a reason why it should not
> work in streaming mode but again, that's not yet tested.

ok, fine.

> 
> 
>>> [...]
>>> +
>>> +#define ASSERT(expr, failret) \
>>> +	if (expr) { \
>>> +	} else { \
>>> +		ERROR("Assertion violated: %s.", #expr); \
>>> +		return failret; \
>>> +	}
>>> +
>>
>> do we need this ? what about "assert" in most code ?
> 
> Well, I don't want to abort the process in case an assertion doesn't
> hold. Printing an error through SWUpdate's notification framework and
> aborting the installation seemed sufficient to me, considering the use
> case of SWUpdate running as daemon. I could replace this by a traditional
> assert() and then have, e.g., systemd respawn SWUpdate on a failed
> assert() abortion, granted. I don't have a strong opinion for one over
> the other. What's your opinion on this?

No, the let's assert() as it is, because this is the standard behavior.
Just change the name of this macro with whatever you want that does not
lead to assert and move it into util.h

> 
> 
>>> [...]
>>> +static int apply_rdiff_patch(struct img_type *img,
>>> +							 void __attribute__((__unused__)) * data)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	struct rdiff_t rdiff_state = {};
>>> +	rdiff_state.type =
>>> +	    strcmp(img->type, "rdiffimage") == 0 ? IMAGE_HANDLER : FILE_HANDLER;
>>> +
>>> +	char *mountpoint = NULL;
>>> +	bool use_mount = (strlen(img->device) && strlen(img->filesystem)) ? true : false;
>>> +
>>> +	char *base_file_filename = NULL;
>>> +	char *dest_file_filename = NULL;
>>> +
>>> +	if (rdiff_state.type == IMAGE_HANDLER) {
>>> +		if (img->seek) {
>>> +			/*
>>> +			 * img->seek mandates copyfile()'s out parameter to be a fd, it
>>> +			 * isn't. So, the seek option is invalid for the rdiff handler.
>>> +			 * */
>>
>> Not clear, as  rdiff_state.dest_file is a fd, and a seek() on this fd
>> should be possible. What am I missing ?
> 
> Yes, true, but.. as called by apply_rdiff_patch(), copyfile()'s out
> parameter is not a file descriptor but &rdiff_state. The (seek) test in
> copyfile() assumes it to be an fd, which is isn't.

ok, thanks.

> Of course, one can support seeking in the destination (device) file, but
> I haven't imagined a good reason to do so since skipping over
> unchanged content is handled quite well by the rdiff algorithm.
> That said, we may of course change this and implement it...

I just needed to understand.

> 
> 
>>> +			ERROR("Option 'seek' is not supported for rdiff.");
>>> +			return -1;
>>> +		}
>>> +
>>> [...]
>>> +	rdiff_state.job = rs_patch_begin(base_file_read_cb, rdiff_state.base_file);
>>> +	ret = copyfile(img->fdin,
>>> +			&rdiff_state,
>>> +			img->size,
>>> +			(unsigned long *)&img->offset,
>>> +			img->seek,
>>> +			0, /* no skip */
>>> +			img->compressed,
>>> +			&img->checksum,
>>> +			img->sha256,
>>> +			img->is_encrypted,
>>> +			apply_rdiff_chunk_cb);
>>> +
>>> +	if (rdiff_state.type == FILE_HANDLER) {
>>> +		struct stat stat_dest_file;
>>> +		if (fstat(fileno(rdiff_state.dest_file), &stat_dest_file) == -1) {
>>> +			ERROR("Cannot fstat file %s: %s", dest_file_filename, strerror(errno));
>>> +			ret = -1;
>>> +			goto cleanup;
>>> +		}
>>> +
>>> +		/*
>>> +		 * Most often $TMPDIR -- in which dest_file resides -- is a different
>>> +		 * filesystem (probably tmpfs) than that base_file resides in. Hence,
>>> +		 * substituting base_file by dest_file cross-filesystem via renameat()
>>> +		 * won't work. If dest_file and base_file are indeed in the same
>>> +		 * filesystem, metadata (uid, gid, mode, xattrs, acl, ...) has to be
>>> +		 * preserved after renameat(). This isn't worth the effort as Linux's
>>> +		 * sendfile() is fast, so copy the content.
>>> +		 */
>>> +		rdiff_state.base_file = freopen(NULL, "wb", rdiff_state.base_file);
>>> +		rdiff_state.dest_file = freopen(NULL, "rb", rdiff_state.dest_file);
>>> +		if ((rdiff_state.base_file == NULL) || (rdiff_state.dest_file == NULL)) {
>>> +			ERROR("Cannot reopen %s or %s: %s", dest_file_filename,
>>> +				  base_file_filename, strerror(errno));
>>> +			ret = -1;
>>> +			goto cleanup;
>>> +		}
>>
>> What about if the file does not exist in the destination directory and
>> it is a new file ?
> 
> Good point, I've also thought about this. In this case, there's nothing
> to apply a patch against, i.e., the patch creates the file and hence the
> patch is naturally larger than the file itself due to rdiff metadata
> overhead. Then, the binary diff sweet spot is missed by large and it
> doesn't make sense to use patches but the file should be created by
> installing it via the files:{} section first and then, possibly, be
> later patched. For consistency, I could implement this as well, sure...

ok

Best regards,
Stefano

Hi Stefano,

> > Roughly the former was with two mkosi-generated ArchLinux full-disk
> > images of 1,5G size which resulted in a patch of 33 MB, i.e., 2% of the
> > original size. So, to give a baseline number, this is the number that
> > can be attributed just to image generation noise with using mkosi and
> > ArchLinux. Add to this, e.g., the ~7 MB plus overhead of openssl and
> > you'll get a patch of ~3-4% or ~50 MB size. Again, your mileage may vary.
> > In general, binary diffs do shine, if you have (a) reproducible
> > builds (i.e., a deterministic build system, stable output order, ...)
> > and (b) small changes such as when applying a security update to a
> > library on an otherwise unchanged root file system. Think of read-only
> > root filesystems that get updated this way, frequently.
> 
> Agree. This should also explained into the documentation. The use case
> where we get a real advantage is when we have a ro filesystem and we
> want to fix a few files, like in case of security fixes, few libraries,
> and so on. It does not want to try to perform a delta when two
> filesystem differ substantially, for example between two Yocto releases.

OK, I'll update the documentation to mention this as a prominent use case.


> >> I would like to understand which is the way to generate and deploy the
> >> delta. The thing is that only the device knows which is the running
> >> version, and just the build system knows the new one. 
> > 
> > Well, if the backend/build system has no knowledge whatsoever of the
> > image currently running on the device, it cannot sensibly compute a diff
> > that fits the device.
> 
> This is my assumption, too.
> 
> > Under this assumption, binary diffs don't really
> > make sense, in particular if they're precomputed at the server's side as
> > it's the case for rdiff. Then, maybe "online binary diffs" like done by
> > casync make more sense...
> 
> This is *exactly* my conclusion when I took a look how to add binary
> deltas to SWUpdate and at the end I land to think about top integrate
> casync. Of course, casync requires some changes to let it couple with
> SWUpdate. An advantage is that the server could be unaware of it, and
> even it does not need to know which version of software is running on
> the device.
> 
> As I have already checked deeper, IMHO adding a "casync-like handler"
> requires also some further changes, because casync thinks to run with
> full rights as root and this breaks privilege separation when accessing
> network. Nevertheless, casync should more used as librsync is, that is a
> library instead of tool. So some surrounding code / script to let casync
> working is not enough, but I think is doable. I have proposed this as
> draft to a couple of customers of mine, but there is not (yet ?) enough
> interest to drive an implementation. The librsync approach is a more
> simple approach that it is suitable for some use cases, as we have stated.

Yes, agreed. The big benefit of librsync in contrast to casync and the
like tools is that the diff can be computed at the server's side with
the device having nothing to know and do except for applying the patch.
Using casync, you have to have the signature calculation done on both
ends, i.e., the device has to calculate the signature of its
/dev/mmcblk0p1 so that casync can find the difference. Granted, you can
store the signature calculation results at the expense of disk space
"wasted". This is a burden put on the device which it may not be up to
if it's short on resources.
So, I think there's room for both options, librsync as well as casync,
depending on the use case, as always :)

Speaking of casync, there's also zchunk (https://github.com/zchunk/zchunk)
which is worth having a look at. Both internally work alike but have
different use cases. casync is a bit "too much" for me, that's why I
found zchunk appealing although not perfectly fitting the partition
binary diff use case.

casync will eventually become a library of some sort if I read the TODO
list correctly, so maybe we just have to wait for it to become more
embedding friendly :)


> > The big plus of rdiff is that *if* you know
> > what's running on the device and that's not changed since your last
> > deployment/update, you can compute the diff on the server's side using
> > "cloud compute resources" and simply ship & apply it without having the
> > device to commit any work like checksumming the device file as it's the
> > case for "online binary diff" tools like casync and the like.
> 
> Yes, agree. If we exactly know which is the version on the device and
> the resulting release, we can compute offline the diff-SWU.
> 
> > A backend may know what's running on the device, e.g., by the device
> > updating properties on the backend side on each update or some other
> > (side-channel) means.
> 
> If you are thinking about Hawkbit, it has not yet this feature. The only
> way for the device to inform the server is via the configData URL, but
> the posted dictionary is not evaluated by Hawkbit core. A custom process
> using the Hawkbit's management interface should be implemented to read
> device data and to provide (or build) the SWU with the diff.

Yes, true, but there are others like mender.io, Siemens MindSphere, ... :)


> This can be more difficult if a secure boot and a verification of the
> root filesystem is implemented. In fact, in the last case even the
> deployment server that generates the diff must be able to sign the
> images, not only the build server (that one running Yocto / Buildroot /..).

Then you open a whole different can of worms. Having secure boot
requires more integration works and close cooperation between the image
generation and application by SWUpdate...


> > Fact is, for this to be beneficial, the backend
> > has to have current knowledge of the device in order to compute the
> > diff at the backend's side.
> 
> Thanks for explanation - I vote to have this sentence in the documentation.

Agreed. I will update the documentation once the review is positive :)


> >> [...]
> >> The next point I want to discuss is the application of this in case of
> >> compressed images. I guess the delta apply before compressing the
> >> images, right ? After compression I feel we have bad chances to get
> >> small deltas.
> > 
> > Yes, you do the diff/delta generation on the "raw" output images. If you
> > do this after compression, you are closer to the /dev/urandom case.
> 
> ok, got it.
> 
> > 
> >>
> >> That means, after reading the example:
> >>
> >> - we build a filesystem, let's say a "my-image-domething.[filesystem]".
> >>
> >> - we run rdiff of it ==> new image type in OE ? A class in
> >> meta-swupdate, maybe ?
> >> I guess it is quite difficult without hacks to run this in OE, because a
> >> build must have the knowledge of previous builds (with different layer
> >> setup, maybe).
> > 
> > There's naturally some turning point after which it doesn't make sense
> > to ship a diff but a full image. The closer you are to the previous
> > build(s), the greater your benefit of using diffs. So, the more OE can
> > do reproducible builds, the more you'll benefit. For now, you build as
> > always and compute the diff between the two (partition) artifacts. This
> > is likely a post-processing step on the resulting (uncompressed) images
> > after the OE chain.
> 
> Ok, this can be done. Let's say we generate a new format ".diff", and we
> can have a "diff.gz", too.

This would be an option, true.


> >> [...]
> >> - result can be compressed, too ==> Swupdate decompress it before
> >> running the handler.
> > 
> > Yes, that and decryption is for free as I intentionally use the
> > copyfile() machinery, exactly for this benefit.
> 
> Fully agree. Handler should use the copyfile(), because SWUpdate will
> take care of most details and the whole pipeline (decompression /
> decryption) is still valid.
>
> > That said, one can also
> > compile librsync to compress the chunk data...
> 
> No, this is not our use case.

Well, actually, if you compile librsync to have this feature and
generate a diff having this feature, SWUpdate won't notice :)
As SWUpdate does compression, I don't see a need for it but if you
want to do it inside librsync, there's nothing stopping you..


> >> Back to the original thread by Paul, can we apply this to the kernel ?
> >> Kernel is already compressed as result of the "compile" task. And image
> >> format can vary, we could have a vmlinux (uncompressed), a zImage, or a
> >> uImage (both compressed) or a fitImage. Is it appliable to the kernel ?
> > 
> > Yes, via the rdiff{,file} handler but the result isn't that mindblowing :) 
> > There's simply too much that changes in between two kernels: A diff from
> > my custom 4.14-76 kernel to my custom 4.18-16 kernel -- both bzImages
> > and both 12MB in size -- is 9,7MB. A 4.18-14 to 4.18-16 diff is 9,2MB.
> 
> I will say that it does not make sense to provide a diff.

Generally not, agreed. If you're after each and every byte transferred,
however, it could.


> > Here, compression butchers our benefit as it's unstable in order.
> > Uncompressed, the kernels are 33MB in size and the diff is 19MB, i.e.,
> > ~50%. That said, for patching single binaries only, one would be better
> > off using something like Chromium's Courgette:
> > https://www.chromium.org/developers/design-documents/software-updates-courgette
> 
> I know this, but this is derived from bsdiff to better make diff on ELF
> files. We cannot use it on any kind of files.

Not on any kind but for ELFs and ELF-like files like the kernel. If
you're adventurous, one can provide a handler specifically for patching
the kernel using Courgette.


> > As said, to get the most out of rdiff binary patches, you need as much
> > reproducibility with stable output order as you can get. A
> > compiler+compression pass usually doesn't satisfy this. It really
> > depends on the mixture: Applied to a root filesystem with, say, just
> > one library changed, you get very decent results for rdiff patches.
> 
> Ok
> 
> > 
> > 
> >>> [...]
> >>> +
> >>> +rdiff handler
> >>> +-------------
> >>> +
> >>> +The rdiff handler adds support for applying binary delta patches generated by
> >>> +`librsync's <http://librsync.sourcefrog.net/>`_ rdiff tool.
> >>> +
> >>> +First, create the signature of the original (base) file via
> >>> +``rdiff signature <basefile> <signaturefile>``.
> >>> +Then, create the delta file (i.e., patch) from the original base file to the target
> >>> +file via ``rdiff delta <signaturefile> <targetfile> <deltafile>``.
> >>> +The ``<deltafile>`` is the artifact to be applied via this handler on the device.
> >>> +Essentially, it mimics running ``rdiff patch <basefile> <deltafile> <targetfile>``
> >>> +on the device. Naturally for patches, the very same ``<basefile>`` has to be used
> >>> +for creating as well as for applying the patch to.
> >>> +
> >>> +This handler registers itself for handling files and images.
> >>> +An exemplary sw-description fragment for the files section is
> >>> +
> >>> +::
> >>> +
> >>> +    files: (
> >>> +        {
> >>> +            type = "rdiff"
> >>
> >> If you use rdiffimage, maybe we have to set it as rdifffile.
> > 
> > I resisted because of the 3 'f's there, but OK, fine with me :)
> 
> I do not like the 3 'f' too, but we have "rawfile". I think adding
> "file" makes names consistent.

OK, then what about "rdiff_file" and "rdiff_image"? In the end, I don't
care that much and I'd like to name it as you propose :)


> >>> [...]
> >>> +
> >>> +#define ASSERT(expr, failret) \
> >>> +	if (expr) { \
> >>> +	} else { \
> >>> +		ERROR("Assertion violated: %s.", #expr); \
> >>> +		return failret; \
> >>> +	}
> >>> +
> >>
> >> do we need this ? what about "assert" in most code ?
> > 
> > Well, I don't want to abort the process in case an assertion doesn't
> > hold. Printing an error through SWUpdate's notification framework and
> > aborting the installation seemed sufficient to me, considering the use
> > case of SWUpdate running as daemon. I could replace this by a traditional
> > assert() and then have, e.g., systemd respawn SWUpdate on a failed
> > assert() abortion, granted. I don't have a strong opinion for one over
> > the other. What's your opinion on this?
> 
> No, the let's assert() as it is, because this is the standard behavior.
> Just change the name of this macro with whatever you want that does not
> lead to assert and move it into util.h

OK, then we're at naming. I do see that this can be of general utility,
so putting it in util.h makes some sense. What about naming it TEST() or
TEST_AND_LOG()?


> >>> [...]
> >>> +static int apply_rdiff_patch(struct img_type *img,
> >>> +							 void __attribute__((__unused__)) * data)
> >>> +{
> >>> +	int ret = 0;
> >>> +
> >>> +	struct rdiff_t rdiff_state = {};
> >>> +	rdiff_state.type =
> >>> +	    strcmp(img->type, "rdiffimage") == 0 ? IMAGE_HANDLER : FILE_HANDLER;
> >>> +
> >>> +	char *mountpoint = NULL;
> >>> +	bool use_mount = (strlen(img->device) && strlen(img->filesystem)) ? true : false;
> >>> +
> >>> +	char *base_file_filename = NULL;
> >>> +	char *dest_file_filename = NULL;
> >>> +
> >>> +	if (rdiff_state.type == IMAGE_HANDLER) {
> >>> +		if (img->seek) {
> >>> +			/*
> >>> +			 * img->seek mandates copyfile()'s out parameter to be a fd, it
> >>> +			 * isn't. So, the seek option is invalid for the rdiff handler.
> >>> +			 * */
> >>
> >> Not clear, as  rdiff_state.dest_file is a fd, and a seek() on this fd
> >> should be possible. What am I missing ?
> > 
> > Yes, true, but.. as called by apply_rdiff_patch(), copyfile()'s out
> > parameter is not a file descriptor but &rdiff_state. The (seek) test in
> > copyfile() assumes it to be an fd, which is isn't.
> 
> ok, thanks.
> 
> > Of course, one can support seeking in the destination (device) file, but
> > I haven't imagined a good reason to do so since skipping over
> > unchanged content is handled quite well by the rdiff algorithm.
> > That said, we may of course change this and implement it...
> 
> I just needed to understand.

Well, I haven't seen a need for seeking. If you do have an example, we
can implement it. Otherwise, it may stay as is and be documented, that's
indeed missing currently.
Looks like lot of documentation works ahead :)


> >>> +			ERROR("Option 'seek' is not supported for rdiff.");
> >>> +			return -1;
> >>> +		}
> >>> +
> >>> [...]
> >>> +	rdiff_state.job = rs_patch_begin(base_file_read_cb, rdiff_state.base_file);
> >>> +	ret = copyfile(img->fdin,
> >>> +			&rdiff_state,
> >>> +			img->size,
> >>> +			(unsigned long *)&img->offset,
> >>> +			img->seek,
> >>> +			0, /* no skip */
> >>> +			img->compressed,
> >>> +			&img->checksum,
> >>> +			img->sha256,
> >>> +			img->is_encrypted,
> >>> +			apply_rdiff_chunk_cb);
> >>> +
> >>> +	if (rdiff_state.type == FILE_HANDLER) {
> >>> +		struct stat stat_dest_file;
> >>> +		if (fstat(fileno(rdiff_state.dest_file), &stat_dest_file) == -1) {
> >>> +			ERROR("Cannot fstat file %s: %s", dest_file_filename, strerror(errno));
> >>> +			ret = -1;
> >>> +			goto cleanup;
> >>> +		}
> >>> +
> >>> +		/*
> >>> +		 * Most often $TMPDIR -- in which dest_file resides -- is a different
> >>> +		 * filesystem (probably tmpfs) than that base_file resides in. Hence,
> >>> +		 * substituting base_file by dest_file cross-filesystem via renameat()
> >>> +		 * won't work. If dest_file and base_file are indeed in the same
> >>> +		 * filesystem, metadata (uid, gid, mode, xattrs, acl, ...) has to be
> >>> +		 * preserved after renameat(). This isn't worth the effort as Linux's
> >>> +		 * sendfile() is fast, so copy the content.
> >>> +		 */
> >>> +		rdiff_state.base_file = freopen(NULL, "wb", rdiff_state.base_file);
> >>> +		rdiff_state.dest_file = freopen(NULL, "rb", rdiff_state.dest_file);
> >>> +		if ((rdiff_state.base_file == NULL) || (rdiff_state.dest_file == NULL)) {
> >>> +			ERROR("Cannot reopen %s or %s: %s", dest_file_filename,
> >>> +				  base_file_filename, strerror(errno));
> >>> +			ret = -1;
> >>> +			goto cleanup;
> >>> +		}
> >>
> >> What about if the file does not exist in the destination directory and
> >> it is a new file ?
> > 
> > Good point, I've also thought about this. In this case, there's nothing
> > to apply a patch against, i.e., the patch creates the file and hence the
> > patch is naturally larger than the file itself due to rdiff metadata
> > overhead. Then, the binary diff sweet spot is missed by large and it
> > doesn't make sense to use patches but the file should be created by
> > installing it via the files:{} section first and then, possibly, be
> > later patched. For consistency, I could implement this as well, sure...
> 
> ok

So I'll implement this then for consistency.



Kind regards,
   Christian

Hi Christian,

On 13/11/18 15:02, Christian Storm wrote:
> Hi Stefano,
> 
>>> Roughly the former was with two mkosi-generated ArchLinux full-disk
>>> images of 1,5G size which resulted in a patch of 33 MB, i.e., 2% of the
>>> original size. So, to give a baseline number, this is the number that
>>> can be attributed just to image generation noise with using mkosi and
>>> ArchLinux. Add to this, e.g., the ~7 MB plus overhead of openssl and
>>> you'll get a patch of ~3-4% or ~50 MB size. Again, your mileage may vary.
>>> In general, binary diffs do shine, if you have (a) reproducible
>>> builds (i.e., a deterministic build system, stable output order, ...)
>>> and (b) small changes such as when applying a security update to a
>>> library on an otherwise unchanged root file system. Think of read-only
>>> root filesystems that get updated this way, frequently.
>>
>> Agree. This should also explained into the documentation. The use case
>> where we get a real advantage is when we have a ro filesystem and we
>> want to fix a few files, like in case of security fixes, few libraries,
>> and so on. It does not want to try to perform a delta when two
>> filesystem differ substantially, for example between two Yocto releases.
> 
> OK, I'll update the documentation to mention this as a prominent use case.
> 

Fine.

> 
>>>> I would like to understand which is the way to generate and deploy the
>>>> delta. The thing is that only the device knows which is the running
>>>> version, and just the build system knows the new one. 
>>>
>>> Well, if the backend/build system has no knowledge whatsoever of the
>>> image currently running on the device, it cannot sensibly compute a diff
>>> that fits the device.
>>
>> This is my assumption, too.
>>
>>> Under this assumption, binary diffs don't really
>>> make sense, in particular if they're precomputed at the server's side as
>>> it's the case for rdiff. Then, maybe "online binary diffs" like done by
>>> casync make more sense...
>>
>> This is *exactly* my conclusion when I took a look how to add binary
>> deltas to SWUpdate and at the end I land to think about top integrate
>> casync. Of course, casync requires some changes to let it couple with
>> SWUpdate. An advantage is that the server could be unaware of it, and
>> even it does not need to know which version of software is running on
>> the device.
>>
>> As I have already checked deeper, IMHO adding a "casync-like handler"
>> requires also some further changes, because casync thinks to run with
>> full rights as root and this breaks privilege separation when accessing
>> network. Nevertheless, casync should more used as librsync is, that is a
>> library instead of tool. So some surrounding code / script to let casync
>> working is not enough, but I think is doable. I have proposed this as
>> draft to a couple of customers of mine, but there is not (yet ?) enough
>> interest to drive an implementation. The librsync approach is a more
>> simple approach that it is suitable for some use cases, as we have stated.
> 
> Yes, agreed. The big benefit of librsync in contrast to casync and the
> like tools is that the diff can be computed at the server's side with
> the device having nothing to know and do except for applying the patch.

Right.

> Using casync, you have to have the signature calculation done on both
> ends, i.e., the device has to calculate the signature of its
> /dev/mmcblk0p1 so that casync can find the difference.

That's correct. IMHO the real big thing is that using casync, SWUpdate
has in SWU just the index (.caibx), and it must load from the casync
repos thze rest. This means that the whole Software including patches is
not in the SWU itself, but it is downloaded later.

> Granted, you can
> store the signature calculation results at the expense of disk space
> "wasted". This is a burden put on the device which it may not be up to
> if it's short on resources.
> So, I think there's room for both options, librsync as well as casync,
> depending on the use case, as always :)

This is what I would like to document. Your patchset is thought for a
well defined use case and it makes sense fr that, while an extension
based on casync can cover other use cases.

> 
> Speaking of casync, there's also zchunk (https://github.com/zchunk/zchunk)
> which is worth having a look at. 

Thanks, I will take a look.

> Both internally work alike but have
> different use cases. casync is a bit "too much" for me, that's why I
> found zchunk appealing although not perfectly fitting the partition
> binary diff use case.
> 
> casync will eventually become a library of some sort if I read the TODO
> list correctly,

This was already a while ago - I think that if we need it into SWUpdate,
there will be some work to be done to use it as library.

> so maybe we just have to wait for it to become more
> embedding friendly :)

Let's see..

> 
> 
>>> The big plus of rdiff is that *if* you know
>>> what's running on the device and that's not changed since your last
>>> deployment/update, you can compute the diff on the server's side using
>>> "cloud compute resources" and simply ship & apply it without having the
>>> device to commit any work like checksumming the device file as it's the
>>> case for "online binary diff" tools like casync and the like.
>>
>> Yes, agree. If we exactly know which is the version on the device and
>> the resulting release, we can compute offline the diff-SWU.
>>
>>> A backend may know what's running on the device, e.g., by the device
>>> updating properties on the backend side on each update or some other
>>> (side-channel) means.
>>
>> If you are thinking about Hawkbit, it has not yet this feature. The only
>> way for the device to inform the server is via the configData URL, but
>> the posted dictionary is not evaluated by Hawkbit core. A custom process
>> using the Hawkbit's management interface should be implemented to read
>> device data and to provide (or build) the SWU with the diff.
> 
> Yes, true, but there are others like mender.io, Siemens MindSphere, ... :)

Well, and they are welcome ! I guess it is a matter of time until new
backends will be added to SWUpdate. SWUpdate is thought to be backend
awareness, and support for new backends can be added.

> 
> 
>> This can be more difficult if a secure boot and a verification of the
>> root filesystem is implemented. In fact, in the last case even the
>> deployment server that generates the diff must be able to sign the
>> images, not only the build server (that one running Yocto / Buildroot /..).
> 
> Then you open a whole different can of worms. Having secure boot
> requires more integration works and close cooperation between the image
> generation and application by SWUpdate...

Really I do not see al lot of work into SWUpdate. But right, image
generation must be done to allow a later verification, like signing the
artifacts that will be verified later when system boots.

> 
> 
>>> Fact is, for this to be beneficial, the backend
>>> has to have current knowledge of the device in order to compute the
>>> diff at the backend's side.
>>
>> Thanks for explanation - I vote to have this sentence in the documentation.
> 
> Agreed. I will update the documentation once the review is positive :)

ok

> 
> 
>>>> [...]
>>>> The next point I want to discuss is the application of this in case of
>>>> compressed images. I guess the delta apply before compressing the
>>>> images, right ? After compression I feel we have bad chances to get
>>>> small deltas.
>>>
>>> Yes, you do the diff/delta generation on the "raw" output images. If you
>>> do this after compression, you are closer to the /dev/urandom case.
>>
>> ok, got it.
>>
>>>
>>>>
>>>> That means, after reading the example:
>>>>
>>>> - we build a filesystem, let's say a "my-image-domething.[filesystem]".
>>>>
>>>> - we run rdiff of it ==> new image type in OE ? A class in
>>>> meta-swupdate, maybe ?
>>>> I guess it is quite difficult without hacks to run this in OE, because a
>>>> build must have the knowledge of previous builds (with different layer
>>>> setup, maybe).
>>>
>>> There's naturally some turning point after which it doesn't make sense
>>> to ship a diff but a full image. The closer you are to the previous
>>> build(s), the greater your benefit of using diffs. So, the more OE can
>>> do reproducible builds, the more you'll benefit. For now, you build as
>>> always and compute the diff between the two (partition) artifacts. This
>>> is likely a post-processing step on the resulting (uncompressed) images
>>> after the OE chain.
>>
>> Ok, this can be done. Let's say we generate a new format ".diff", and we
>> can have a "diff.gz", too.
> 
> This would be an option, true.
> 
> 
>>>> [...]
>>>> - result can be compressed, too ==> Swupdate decompress it before
>>>> running the handler.
>>>
>>> Yes, that and decryption is for free as I intentionally use the
>>> copyfile() machinery, exactly for this benefit.
>>
>> Fully agree. Handler should use the copyfile(), because SWUpdate will
>> take care of most details and the whole pipeline (decompression /
>> decryption) is still valid.
>>
>>> That said, one can also
>>> compile librsync to compress the chunk data...
>>
>> No, this is not our use case.
> 
> Well, actually, if you compile librsync to have this feature and
> generate a diff having this feature, SWUpdate won't notice :)
> As SWUpdate does compression, I don't see a need for it but if you
> want to do it inside librsync, there's nothing stopping you..
> 
> 
>>>> Back to the original thread by Paul, can we apply this to the kernel ?
>>>> Kernel is already compressed as result of the "compile" task. And image
>>>> format can vary, we could have a vmlinux (uncompressed), a zImage, or a
>>>> uImage (both compressed) or a fitImage. Is it appliable to the kernel ?
>>>
>>> Yes, via the rdiff{,file} handler but the result isn't that mindblowing :) 
>>> There's simply too much that changes in between two kernels: A diff from
>>> my custom 4.14-76 kernel to my custom 4.18-16 kernel -- both bzImages
>>> and both 12MB in size -- is 9,7MB. A 4.18-14 to 4.18-16 diff is 9,2MB.
>>
>> I will say that it does not make sense to provide a diff.
> 
> Generally not, agreed. If you're after each and every byte transferred,
> however, it could.

Yes, of course.

> 
> 
>>> Here, compression butchers our benefit as it's unstable in order.
>>> Uncompressed, the kernels are 33MB in size and the diff is 19MB, i.e.,
>>> ~50%. That said, for patching single binaries only, one would be better
>>> off using something like Chromium's Courgette:
>>> https://www.chromium.org/developers/design-documents/software-updates-courgette
>>
>> I know this, but this is derived from bsdiff to better make diff on ELF
>> files. We cannot use it on any kind of files.
> 
> Not on any kind but for ELFs and ELF-like files like the kernel.

Right.

> If
> you're adventurous, one can provide a handler specifically for patching
> the kernel using Courgette.

This can be an option, right. We could have a list of new handlers for
delta update, each of them for a specific topic.

> 
> 
>>> As said, to get the most out of rdiff binary patches, you need as much
>>> reproducibility with stable output order as you can get. A
>>> compiler+compression pass usually doesn't satisfy this. It really
>>> depends on the mixture: Applied to a root filesystem with, say, just
>>> one library changed, you get very decent results for rdiff patches.
>>
>> Ok
>>
>>>
>>>
>>>>> [...]
>>>>> +
>>>>> +rdiff handler
>>>>> +-------------
>>>>> +
>>>>> +The rdiff handler adds support for applying binary delta patches generated by
>>>>> +`librsync's <http://librsync.sourcefrog.net/>`_ rdiff tool.
>>>>> +
>>>>> +First, create the signature of the original (base) file via
>>>>> +``rdiff signature <basefile> <signaturefile>``.
>>>>> +Then, create the delta file (i.e., patch) from the original base file to the target
>>>>> +file via ``rdiff delta <signaturefile> <targetfile> <deltafile>``.
>>>>> +The ``<deltafile>`` is the artifact to be applied via this handler on the device.
>>>>> +Essentially, it mimics running ``rdiff patch <basefile> <deltafile> <targetfile>``
>>>>> +on the device. Naturally for patches, the very same ``<basefile>`` has to be used
>>>>> +for creating as well as for applying the patch to.
>>>>> +
>>>>> +This handler registers itself for handling files and images.
>>>>> +An exemplary sw-description fragment for the files section is
>>>>> +
>>>>> +::
>>>>> +
>>>>> +    files: (
>>>>> +        {
>>>>> +            type = "rdiff"
>>>>
>>>> If you use rdiffimage, maybe we have to set it as rdifffile.
>>>
>>> I resisted because of the 3 'f's there, but OK, fine with me :)
>>
>> I do not like the 3 'f' too, but we have "rawfile". I think adding
>> "file" makes names consistent.
> 
> OK, then what about "rdiff_file" and "rdiff_image"?

Fine with me.

> In the end, I don't
> care that much and I'd like to name it as you propose :)

It is just to be consistent with the rest of SWUpdate, nothing more.

> 
> 
>>>>> [...]
>>>>> +
>>>>> +#define ASSERT(expr, failret) \
>>>>> +	if (expr) { \
>>>>> +	} else { \
>>>>> +		ERROR("Assertion violated: %s.", #expr); \
>>>>> +		return failret; \
>>>>> +	}
>>>>> +
>>>>
>>>> do we need this ? what about "assert" in most code ?
>>>
>>> Well, I don't want to abort the process in case an assertion doesn't
>>> hold. Printing an error through SWUpdate's notification framework and
>>> aborting the installation seemed sufficient to me, considering the use
>>> case of SWUpdate running as daemon. I could replace this by a traditional
>>> assert() and then have, e.g., systemd respawn SWUpdate on a failed
>>> assert() abortion, granted. I don't have a strong opinion for one over
>>> the other. What's your opinion on this?
>>
>> No, the let's assert() as it is, because this is the standard behavior.
>> Just change the name of this macro with whatever you want that does not
>> lead to assert and move it into util.h
> 
> OK, then we're at naming. I do see that this can be of general utility,
> so putting it in util.h makes some sense. What about naming it TEST() or
> TEST_AND_LOG()?

+1

> 
> 
>>>>> [...]
>>>>> +static int apply_rdiff_patch(struct img_type *img,
>>>>> +							 void __attribute__((__unused__)) * data)
>>>>> +{
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	struct rdiff_t rdiff_state = {};
>>>>> +	rdiff_state.type =
>>>>> +	    strcmp(img->type, "rdiffimage") == 0 ? IMAGE_HANDLER : FILE_HANDLER;
>>>>> +
>>>>> +	char *mountpoint = NULL;
>>>>> +	bool use_mount = (strlen(img->device) && strlen(img->filesystem)) ? true : false;
>>>>> +
>>>>> +	char *base_file_filename = NULL;
>>>>> +	char *dest_file_filename = NULL;
>>>>> +
>>>>> +	if (rdiff_state.type == IMAGE_HANDLER) {
>>>>> +		if (img->seek) {
>>>>> +			/*
>>>>> +			 * img->seek mandates copyfile()'s out parameter to be a fd, it
>>>>> +			 * isn't. So, the seek option is invalid for the rdiff handler.
>>>>> +			 * */
>>>>
>>>> Not clear, as  rdiff_state.dest_file is a fd, and a seek() on this fd
>>>> should be possible. What am I missing ?
>>>
>>> Yes, true, but.. as called by apply_rdiff_patch(), copyfile()'s out
>>> parameter is not a file descriptor but &rdiff_state. The (seek) test in
>>> copyfile() assumes it to be an fd, which is isn't.
>>
>> ok, thanks.
>>
>>> Of course, one can support seeking in the destination (device) file, but
>>> I haven't imagined a good reason to do so since skipping over
>>> unchanged content is handled quite well by the rdiff algorithm.
>>> That said, we may of course change this and implement it...
>>
>> I just needed to understand.
> 
> Well, I haven't seen a need for seeking. If you do have an example, we
> can implement it. 

Not yet - just when a use case occurs.

> Otherwise, it may stay as is and be documented, that's
> indeed missing currently.

+1

> Looks like lot of documentation works ahead :)

as usually, documentation is the last...

> 
> 
>>>>> +			ERROR("Option 'seek' is not supported for rdiff.");
>>>>> +			return -1;
>>>>> +		}
>>>>> +
>>>>> [...]
>>>>> +	rdiff_state.job = rs_patch_begin(base_file_read_cb, rdiff_state.base_file);
>>>>> +	ret = copyfile(img->fdin,
>>>>> +			&rdiff_state,
>>>>> +			img->size,
>>>>> +			(unsigned long *)&img->offset,
>>>>> +			img->seek,
>>>>> +			0, /* no skip */
>>>>> +			img->compressed,
>>>>> +			&img->checksum,
>>>>> +			img->sha256,
>>>>> +			img->is_encrypted,
>>>>> +			apply_rdiff_chunk_cb);
>>>>> +
>>>>> +	if (rdiff_state.type == FILE_HANDLER) {
>>>>> +		struct stat stat_dest_file;
>>>>> +		if (fstat(fileno(rdiff_state.dest_file), &stat_dest_file) == -1) {
>>>>> +			ERROR("Cannot fstat file %s: %s", dest_file_filename, strerror(errno));
>>>>> +			ret = -1;
>>>>> +			goto cleanup;
>>>>> +		}
>>>>> +
>>>>> +		/*
>>>>> +		 * Most often $TMPDIR -- in which dest_file resides -- is a different
>>>>> +		 * filesystem (probably tmpfs) than that base_file resides in. Hence,
>>>>> +		 * substituting base_file by dest_file cross-filesystem via renameat()
>>>>> +		 * won't work. If dest_file and base_file are indeed in the same
>>>>> +		 * filesystem, metadata (uid, gid, mode, xattrs, acl, ...) has to be
>>>>> +		 * preserved after renameat(). This isn't worth the effort as Linux's
>>>>> +		 * sendfile() is fast, so copy the content.
>>>>> +		 */
>>>>> +		rdiff_state.base_file = freopen(NULL, "wb", rdiff_state.base_file);
>>>>> +		rdiff_state.dest_file = freopen(NULL, "rb", rdiff_state.dest_file);
>>>>> +		if ((rdiff_state.base_file == NULL) || (rdiff_state.dest_file == NULL)) {
>>>>> +			ERROR("Cannot reopen %s or %s: %s", dest_file_filename,
>>>>> +				  base_file_filename, strerror(errno));
>>>>> +			ret = -1;
>>>>> +			goto cleanup;
>>>>> +		}
>>>>
>>>> What about if the file does not exist in the destination directory and
>>>> it is a new file ?
>>>
>>> Good point, I've also thought about this. In this case, there's nothing
>>> to apply a patch against, i.e., the patch creates the file and hence the
>>> patch is naturally larger than the file itself due to rdiff metadata
>>> overhead. Then, the binary diff sweet spot is missed by large and it
>>> doesn't make sense to use patches but the file should be created by
>>> installing it via the files:{} section first and then, possibly, be
>>> later patched. For consistency, I could implement this as well, sure...
>>
>> ok
> 
> So I'll implement this then for consistency.

Best regards,
Stefano

Hi Stefano,

> > Using casync, you have to have the signature calculation done on both
> > ends, i.e., the device has to calculate the signature of its
> > /dev/mmcblk0p1 so that casync can find the difference.
> 
> That's correct. IMHO the real big thing is that using casync, SWUpdate
> has in SWU just the index (.caibx), and it must load from the casync
> repos thze rest. This means that the whole Software including patches is
> not in the SWU itself, but it is downloaded later.

Well, or nothing except the URL since the signature calculation of the
device file/partition/ordinary file has to be done on the device as
well, otherwise the backend has to know what's currently on the device
and then you're at the librsync use case :)

True, you don't know what you get by just looking at the .swu as that's
to be determined at run-time by the casync algorithm... I'm not sure if
I really like this behavior unconditionally although I do see the
benefits. That was also one of the motivations to go for librsync while
I was evaluating the options.


> >>> The big plus of rdiff is that *if* you know
> >>> what's running on the device and that's not changed since your last
> >>> deployment/update, you can compute the diff on the server's side using
> >>> "cloud compute resources" and simply ship & apply it without having the
> >>> device to commit any work like checksumming the device file as it's the
> >>> case for "online binary diff" tools like casync and the like.
> >>
> >> Yes, agree. If we exactly know which is the version on the device and
> >> the resulting release, we can compute offline the diff-SWU.
> >>
> >>> A backend may know what's running on the device, e.g., by the device
> >>> updating properties on the backend side on each update or some other
> >>> (side-channel) means.
> >>
> >> If you are thinking about Hawkbit, it has not yet this feature. The only
> >> way for the device to inform the server is via the configData URL, but
> >> the posted dictionary is not evaluated by Hawkbit core. A custom process
> >> using the Hawkbit's management interface should be implemented to read
> >> device data and to provide (or build) the SWU with the diff.
> > 
> > Yes, true, but there are others like mender.io, Siemens MindSphere, ... :)
> 
> Well, and they are welcome ! I guess it is a matter of time until new
> backends will be added to SWUpdate. SWUpdate is thought to be backend
> awareness, and support for new backends can be added.

Absolutely, and I really love to see further added if only to have a choice.


> >> This can be more difficult if a secure boot and a verification of the
> >> root filesystem is implemented. In fact, in the last case even the
> >> deployment server that generates the diff must be able to sign the
> >> images, not only the build server (that one running Yocto / Buildroot /..).
> > 
> > Then you open a whole different can of worms. Having secure boot
> > requires more integration works and close cooperation between the image
> > generation and application by SWUpdate...
> 
> Really I do not see al lot of work into SWUpdate. But right, image
> generation must be done to allow a later verification, like signing the
> artifacts that will be verified later when system boots.

True, I also think SWUpdate with its flexibility is ready for this but
the integrator has to think things through thoroughly.


> >>>> Back to the original thread by Paul, can we apply this to the kernel ?
> >>>> Kernel is already compressed as result of the "compile" task. And image
> >>>> format can vary, we could have a vmlinux (uncompressed), a zImage, or a
> >>>> uImage (both compressed) or a fitImage. Is it appliable to the kernel ?
> >>>
> >>> Yes, via the rdiff{,file} handler but the result isn't that mindblowing :) 
> >>> There's simply too much that changes in between two kernels: A diff from
> >>> my custom 4.14-76 kernel to my custom 4.18-16 kernel -- both bzImages
> >>> and both 12MB in size -- is 9,7MB. A 4.18-14 to 4.18-16 diff is 9,2MB.
> >>
> >> I will say that it does not make sense to provide a diff.
> > 
> > Generally not, agreed. If you're after each and every byte transferred,
> > however, it could.
> 
> Yes, of course.
> 
> > 
> > 
> >>> Here, compression butchers our benefit as it's unstable in order.
> >>> Uncompressed, the kernels are 33MB in size and the diff is 19MB, i.e.,
> >>> ~50%. That said, for patching single binaries only, one would be better
> >>> off using something like Chromium's Courgette:
> >>> https://www.chromium.org/developers/design-documents/software-updates-courgette
> >>
> >> I know this, but this is derived from bsdiff to better make diff on ELF
> >> files. We cannot use it on any kind of files.
> > 
> > Not on any kind but for ELFs and ELF-like files like the kernel.
> 
> Right.
> 
> > If
> > you're adventurous, one can provide a handler specifically for patching
> > the kernel using Courgette.
> 
> This can be an option, right. We could have a list of new handlers for
> delta update, each of them for a specific topic.

Yes, then we would have
* librsync,
* a casync-alike, and
* an ELF file binary delta updater (possibly courgette-based).
Sounds good, that's quite an arsenal of options :)



So then I'll wait a bit for more (code) review comments and thereafter
do my fix/documentation homework for a v2 of this patch.



Kind regards,
   Christian

Hi Christian,

On 14/11/18 10:08, Christian Storm wrote:
> Hi Stefano,
> 
>>> Using casync, you have to have the signature calculation done on both
>>> ends, i.e., the device has to calculate the signature of its
>>> /dev/mmcblk0p1 so that casync can find the difference.
>>
>> That's correct. IMHO the real big thing is that using casync, SWUpdate
>> has in SWU just the index (.caibx), and it must load from the casync
>> repos thze rest. This means that the whole Software including patches is
>> not in the SWU itself, but it is downloaded later.
> 
> Well, or nothing except the URL

Yes, ot just the URL

> since the signature calculation of the
> device file/partition/ordinary file has to be done on the device as
> well, otherwise the backend has to know what's currently on the device
> and then you're at the librsync use case :)
> 
> True, you don't know what you get by just looking at the .swu as that's
> to be determined at run-time by the casync algorithm...

Right, this is the big benefit. The biggest drawback in most binary
delta approach is that you must exactly know which is the version you
are coming from and you have to compute the delta to the new one,
resulting in a bunch of deltas that are difficult to handle. And this is
the advantage to use a casync-like approach.

But as I said, it opens also a list of new issues and security problems
that must be careful considered before introducing some approaches into
SWUpdate.

> I'm not sure if
> I really like this behavior unconditionally although I do see the
> benefits. That was also one of the motivations to go for librsync while
> I was evaluating the options.

Good. I agree that librsync can be used in a specific use case and there
are benefits.

> 
> 
>>>>> The big plus of rdiff is that *if* you know
>>>>> what's running on the device and that's not changed since your last
>>>>> deployment/update, you can compute the diff on the server's side using
>>>>> "cloud compute resources" and simply ship & apply it without having the
>>>>> device to commit any work like checksumming the device file as it's the
>>>>> case for "online binary diff" tools like casync and the like.
>>>>
>>>> Yes, agree. If we exactly know which is the version on the device and
>>>> the resulting release, we can compute offline the diff-SWU.
>>>>
>>>>> A backend may know what's running on the device, e.g., by the device
>>>>> updating properties on the backend side on each update or some other
>>>>> (side-channel) means.
>>>>
>>>> If you are thinking about Hawkbit, it has not yet this feature. The only
>>>> way for the device to inform the server is via the configData URL, but
>>>> the posted dictionary is not evaluated by Hawkbit core. A custom process
>>>> using the Hawkbit's management interface should be implemented to read
>>>> device data and to provide (or build) the SWU with the diff.
>>>
>>> Yes, true, but there are others like mender.io, Siemens MindSphere, ... :)
>>
>> Well, and they are welcome ! I guess it is a matter of time until new
>> backends will be added to SWUpdate. SWUpdate is thought to be backend
>> awareness, and support for new backends can be added.
> 
> Absolutely, and I really love to see further added if only to have a choice.

Let's see if someone have interest on it.

> 
> 
>>>> This can be more difficult if a secure boot and a verification of the
>>>> root filesystem is implemented. In fact, in the last case even the
>>>> deployment server that generates the diff must be able to sign the
>>>> images, not only the build server (that one running Yocto / Buildroot /..).
>>>
>>> Then you open a whole different can of worms. Having secure boot
>>> requires more integration works and close cooperation between the image
>>> generation and application by SWUpdate...
>>
>> Really I do not see al lot of work into SWUpdate. But right, image
>> generation must be done to allow a later verification, like signing the
>> artifacts that will be verified later when system boots.
> 
> True, I also think SWUpdate with its flexibility is ready for this but
> the integrator has to think things through thoroughly.

Right.

> 
> 
>>>>>> Back to the original thread by Paul, can we apply this to the kernel ?
>>>>>> Kernel is already compressed as result of the "compile" task. And image
>>>>>> format can vary, we could have a vmlinux (uncompressed), a zImage, or a
>>>>>> uImage (both compressed) or a fitImage. Is it appliable to the kernel ?
>>>>>
>>>>> Yes, via the rdiff{,file} handler but the result isn't that mindblowing :) 
>>>>> There's simply too much that changes in between two kernels: A diff from
>>>>> my custom 4.14-76 kernel to my custom 4.18-16 kernel -- both bzImages
>>>>> and both 12MB in size -- is 9,7MB. A 4.18-14 to 4.18-16 diff is 9,2MB.
>>>>
>>>> I will say that it does not make sense to provide a diff.
>>>
>>> Generally not, agreed. If you're after each and every byte transferred,
>>> however, it could.
>>
>> Yes, of course.
>>
>>>
>>>
>>>>> Here, compression butchers our benefit as it's unstable in order.
>>>>> Uncompressed, the kernels are 33MB in size and the diff is 19MB, i.e.,
>>>>> ~50%. That said, for patching single binaries only, one would be better
>>>>> off using something like Chromium's Courgette:
>>>>> https://www.chromium.org/developers/design-documents/software-updates-courgette
>>>>
>>>> I know this, but this is derived from bsdiff to better make diff on ELF
>>>> files. We cannot use it on any kind of files.
>>>
>>> Not on any kind but for ELFs and ELF-like files like the kernel.
>>
>> Right.
>>
>>> If
>>> you're adventurous, one can provide a handler specifically for patching
>>> the kernel using Courgette.
>>
>> This can be an option, right. We could have a list of new handlers for
>> delta update, each of them for a specific topic.
> 
> Yes, then we would have
> * librsync,
> * a casync-alike, and
> * an ELF file binary delta updater (possibly courgette-based).
> Sounds good, that's quite an arsenal of options :)

Nothing against this approach, let's see what happens in future !


> 
> 
> 
> So then I'll wait a bit for more (code) review comments and thereafter
> do my fix/documentation homework for a v2 of this patch.

ok, thanks - I will check the rest of 2/3 and I will report some more
comments, if any.

Best regards,
Stefano