Remote Image Handler based on Nanomsg library

This adds remote handler which uses  Nanomsg library
instead of zeromq library. This handlers works as IPC client
in request response configuration. It tries to open a connection
 to a path defined in the data field and send image to local IPC server.

Signed-off-by: Wojciech Michna <Wojciech.Michna@assaabloy.com>
---
 Makefile.flags             |   4 +
 handlers/Config.in         |  10 ++
 handlers/Makefile          |   1 +
 handlers/nanomsg_handler.c | 301 +++++++++++++++++++++++++++++++++++++
 4 files changed, 316 insertions(+)
 create mode 100644 handlers/nanomsg_handler.c

Hi Wojciech,

please do not attach the patch but inline it to make review easier and
avoid that mailclient can mishandle it.

(Use git send-email --to swupdate@googlegroups.com )

On 26/02/19 12:24, Wojciech Michna wrote:
> 
> This adds remote handler which uses  Nanomsg library
> instead of zeromq library.

It is not a replacement of zeromq, it is a new handler.

Have you consider to use the nng library as suggested by nanomsg website
? I do not see new update for nanomsg, and development flows into nng
instead.

> This handlers

handler

> works as IPC client
> in request response configuration.

Can you formulate again the phrase ? It is not clear to me. Is it like "
This handler forwards the image " ?

> It tries to open a connection
>  to a path defined in the data field and send image to local IPC server.
> 
> Signed-off-by: Wojciech Michna <Wojciech.Michna@assaabloy.com>
> ---
>  Makefile.flags             |   4 +
>  handlers/Config.in         |  10 ++
>  handlers/Makefile          |   1 +
>  handlers/nanomsg_handler.c | 301 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 316 insertions(+)
>  create mode 100644 handlers/nanomsg_handler.c
> 

> 
> 
> diff --git a/Makefile.flags b/Makefile.flags
> index 82365a5..d991ad3 100644
> --- a/Makefile.flags
> +++ b/Makefile.flags
> @@ -173,6 +173,10 @@  ifeq ($(CONFIG_REMOTE_HANDLER),y)
>  LDLIBS += zmq
>  endif
>  
> +ifeq ($(CONFIG_NANOMSG_HANDLER),y)
> +LDLIBS += nanomsg
> +endif
> +

I need to see the corresponding patches for meta-swupdate, else build is
broken if nanomsg is set.

>  ifeq ($(CONFIG_UCFWHANDLER),y)
>  LDLIBS += gpiod
>  endif
> diff --git a/handlers/Config.in b/handlers/Config.in
> index 91d43f3..cb5816f 100644
> --- a/handlers/Config.in
> +++ b/handlers/Config.in
> @@ -177,6 +177,16 @@  config REMOTE_HANDLER
>  	  the image to be updated with the help of the
>  	  zeromq library.
>  
> +config NANOMSG_HANDLER
> +	bool "nanomsg handler"
> +	default n
> +	help
> +	  Update handler which communicates with external
> +	  processes using NanoMsg communication over
> +	  IPC channel. This handler creates Nanomsg Request
> +	  client which will try to connect to server running
> +	  on a client device.

This is not very clear - I think it is enough to say that the handler
forwards the image to a listening process using the nanomsg library.
Please also add a link to nanomsg (https://nanomsg.org/)

> +
>  comment "remote handler needs zeromq"
>  	depends on !HAVE_LIBZEROMQ
>  

Wrong - you put your code in between. This is related to the above
handler (zeromq).

You need an equivalent code here for nanomsg or you break
buildroot.(HAVE_NANOMSG)

> diff --git a/handlers/Makefile b/handlers/Makefile
> index b75c3ba..3fad943 100644
> --- a/handlers/Makefile
> +++ b/handlers/Makefile
> @@ -8,6 +8,7 @@ 
>  # Handler can be called dynamically based
>  # on the received image type.
>  obj-y	+= dummy_handler.o
> +obj-$(CONFIG_NANOMSG_HANDLER) += nanomsg_handler.o
>  obj-$(CONFIG_ARCHIVE) += archive_handler.o
>  obj-$(CONFIG_BOOTLOADERHANDLER) += boot_handler.o
>  obj-$(CONFIG_CFI)	+= flash_handler.o
> diff --git a/handlers/nanomsg_handler.c b/handlers/nanomsg_handler.c
> new file mode 100644
> index 0000000..a06596a
> --- /dev/null
> +++ b/handlers/nanomsg_handler.c
> @@ -0,0 +1,301 @@ 
> +/*
> + * (C) Copyright 2019
> + * Wojciech Michna, Assa Aabloy, Wojciech.Michna@assaabloy.com
> + *
> + * SPDX-License-Identifier:     GPL-2.0-or-later
> + */
> +
> +/**
> + * @file nanomsg_handler.c
> + * @brief Handler for remote updates using NanoMsg library
> + * 
> + * 
> + * This file contains update handler which communicates with external
> + * processes using NanoMsg communication over IPC channel.
> + * This handler creates Nanomsg Request client which will try
> + * to connect to server running on a client device. 
> + * 
> + */
> +
> +/* standard includes. */

This comment is not useful

> +#include <stdio.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +/* nanomsg specific includes. */

Ditto

> +#include <nanomsg/nn.h>
> +#include <nanomsg/reqrep.h>
> +
> +/* swupdate specific includes. */

Ditto

> +#include "swupdate.h"
> +#include "handler.h"
> +#include "util.h"
> +
> +#define ACK_MESSAGE "ACK"
> +#define ACK_MESSAGE_SIZE 3
> +
> +#define RET_SUCCESS 0
> +#define RET_ERROR -1

I do not like these two generic defines. There is already the non
written rule that a SWUpdate's function returns 0 in case of success,
and a useful negative value (-ENOMEM, _EBADMSG, ...) in case of error.

> +
> +#define LOW_TIMEOUT 1000
> +#define HIGH_TIMEOUT 20000

The receive timout is set to 20 seconds - one SWUpdate runs the handlers
sequentially in one thread, that means SWUpdate stops for up to 20
seconds. I suggest at least to make the time configurable via a
"property" in sw-description.

> +
> +#define EOF_SIZE 1

Waht is EOF here ? I understand from code that there are just C strings,
that is Null terminated string. EOF is at least the wrong name and the
define is not very useful.

> +
> +#define MESSAGE_BUFF_SIZE 64
> +char message_buff[MESSAGE_BUFF_SIZE];

What are coming these size ? Can you also better explain your use case ?
What are you updating with this handler ?

> +
> +static void nanomsg_handler(void) __attribute__((constructor));
> +static int send_message_with_ack(int sock, char* message, int n);
> +
> +static unsigned int max_send_size = 0;
> +static int set_max_send_size(int sock);
> +
> +static int send_init(int sock, long long size);
> +static int send_data_begin(int sock);
> +
> +/**
> + * @brief Function sending message and waiting for ACK message
> + * 
> + * @param [in] int sock Socket which will be used to send and receive messages
> + * @param [in] char* message Message which will be send 
> + * @param [in] int n size in bytes of message to send  
> + * 
> + * @return RET_ERROR - indicating send or receive error or RET_SUCCESS - indicating success
> + */
> +static int send_message_with_ack(int sock, char* message, int n){

space before { or better "{" to newline
		
> +		int bytes = -1;
> +		int ret = RET_ERROR;
> +		char tmp[ACK_MESSAGE_SIZE];

indent is reported as wrong by any "checkpatch" run, even if SWUpdate
has not a own one. Parameter should be after the above "("

> +	
> +        if ((bytes = nn_send(sock, message, n, 0)) < 0) {
> +			return ret;

indent is wrong - this must be fixed globally.

> +        }
> +        
> +        char *buf = NULL;
> +        
> +        /*
> +		 Recieve response from server

The rule for multiline comment is:

/*
 * First line
 * Secondline
 */

> +        */
> +        if ((bytes = nn_recv(sock, &buf, NN_MSG, 0)) <= 0) {
> +			return ret;

I do not like this. You check for error, but when you find it, you
overload it with -1. You should add an "ERROR" statement to trace what
is happening.

> +        }
> +        
> +        /*
> +          Check if we received ACK
> +          Any other message is treated as failure
> +         */

Multiline comment

> +		if(bytes >= ACK_MESSAGE_SIZE){
> +			memcpy(tmp,buf,ACK_MESSAGE_SIZE);
> +			if( 0 == strncmp(tmp, ACK_MESSAGE, ACK_MESSAGE_SIZE)){
> +				ret = RET_SUCCESS;
> +			}						
> +		}

So you forward data and the receiver sends back "ACK" in ASCII format.
It would bet fine to add a small documentation about this small protocol
at the beginning of the file.

> +		
> +        nn_freemsg(buf);
> +		return ret;     
> +}
> +
> +/**
> + * @brief Function sending INIT message
> + * 
> + * @param [in] sock Socket which will be used to send and receive messages
> + * @param [in] long long size Size size of image that will be sent 
> + * 
> + * @return RET_ERROR - indicating send or receive error or RET_SUCCESS - indicating success
> + */
> +static int send_init(int sock, long long size){
> +	snprintf(message_buff, MESSAGE_BUFF_SIZE-EOF_SIZE , "INIT:%lld", size);
                                                ^-- space is missing

> +	return send_message_with_ack(sock,message_buff,strlen(message_buff));
> +}
> +
> +/**
> + * @brief Function sending DATA message
> + * 
> + * @param [in] sock Socket which will be used to send and receive messages
> + * 
> + * @return RET_ERROR - indicating send or receive error or RET_SUCCESS - indicating success
> + */
> +static int send_data_begin(int sock){
> +	snprintf(message_buff, MESSAGE_BUFF_SIZE-EOF_SIZE , "DATA");
> +	return send_message_with_ack(sock,message_buff,strlen(message_buff));

return send_message_with_ack(sock, message_buff, strlen(message_buff));

> +}
> +
> +/**
> + * @brief Function seting maximum size handler is able to send
> + * 
> + * @param [in] sock Socket from which maximum size will be determined
> + * 
> + * @return RET_ERROR - indicating failure when accessing socket RET_SUCCESS - indicating success
> + */
> +static int  set_max_send_size(int sock){
> +	int tmp_send_size;
> +	size_t sz = sizeof (tmp_send_size);
> +	
> +	max_send_size = 0;
> +	
> +	if (nn_getsockopt (sock, NN_SOL_SOCKET, NN_SNDBUF, &tmp_send_size, &sz) < 0) {
> +		return RET_ERROR;
> +	}
> +	
> +	max_send_size = (unsigned int)tmp_send_size;
> +	
> +	return RET_SUCCESS;
> +}

The way how the size is handled looks to me wrong, more in forward data


> +
> +/**
> + * @brief Function sending chunks of image data to server
> + * 
> + * @param [in] void *request Pointer to socket, have to be populated before 
> + * @param [in] const void *buf Pointer to buffer containing chunk of data
> + * @param [in] unsigned int len Buffer Length
> + * 
> + * @return EFAULT - indicating failure and interrupt further sending RET_SUCCESS (HAVE TO BE 0) - indicating success
> + */
> +static int forward_data(void *request, const void *buf, unsigned int len)
> +{
> +	int ret = 0;
> +	int* psock = (int*)request;
> +	int sock = 0;
> +	char* tmp = 0x00;
> +	
> +	if ( 0 == psock){

if (!psock)

> +		return -EFAULT;
> +	}
> +
> +	sock = *psock;

You do not need sock, just use *psock.

> +
> +	if(max_send_size<len){
> +		return -EFAULT;
> +	}

The handling of the size is IMHO wrong. The handler is the sink, not the
source, and it cannot handle the size of the data. It just receive data
from SWUpdate's core (the copyimage function) after the main processing.

Just dropping and returning with error does not seem a solution - if a
bigger message is received from SWUpdate and cannot be sent via nanomsg,
forward_data() should split the message in smaller chucnks and iterate
until all data is sent.

> +	
> +	tmp = (char*) malloc(len+EOF_SIZE);
> +	memset(tmp,0x00,len+EOF_SIZE);

You do not need to zero the buffer because you fill in.

> +	memcpy(tmp,buf,len);
> +	ret = send_message_with_ack(sock,tmp,len);
> +	
> +	free(tmp);
> +	
> +	return ret;
> +}
> +
> +/**
> + * @brief Handler installed in swupdate responsible for handling "nanomsg" update type 
> + * 
> + * @param [in] struct img_type *img pointer to update structure 
> + * @param [in] *data unused pointer for compliance with handler definition
> + * 
> + * @return EFAULT - indicating failure and interrupt update RET_SUCCESS (HAVE TO BE 0) - indicating success
> + */
> +static int install_send_to_remote(struct img_type *img,
> +	void __attribute__ ((__unused__)) *data)
> +{	
> +	char *connect_string;
> +	int len;
> +	int sock = 0;
> +	int rv = 0;
> +	int ret = 0;
> +	int time_out = 0;
> +	
> +	if( 0 == img){

(!img)

> +		ERROR("Critical fault no img passed to handler");
> +		return -EFAULT;
> +	}
> +	
> +	if( 0 == strlen(img->type_data) ){

!strlen(img->type_data)

> +		ERROR("No data section, can't open socket");
> +		return -EFAULT;
> +	}
> +	
> +	len = strlen(img->type_data) 
> +		+ strlen(CONFIG_SOCKET_REMOTE_HANDLER_DIRECTORY)
> +		+ strlen("ipc://")
> +		+ EOF_SIZE;
> +
> +	connect_string = (char*)malloc(len);
> +	
> +	if (!connect_string) {
> +		ERROR("Not enough memory");
> +		return -ENOMEM;
> +	}
> +	
> +	snprintf(	connect_string,
> +				len,
> +				"ipc://%s%s",
> +				CONFIG_SOCKET_REMOTE_HANDLER_DIRECTORY,
> +				img->type_data);
> +	

indent is wrong

> +	if (( sock = nn_socket(AF_SP, NN_REQ)) < 0) {
> +		ERROR("Can't create socket");
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +		
> +	if (( rv = nn_connect (sock, connect_string)) < 0) {
> +		ERROR("Can't connect to socket");
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +	
> +	time_out = LOW_TIMEOUT;
> +	if (nn_setsockopt(sock, NN_SOL_SOCKET, NN_RCVTIMEO, &time_out, sizeof (time_out)) < 0) {
> +		ERROR("Can't change opt");
> +		nn_shutdown(sock, rv);
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +
> +	if(send_init(sock, img->size ) != 0){
> +		ERROR("Sending init message failed");
> +		nn_shutdown(sock, rv);
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +	
> +	if(send_data_begin(sock) != 0){
> +		ERROR("Sending data message failed");
> +		nn_shutdown(sock, rv);
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +	
> +	time_out = HIGH_TIMEOUT;
> +	if (nn_setsockopt(sock, NN_SOL_SOCKET, NN_RCVTIMEO, &time_out, sizeof (time_out)) < 0) {
> +		ERROR("Can't change opt");
> +		nn_shutdown(sock, rv);
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +	
> +	if (set_max_send_size(sock) != RET_SUCCESS) {
> +		ERROR("Can't set max send size");
> +		nn_shutdown(sock, rv);
> +		free(connect_string);
> +		return -EFAULT;
> +	}
> +	
> +	ret = copyimage(&sock, img, forward_data);	
> +		
> +	nn_shutdown(sock, rv);
> +	free(connect_string);
> +	
> +	if(ret != 0){
> +		ERROR("Send error");
> +		return -EFAULT;
> +	}
> +	
> +	return RET_SUCCESS;
> +}
> +
> +/**
> + * @brief Constructor function run at start of application responsible for installing handler
> + */
> +static void nanomsg_handler(void)
> +{
> +	register_handler("nanomsg",
> +					 install_send_to_remote,
> +					 IMAGE_HANDLER,
> +					 NULL);
> +}
> 


Best regards,
Stefano Babic

Hi Michna,

please do not drop the ML in your answers - thanks !

On 19/03/19 12:45, Michna, Wojciech wrote:
> Good Morning.
> 
> Thanks for your response. I have two questions.
> One, I did use inline and send mail by creating path by 
> git format-patch --inline -1 HEAD

You do not need inline

> and sending it with
> git send-email --annotate --smtp-debug [path_filename.patch]
> Is there some other way?
> 

Apart inlininig that create multi-part messages, this is the correct way.

> The other question is, is there some form of watchdog feature in swupdate? 
> I see that for example Webserver and Suricatta spawns new processes

Webserver and suricatta do not spawn any new processed, they are spawned
by the main process.

> but is there some mechanism that monitors whether that process freezes?

It is handled if one of the spawned processes die, resulting in an ERROR
and the main process stops with error.

> We are wondering if swupdate could ping external watchdog

This is not - in many projects using systemd, processes are monitored by
systemd that triggers the hardware watchdog, too. Triggering the
hardware watchdog should be done by a single process, not by all
processes including SWUpdate.

> and with that, we wonder does the internal sub-processes are monitored against freezing.
> 
> Thanks for your previous response, right now according to your suggestion we changed handler to use nng library. 

ok - feel free to post it again for inclusion. I will review and discuss
with it how to merge it in mainline.

Best regards,
Stefano Babic

> 
> Best regards,
> Wojciech Michna
> 
> -----Original Message-----
> From: Stefano Babic [mailto:sbabic@denx.de] 
> Sent: Monday, March 4, 2019 10:34 AM
> To: Michna, Wojciech <Wojciech.Michna@assaabloy.com>; swupdate@googlegroups.com
> Subject: Re: [swupdate] [PATCH] Remote Image Handler based on Nanomsg library
> 
> Hi Wojciech,
> 
> please do not attach the patch but inline it to make review easier and avoid that mailclient can mishandle it.
> 
> (Use git send-email --to swupdate@googlegroups.com )
> 
> On 26/02/19 12:24, Wojciech Michna wrote:
>>
>> This adds remote handler which uses  Nanomsg library instead of zeromq 
>> library.
> 
> It is not a replacement of zeromq, it is a new handler.
> 
> Have you consider to use the nng library as suggested by nanomsg website ? I do not see new update for nanomsg, and development flows into nng instead.
> 
>> This handlers
> 
> handler
> 
>> works as IPC client
>> in request response configuration.
> 
> Can you formulate again the phrase ? It is not clear to me. Is it like "
> This handler forwards the image " ?
> 
>> It tries to open a connection
>>  to a path defined in the data field and send image to local IPC server.
>>
>> Signed-off-by: Wojciech Michna <Wojciech.Michna@assaabloy.com>
>> ---
>>  Makefile.flags             |   4 +
>>  handlers/Config.in         |  10 ++
>>  handlers/Makefile          |   1 +
>>  handlers/nanomsg_handler.c | 301 
>> +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 316 insertions(+)
>>  create mode 100644 handlers/nanomsg_handler.c
>>
> 
>>
>>
>> diff --git a/Makefile.flags b/Makefile.flags index 82365a5..d991ad3 
>> 100644
>> --- a/Makefile.flags
>> +++ b/Makefile.flags
>> @@ -173,6 +173,10 @@  ifeq ($(CONFIG_REMOTE_HANDLER),y)  LDLIBS += zmq  
>> endif
>>  
>> +ifeq ($(CONFIG_NANOMSG_HANDLER),y)
>> +LDLIBS += nanomsg
>> +endif
>> +
> 
> I need to see the corresponding patches for meta-swupdate, else build is broken if nanomsg is set.
> 
>>  ifeq ($(CONFIG_UCFWHANDLER),y)
>>  LDLIBS += gpiod
>>  endif
>> diff --git a/handlers/Config.in b/handlers/Config.in index 
>> 91d43f3..cb5816f 100644
>> --- a/handlers/Config.in
>> +++ b/handlers/Config.in
>> @@ -177,6 +177,16 @@  config REMOTE_HANDLER
>>  	  the image to be updated with the help of the
>>  	  zeromq library.
>>  
>> +config NANOMSG_HANDLER
>> +	bool "nanomsg handler"
>> +	default n
>> +	help
>> +	  Update handler which communicates with external
>> +	  processes using NanoMsg communication over
>> +	  IPC channel. This handler creates Nanomsg Request
>> +	  client which will try to connect to server running
>> +	  on a client device.
> 
> This is not very clear - I think it is enough to say that the handler forwards the image to a listening process using the nanomsg library.
> Please also add a link to nanomsg (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fnanomsg.org%2F&amp;data=02%7C01%7C%7C4dc23ca996ea4378842808d6a0848969%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C636872888478975177&amp;sdata=gTePS6l9RQlI0qdpsgLlRYbZ%2F7VUjvS0z%2FahbDt0sq0%3D&amp;reserved=0)
> 
>> +
>>  comment "remote handler needs zeromq"
>>  	depends on !HAVE_LIBZEROMQ
>>  
> 
> Wrong - you put your code in between. This is related to the above handler (zeromq).
> 
> You need an equivalent code here for nanomsg or you break
> buildroot.(HAVE_NANOMSG)
> 
>> diff --git a/handlers/Makefile b/handlers/Makefile index 
>> b75c3ba..3fad943 100644
>> --- a/handlers/Makefile
>> +++ b/handlers/Makefile
>> @@ -8,6 +8,7 @@
>>  # Handler can be called dynamically based  # on the received image 
>> type.
>>  obj-y	+= dummy_handler.o
>> +obj-$(CONFIG_NANOMSG_HANDLER) += nanomsg_handler.o
>>  obj-$(CONFIG_ARCHIVE) += archive_handler.o
>>  obj-$(CONFIG_BOOTLOADERHANDLER) += boot_handler.o
>>  obj-$(CONFIG_CFI)	+= flash_handler.o
>> diff --git a/handlers/nanomsg_handler.c b/handlers/nanomsg_handler.c 
>> new file mode 100644 index 0000000..a06596a
>> --- /dev/null
>> +++ b/handlers/nanomsg_handler.c
>> @@ -0,0 +1,301 @@
>> +/*
>> + * (C) Copyright 2019
>> + * Wojciech Michna, Assa Aabloy, Wojciech.Michna@assaabloy.com
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0-or-later
>> + */
>> +
>> +/**
>> + * @file nanomsg_handler.c
>> + * @brief Handler for remote updates using NanoMsg library
>> + *
>> + *
>> + * This file contains update handler which communicates with external
>> + * processes using NanoMsg communication over IPC channel.
>> + * This handler creates Nanomsg Request client which will try
>> + * to connect to server running on a client device. 
>> + *
>> + */
>> +
>> +/* standard includes. */
> 
> This comment is not useful
> 
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <stdlib.h>
>> +#include <errno.h>
>> +
>> +/* nanomsg specific includes. */
> 
> Ditto
> 
>> +#include <nanomsg/nn.h>
>> +#include <nanomsg/reqrep.h>
>> +
>> +/* swupdate specific includes. */
> 
> Ditto
> 
>> +#include "swupdate.h"
>> +#include "handler.h"
>> +#include "util.h"
>> +
>> +#define ACK_MESSAGE "ACK"
>> +#define ACK_MESSAGE_SIZE 3
>> +
>> +#define RET_SUCCESS 0
>> +#define RET_ERROR -1
> 
> I do not like these two generic defines. There is already the non written rule that a SWUpdate's function returns 0 in case of success, and a useful negative value (-ENOMEM, _EBADMSG, ...) in case of error.
> 
>> +
>> +#define LOW_TIMEOUT 1000
>> +#define HIGH_TIMEOUT 20000
> 
> The receive timout is set to 20 seconds - one SWUpdate runs the handlers sequentially in one thread, that means SWUpdate stops for up to 20 seconds. I suggest at least to make the time configurable via a "property" in sw-description.
> 
>> +
>> +#define EOF_SIZE 1
> 
> Waht is EOF here ? I understand from code that there are just C strings, that is Null terminated string. EOF is at least the wrong name and the define is not very useful.
> 
>> +
>> +#define MESSAGE_BUFF_SIZE 64
>> +char message_buff[MESSAGE_BUFF_SIZE];
> 
> What are coming these size ? Can you also better explain your use case ?
> What are you updating with this handler ?
> 
>> +
>> +static void nanomsg_handler(void) __attribute__((constructor)); 
>> +static int send_message_with_ack(int sock, char* message, int n);
>> +
>> +static unsigned int max_send_size = 0; static int 
>> +set_max_send_size(int sock);
>> +
>> +static int send_init(int sock, long long size); static int 
>> +send_data_begin(int sock);
>> +
>> +/**
>> + * @brief Function sending message and waiting for ACK message
>> + *
>> + * @param [in] int sock Socket which will be used to send and receive 
>> +messages
>> + * @param [in] char* message Message which will be send
>> + * @param [in] int n size in bytes of message to send
>> + *
>> + * @return RET_ERROR - indicating send or receive error or 
>> +RET_SUCCESS - indicating success  */ static int 
>> +send_message_with_ack(int sock, char* message, int n){
> 
> space before { or better "{" to newline
> 		
>> +		int bytes = -1;
>> +		int ret = RET_ERROR;
>> +		char tmp[ACK_MESSAGE_SIZE];
> 
> indent is reported as wrong by any "checkpatch" run, even if SWUpdate has not a own one. Parameter should be after the above "("
> 
>> +	
>> +        if ((bytes = nn_send(sock, message, n, 0)) < 0) {
>> +			return ret;
> 
> indent is wrong - this must be fixed globally.
> 
>> +        }
>> +        
>> +        char *buf = NULL;
>> +        
>> +        /*
>> +		 Recieve response from server
> 
> The rule for multiline comment is:
> 
> /*
>  * First line
>  * Secondline
>  */
> 
>> +        */
>> +        if ((bytes = nn_recv(sock, &buf, NN_MSG, 0)) <= 0) {
>> +			return ret;
> 
> I do not like this. You check for error, but when you find it, you overload it with -1. You should add an "ERROR" statement to trace what is happening.
> 
>> +        }
>> +        
>> +        /*
>> +          Check if we received ACK
>> +          Any other message is treated as failure
>> +         */
> 
> Multiline comment
> 
>> +		if(bytes >= ACK_MESSAGE_SIZE){
>> +			memcpy(tmp,buf,ACK_MESSAGE_SIZE);
>> +			if( 0 == strncmp(tmp, ACK_MESSAGE, ACK_MESSAGE_SIZE)){
>> +				ret = RET_SUCCESS;
>> +			}						
>> +		}
> 
> So you forward data and the receiver sends back "ACK" in ASCII format.
> It would bet fine to add a small documentation about this small protocol at the beginning of the file.
> 
>> +		
>> +        nn_freemsg(buf);
>> +		return ret;     
>> +}
>> +
>> +/**
>> + * @brief Function sending INIT message
>> + *
>> + * @param [in] sock Socket which will be used to send and receive 
>> +messages
>> + * @param [in] long long size Size size of image that will be sent
>> + *
>> + * @return RET_ERROR - indicating send or receive error or 
>> +RET_SUCCESS - indicating success  */ static int send_init(int sock, 
>> +long long size){
>> +	snprintf(message_buff, MESSAGE_BUFF_SIZE-EOF_SIZE , "INIT:%lld", 
>> +size);
>                                                 ^-- space is missing
> 
>> +	return 
>> +send_message_with_ack(sock,message_buff,strlen(message_buff));
>> +}
>> +
>> +/**
>> + * @brief Function sending DATA message
>> + *
>> + * @param [in] sock Socket which will be used to send and receive 
>> +messages
>> + *
>> + * @return RET_ERROR - indicating send or receive error or 
>> +RET_SUCCESS - indicating success  */ static int send_data_begin(int 
>> +sock){
>> +	snprintf(message_buff, MESSAGE_BUFF_SIZE-EOF_SIZE , "DATA");
>> +	return 
>> +send_message_with_ack(sock,message_buff,strlen(message_buff));
> 
> return send_message_with_ack(sock, message_buff, strlen(message_buff));
> 
>> +}
>> +
>> +/**
>> + * @brief Function seting maximum size handler is able to send
>> + *
>> + * @param [in] sock Socket from which maximum size will be determined
>> + *
>> + * @return RET_ERROR - indicating failure when accessing socket 
>> +RET_SUCCESS - indicating success  */ static int  
>> +set_max_send_size(int sock){
>> +	int tmp_send_size;
>> +	size_t sz = sizeof (tmp_send_size);
>> +	
>> +	max_send_size = 0;
>> +	
>> +	if (nn_getsockopt (sock, NN_SOL_SOCKET, NN_SNDBUF, &tmp_send_size, &sz) < 0) {
>> +		return RET_ERROR;
>> +	}
>> +	
>> +	max_send_size = (unsigned int)tmp_send_size;
>> +	
>> +	return RET_SUCCESS;
>> +}
> 
> The way how the size is handled looks to me wrong, more in forward data
> 
> 
>> +
>> +/**
>> + * @brief Function sending chunks of image data to server
>> + *
>> + * @param [in] void *request Pointer to socket, have to be populated 
>> +before
>> + * @param [in] const void *buf Pointer to buffer containing chunk of 
>> +data
>> + * @param [in] unsigned int len Buffer Length
>> + *
>> + * @return EFAULT - indicating failure and interrupt further sending 
>> +RET_SUCCESS (HAVE TO BE 0) - indicating success  */ static int 
>> +forward_data(void *request, const void *buf, unsigned int len) {
>> +	int ret = 0;
>> +	int* psock = (int*)request;
>> +	int sock = 0;
>> +	char* tmp = 0x00;
>> +	
>> +	if ( 0 == psock){
> 
> if (!psock)
> 
>> +		return -EFAULT;
>> +	}
>> +
>> +	sock = *psock;
> 
> You do not need sock, just use *psock.
> 
>> +
>> +	if(max_send_size<len){
>> +		return -EFAULT;
>> +	}
> 
> The handling of the size is IMHO wrong. The handler is the sink, not the source, and it cannot handle the size of the data. It just receive data from SWUpdate's core (the copyimage function) after the main processing.
> 
> Just dropping and returning with error does not seem a solution - if a bigger message is received from SWUpdate and cannot be sent via nanomsg,
> forward_data() should split the message in smaller chucnks and iterate until all data is sent.
> 
>> +	
>> +	tmp = (char*) malloc(len+EOF_SIZE);
>> +	memset(tmp,0x00,len+EOF_SIZE);
> 
> You do not need to zero the buffer because you fill in.
> 
>> +	memcpy(tmp,buf,len);
>> +	ret = send_message_with_ack(sock,tmp,len);
>> +	
>> +	free(tmp);
>> +	
>> +	return ret;
>> +}
>> +
>> +/**
>> + * @brief Handler installed in swupdate responsible for handling 
>> +"nanomsg" update type
>> + *
>> + * @param [in] struct img_type *img pointer to update structure
>> + * @param [in] *data unused pointer for compliance with handler 
>> +definition
>> + *
>> + * @return EFAULT - indicating failure and interrupt update 
>> +RET_SUCCESS (HAVE TO BE 0) - indicating success  */ static int 
>> +install_send_to_remote(struct img_type *img,
>> +	void __attribute__ ((__unused__)) *data)
>> +{	
>> +	char *connect_string;
>> +	int len;
>> +	int sock = 0;
>> +	int rv = 0;
>> +	int ret = 0;
>> +	int time_out = 0;
>> +	
>> +	if( 0 == img){
> 
> (!img)
> 
>> +		ERROR("Critical fault no img passed to handler");
>> +		return -EFAULT;
>> +	}
>> +	
>> +	if( 0 == strlen(img->type_data) ){
> 
> !strlen(img->type_data)
> 
>> +		ERROR("No data section, can't open socket");
>> +		return -EFAULT;
>> +	}
>> +	
>> +	len = strlen(img->type_data) 
>> +		+ strlen(CONFIG_SOCKET_REMOTE_HANDLER_DIRECTORY)
>> +		+ strlen("ipc://")
>> +		+ EOF_SIZE;
>> +
>> +	connect_string = (char*)malloc(len);
>> +	
>> +	if (!connect_string) {
>> +		ERROR("Not enough memory");
>> +		return -ENOMEM;
>> +	}
>> +	
>> +	snprintf(	connect_string,
>> +				len,
>> +				"ipc://%s%s",
>> +				CONFIG_SOCKET_REMOTE_HANDLER_DIRECTORY,
>> +				img->type_data);
>> +	
> 
> indent is wrong
> 
>> +	if (( sock = nn_socket(AF_SP, NN_REQ)) < 0) {
>> +		ERROR("Can't create socket");
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +		
>> +	if (( rv = nn_connect (sock, connect_string)) < 0) {
>> +		ERROR("Can't connect to socket");
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +	
>> +	time_out = LOW_TIMEOUT;
>> +	if (nn_setsockopt(sock, NN_SOL_SOCKET, NN_RCVTIMEO, &time_out, sizeof (time_out)) < 0) {
>> +		ERROR("Can't change opt");
>> +		nn_shutdown(sock, rv);
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +
>> +	if(send_init(sock, img->size ) != 0){
>> +		ERROR("Sending init message failed");
>> +		nn_shutdown(sock, rv);
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +	
>> +	if(send_data_begin(sock) != 0){
>> +		ERROR("Sending data message failed");
>> +		nn_shutdown(sock, rv);
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +	
>> +	time_out = HIGH_TIMEOUT;
>> +	if (nn_setsockopt(sock, NN_SOL_SOCKET, NN_RCVTIMEO, &time_out, sizeof (time_out)) < 0) {
>> +		ERROR("Can't change opt");
>> +		nn_shutdown(sock, rv);
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +	
>> +	if (set_max_send_size(sock) != RET_SUCCESS) {
>> +		ERROR("Can't set max send size");
>> +		nn_shutdown(sock, rv);
>> +		free(connect_string);
>> +		return -EFAULT;
>> +	}
>> +	
>> +	ret = copyimage(&sock, img, forward_data);	
>> +		
>> +	nn_shutdown(sock, rv);
>> +	free(connect_string);
>> +	
>> +	if(ret != 0){
>> +		ERROR("Send error");
>> +		return -EFAULT;
>> +	}
>> +	
>> +	return RET_SUCCESS;
>> +}
>> +
>> +/**
>> + * @brief Constructor function run at start of application 
>> +responsible for installing handler  */ static void 
>> +nanomsg_handler(void) {
>> +	register_handler("nanomsg",
>> +					 install_send_to_remote,
>> +					 IMAGE_HANDLER,
>> +					 NULL);
>> +}
>>
> 
> 
> Best regards,
> Stefano Babic
>