[v2,3/4] 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  |  75 +++++++
 doc/source/swupdate.rst  |   1 +
 handlers/Config.in       |   7 +
 handlers/Makefile        |   1 +
 handlers/rdiff_handler.c | 427 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 515 insertions(+)
 create mode 100644 handlers/rdiff_handler.c

Hi Christian,

On 14/12/18 15:41, Christian Storm wrote:
> 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>
> ---

I get a warning after applying your patch:

  CC      handlers/rdiff_handler.o
handlers/rdiff_handler.c: In function ‘apply_rdiff_patch’:
handlers/rdiff_handler.c:321:14: warning: passing argument 1 of
‘rs_trace_to’ from incompatible pointer type [-Wincompatible-pointer-types]
  rs_trace_to(rdiff_log);
              ^~~~~~~~~
In file included from handlers/rdiff_handler.c:15:0:
/usr/include/librsync.h:87:17: note: expected ‘void (*)(int,  const char
*)’ but argument is of type ‘void (*)(rs_loglevel,  const char *) {aka
void (*)(enum <anonymous>,  const char *)}’
 void            rs_trace_to(rs_trace_fn_t *);
                 ^~~~~~~~~~~

Can you fix it, please ? I have already applied patches 1/4 and 2/4.

Best regards,
Stefano

>  Makefile.flags           |   4 +
>  doc/source/handlers.rst  |  75 +++++++
>  doc/source/swupdate.rst  |   1 +
>  handlers/Config.in       |   7 +
>  handlers/Makefile        |   1 +
>  handlers/rdiff_handler.c | 427 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 515 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..a8f0081 100644
> --- a/doc/source/handlers.rst
> +++ b/doc/source/handlers.rst
> @@ -350,6 +350,81 @@ 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.
> +
> +Naturally, the smaller the difference between the diff's source and target, the
> +more effective is using this handler rather than shipping the full target, e.g.,
> +via the image handler. Hence, the most prominent use case for the rdiff handler
> +is when having a read-only root filesystem and applying a small update like
> +security fixes or feature additions. If the sweet spot is crossed, an rdiff
> +patch may even exceed the full target's size due to necessary patch metadata.
> +Also note that in order to be most effective, an image to be processed with
> +rdiff should be built deterministic
> +(see `reproducible-builds.org <https://reproducible-builds.org>`_).
> +
> +The rdiff algorithm requires no resources whatsoever on the device as the patch
> +is fully computed in the backend. Consequently, the backend has to have
> +knowledge of the current software running on the device in order to compute
> +a sensible patch. Alike, the patch has to be applied on the device to an
> +unmodified source as used in the backend for patch computation. This property is
> +in particular useful for resource-constrained devices as there's no need for the
> +device to, e.g., aid in the difference computation.
> +
> +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_file"
> +            filename = "file.rdiff.delta";
> +            path = "/usr/bin/file";
> +        }
> +    );
> +
> +
> +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 = "rdiff_image";
> +            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>``.
> +Note that there's no support for the optional ``offset`` attribute in the 
> +``rdiff_image`` handler as there's currently no apparent use case for it and
> +skipping over unchanged content is handled well by the rdiff algorithm.
> +
> +
>  ucfw handler
>  ------------
>  
> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
> index b72a0c7..9e36924 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..c51c8aa
> --- /dev/null
> +++ b/handlers/rdiff_handler.c
> @@ -0,0 +1,427 @@
> +/*
> + * 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 <libgen.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 TEST_OR_FAIL(expr, failret) \
> +	if (expr) { \
> +	} else { \
> +		ERROR("Assertion violated: %s.", #expr); \
> +		return failret; \
> +	}
> +
> +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... */
> +		TEST_OR_FAIL(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;
> +		TEST_OR_FAIL(target - buffers->next_in <= RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +		TEST_OR_FAIL(buffers->next_in >= rdiff_state->inbuf, RS_IO_ERROR);
> +		TEST_OR_FAIL(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;
> +	TEST_OR_FAIL(len <= RDIFF_BUFFER_SIZE, RS_IO_ERROR);
> +	TEST_OR_FAIL(buffers->next_out >= rdiff_state->outbuf, RS_IO_ERROR);
> +	TEST_OR_FAIL(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) {
> +		TEST_OR_FAIL(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, "rdiff_image") == 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.
> +			 * */
> +			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;
> +
> +		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;
> +		}
> +
> +		base_file_filename = img->path;
> +		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;
> +			}
> +
> +			base_file_filename = alloca(strlen(mountpoint) + strlen(img->path) + 1);
> +			sprintf(base_file_filename, "%s%s", mountpoint, img->path);
> +		}
> +
> +		char* make_path = dict_get_value(&img->properties, "create-destination");
> +		if (make_path != NULL && strcmp(make_path, "true") == 0) {
> +			TRACE("Creating path %s", dirname(base_file_filename));
> +			if (mkpath(dirname(strdupa(base_file_filename)), 0755) < 0) {
> +				ERROR("Cannot create path %s: %s", dirname(base_file_filename),
> +					  strerror(errno));
> +				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;
> +		}
> +
> +#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("rdiff_image", apply_rdiff_patch, IMAGE_HANDLER, NULL);
> +}
> +
> +__attribute__((constructor))
> +void rdiff_file_handler(void)
> +{
> +	register_handler("rdiff_file", apply_rdiff_patch, FILE_HANDLER, NULL);
> +}
>

Hi Stefano,

> On 14/12/18 15:41, Christian Storm wrote:
> > 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>
> > ---
> 
> I get a warning after applying your patch:
> 
>   CC      handlers/rdiff_handler.o
> handlers/rdiff_handler.c: In function ‘apply_rdiff_patch’:
> handlers/rdiff_handler.c:321:14: warning: passing argument 1 of
> ‘rs_trace_to’ from incompatible pointer type [-Wincompatible-pointer-types]
>   rs_trace_to(rdiff_log);
>               ^~~~~~~~~
> In file included from handlers/rdiff_handler.c:15:0:
> /usr/include/librsync.h:87:17: note: expected ‘void (*)(int,  const char
> *)’ but argument is of type ‘void (*)(rs_loglevel,  const char *) {aka
> void (*)(enum <anonymous>,  const char *)}’
>  void            rs_trace_to(rs_trace_fn_t *);
>                  ^~~~~~~~~~~
> 
> Can you fix it, please ? I have already applied patches 1/4 and 2/4.

Thanks for applying those!

Well, unfortunately, I can't fix the warning :)
The reason is in the (4/4) patch's description:

  Note that travis's Ubuntu Trusty is that old that it ships with
  librsync 0.9.7. Versions 2.0.1 onwards (released 2017-10-17) have
    typedef void rs_trace_fn_t(rs_loglevel level, char const *msg)
  while earlier versions such as travis's have
    typedef void rs_trace_fn_t(int level, char const *msg).
  Hence the compiler warning in CI which cannot be silenced by
  a gcc pragma as travis's gcc is too old, too. Unfortunately,
  librsync doesn't provide a version #define palatable to a #ifdef.

So, with an old librsync < 2.0.1 the warning will show up while with
newer versions there's no warning. Unfortunately, there's no proper
version sting defined in librsync either.
If we would have proper library version detection e.g. via pkgconfig or
something, we may me able to circumvent this warning.
However, as of now, I do only see the options to either have two
KConfig switches (or one giving the version) or live with the warning
until the distros catch up to the new version.

What's your take on this?


Kind regards,
   Christian

Hi Christian,

On 07/01/19 09:47, Christian Storm wrote:
> Hi Stefano,
> 
>> On 14/12/18 15:41, Christian Storm wrote:
>>> 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>
>>> ---
>>
>> I get a warning after applying your patch:
>>
>>   CC      handlers/rdiff_handler.o
>> handlers/rdiff_handler.c: In function ‘apply_rdiff_patch’:
>> handlers/rdiff_handler.c:321:14: warning: passing argument 1 of
>> ‘rs_trace_to’ from incompatible pointer type [-Wincompatible-pointer-types]
>>   rs_trace_to(rdiff_log);
>>               ^~~~~~~~~
>> In file included from handlers/rdiff_handler.c:15:0:
>> /usr/include/librsync.h:87:17: note: expected ‘void (*)(int,  const char
>> *)’ but argument is of type ‘void (*)(rs_loglevel,  const char *) {aka
>> void (*)(enum <anonymous>,  const char *)}’
>>  void            rs_trace_to(rs_trace_fn_t *);
>>                  ^~~~~~~~~~~
>>
>> Can you fix it, please ? I have already applied patches 1/4 and 2/4.
> 
> Thanks for applying those!
> 
> Well, unfortunately, I can't fix the warning :)
> The reason is in the (4/4) patch's description:
> 
>   Note that travis's Ubuntu Trusty is that old that it ships with

This happens with Ubuntu bionic (18.04), too.

>   librsync 0.9.7. 

Same version is delivered in bionic. In meta-swupdate we have 2.0.2

> Versions 2.0.1 onwards (released 2017-10-17) have
>     typedef void rs_trace_fn_t(rs_loglevel level, char const *msg)
>   while earlier versions such as travis's have
>     typedef void rs_trace_fn_t(int level, char const *msg).

I see - I am comparing librsync.h in both versions and we could even try
to check if rs_loglevel is set. It was already defined in 0.9.7.

>   Hence the compiler warning in CI which cannot be silenced by
>   a gcc pragma as travis's gcc is too old, too. Unfortunately,
>   librsync doesn't provide a version #define palatable to a #ifdef.

This is an issue that happens quite often - I had recently to fix a
change in API for libgpiod, and without versioning the API I had to
introduce a quirk.

> 
> So, with an old librsync < 2.0.1 the warning will show up while with
> newer versions there's no warning. Unfortunately, there's no proper
> version sting defined in librsync either.

ok, got it.

> If we would have proper library version detection e.g. via pkgconfig or
> something, we may me able to circumvent this warning.
> However, as of now, I do only see the options to either have two
> KConfig switches (or one giving the version)

This is what I do for libgpiod, see
bdc11f9e6fd0497fa3adeb1feacc160d655aedff. But I had build errors, not
simply warningss, because the prototypes changed.

> or live with the warning
> until the distros catch up to the new version.

We live with the warning - I had no time to test your patch. I will
merge it anyway as this allows to more people to test it.

> 
> What's your take on this?
> 

Best regards,
Stefano

Hi Stefano,

> >> On 14/12/18 15:41, Christian Storm wrote:
> >>> 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>
> >>> ---
> >>
> >> I get a warning after applying your patch:
> >>
> >>   CC      handlers/rdiff_handler.o
> >> handlers/rdiff_handler.c: In function ‘apply_rdiff_patch’:
> >> handlers/rdiff_handler.c:321:14: warning: passing argument 1 of
> >> ‘rs_trace_to’ from incompatible pointer type [-Wincompatible-pointer-types]
> >>   rs_trace_to(rdiff_log);
> >>               ^~~~~~~~~
> >> In file included from handlers/rdiff_handler.c:15:0:
> >> /usr/include/librsync.h:87:17: note: expected ‘void (*)(int,  const char
> >> *)’ but argument is of type ‘void (*)(rs_loglevel,  const char *) {aka
> >> void (*)(enum <anonymous>,  const char *)}’
> >>  void            rs_trace_to(rs_trace_fn_t *);
> >>                  ^~~~~~~~~~~
> >>
> >> Can you fix it, please ? I have already applied patches 1/4 and 2/4.
> > 
> > Thanks for applying those!
> > 
> > Well, unfortunately, I can't fix the warning :)
> > The reason is in the (4/4) patch's description:
> > 
> >   Note that travis's Ubuntu Trusty is that old that it ships with
> 
> This happens with Ubuntu bionic (18.04), too.

Ah, OK, I tested locally and on CI with travis' Ubuntu version.
Then, they haven't updated it (again).


> >   librsync 0.9.7. 
> 
> Same version is delivered in bionic. In meta-swupdate we have 2.0.2
> 
> > Versions 2.0.1 onwards (released 2017-10-17) have
> >     typedef void rs_trace_fn_t(rs_loglevel level, char const *msg)
> >   while earlier versions such as travis's have
> >     typedef void rs_trace_fn_t(int level, char const *msg).
> 
> I see - I am comparing librsync.h in both versions and we could even try
> to check if rs_loglevel is set. It was already defined in 0.9.7.

One could do some workarounds and in fact I tried some that work locally
avoiding the warning. Then, however, travis compilation fails as it has
a too old gcc :)


> >   Hence the compiler warning in CI which cannot be silenced by
> >   a gcc pragma as travis's gcc is too old, too. Unfortunately,
> >   librsync doesn't provide a version #define palatable to a #ifdef.
> 
> This is an issue that happens quite often - I had recently to fix a
> change in API for libgpiod, and without versioning the API I had to
> introduce a quirk.

Unversioned APIs are a always a pain, I do fully agree.


> > So, with an old librsync < 2.0.1 the warning will show up while with
> > newer versions there's no warning. Unfortunately, there's no proper
> > version sting defined in librsync either.
> 
> ok, got it.
> 
> > If we would have proper library version detection e.g. via pkgconfig or
> > something, we may me able to circumvent this warning.
> > However, as of now, I do only see the options to either have two
> > KConfig switches (or one giving the version)
> 
> This is what I do for libgpiod, see
> bdc11f9e6fd0497fa3adeb1feacc160d655aedff. But I had build errors, not
> simply warningss, because the prototypes changed.

Yes, this is exactly what I had in mind for this purpose as well. But
since this is just a warning I refrained from doing so in the hope
distros will catch up eventually so that the cosmetically unappealing
warning will vanish...


> > or live with the warning
> > until the distros catch up to the new version.
> 
> We live with the warning - I had no time to test your patch. I will
> merge it anyway as this allows to more people to test it.

Thanks! 


Kind regards,
   Christian