diff mbox series

[v3] mongoose: Allow WEB API v1 and v2 both be enabled

Message ID 1519984669-11527-1-git-send-email-sami.hartikainen@teleste.com
State Changes Requested
Headers show
Series [v3] mongoose: Allow WEB API v1 and v2 both be enabled | expand

Commit Message

Hartikainen, Sami March 2, 2018, 9:57 a.m. UTC
To ease transition, or simply to enable the use of modern clients
while still supporting the use of the swuforwarder handler (which
only talks API v1), both the v1 and v2 API may be enabled.

Clients for v1 and v2 should not connect at the *same time*; this
restriction is due to having a simple queue for the status messages,
consumed by the first reader.

Signed-off-by: Sami Hartikainen <sami.hartikainen@teleste.com>
---
 mongoose/Config.in            |  31 +++++---
 mongoose/mongoose_interface.c | 167 +++++++++++++++++++++++++++---------------
 2 files changed, 129 insertions(+), 69 deletions(-)

Comments

Stefano Babic March 2, 2018, 1:19 p.m. UTC | #1
Hi Sami,

read the whole thread but I just answer here:

On 02/03/2018 10:57, Sami Hartikainen wrote:
> To ease transition, or simply to enable the use of modern clients
> while still supporting the use of the swuforwarder handler (which
> only talks API v1), both the v1 and v2 API may be enabled.
> 

The first question I have is regarding the use case. I wrote the
swuforwarder handler to take care when I have a device connected to
external network, let's say a gateway, and other devices just connected
to the previous one in a separate and not reachable network. The gateway
must be in charge to update itself and the other ones, and using again
http on the internal netwok gives me automatically the possibility to
encrypt it with SSL - in cases where this is required or high desired.

But just the gateway nees to have a fancy interface - this is the
interface to the world and to the operator and runs on the browser. If
we have a machine-to-machine update, there is no need that it looks
nice. This use case foresees to have V2 running on the gateway, and V1
on the other devices. I do not see issue with it. V1 and V2 are not
running on the same machine.

Now it seems you have a different use case, that you coul maybe better
explained. In the above concept, it looks you want that all machines are
connected to the public network, and they could also updated by another
device as well. There is no hierarchy with gateway / devices as far as I
understand. Or do you have found an issue with current implementation ?

> Clients for v1 and v2 should not connect at the *same time*; this
> restriction is due to having a simple queue for the status messages,
> consumed by the first reader.

This is a hard constraint and it is not acceptable. We cannot guarantee
what the operator is doing, and even less we cannot guarantee that his
behavior is fair.

> 
> Signed-off-by: Sami Hartikainen <sami.hartikainen@teleste.com>
> ---
>  mongoose/Config.in            |  31 +++++---
>  mongoose/mongoose_interface.c | 167 +++++++++++++++++++++++++++---------------
>  2 files changed, 129 insertions(+), 69 deletions(-)
> 
> diff --git a/mongoose/Config.in b/mongoose/Config.in
> index a001247..2d66bf3 100644
> --- a/mongoose/Config.in
> +++ b/mongoose/Config.in
> @@ -14,27 +14,40 @@ choice
>  config MONGOOSE
>  	bool "mongoose"
>  	help
> -	  Mongoose embeddded web server
> +	  Mongoose embedded web server
>  
>  endchoice
>  
> -choice
> -	prompt "Web Application Interface"
> -	default MONGOOSE_WEB_API_V1
> -	help
> -	  Choose the bootloader
> +menu "Web Application Interface"
>  
>  config MONGOOSE_WEB_API_V1
>  	bool "Version 1 (deprecated)"
> +	default y
>  	help
> -	  Support for version 1
> +	  Support for version 1.
> +
> +	  API version 1 consists of the following URIs:
> +	   - /handle_post_request (upload .swu image)
> +	   - /getstatus.json (poll update status)
> +	   - /rebootTarget
> +	   - /postUpdateCommand
>  
>  config MONGOOSE_WEB_API_V2
>  	bool "Version 2"
> +	default y
>  	help
> -	  Support for version 2
> +	  Support for version 2.
>  
> -endchoice

It should be still discussed if both are allowed to be set on the same
device. Not sure why, maybe you can better explain this.

> +	  API version 2 consists of the following URIs:
> +	   - /upload (upload .swu image)
> +	   - /restart
> +
> +	  The update status is received via WebSocket interface.
> +
> +comment "Web API v1 or v2 or both required"
> +	depends on !MONGOOSE_WEB_API_V1 && !MONGOOSE_WEB_API_V2
> +
> +endmenu
>  
>  config MONGOOSEIPV6
>  	bool "IPv6 support"
> diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c
> index 89a51f3..2058ce3 100644
> --- a/mongoose/mongoose_interface.c
> +++ b/mongoose/mongoose_interface.c
> @@ -25,6 +25,7 @@
>  #include <parselib.h>
>  #include <progress_ipc.h>
>  #include <swupdate_settings.h>
> +#include <util.h>
>  
>  #include "mongoose.h"
>  
> @@ -49,6 +50,10 @@ struct file_upload_state {
>  
>  static struct mg_serve_http_opts s_http_server_opts;
>  
> +#if !defined(CONFIG_MONGOOSE_WEB_API_V1) && !defined(CONFIG_MONGOOSE_WEB_API_V2)
> +#error "WEB API v1 or v2 or both must be defined"
> +#endif

This does not make sense and it should be solved by Kconfig. That means,
we should have V1/V2/V1+V2, and the case does not exist.

> +
>  #if defined(CONFIG_MONGOOSE_WEB_API_V2)
>  #define enum_string(x)	[x] = #x
>  static const char *get_status_string(unsigned int status)
> @@ -116,16 +121,72 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p)
>  {

To be consistent: if you introduce upload_handler_v1, this should be
upload_handler_v2.

>  	struct mg_http_multipart_part *mp;
>  	struct file_upload_state *fus;
> +
> +	switch (ev) {
> +	case MG_EV_HTTP_PART_BEGIN:
> +		mp = (struct mg_http_multipart_part *) p;
> +
> +		fus = (struct file_upload_state *) calloc(1, sizeof(*fus));
> +		if (fus == NULL) {
> +			mg_http_send_error(nc, 500, "Out of memory");
> +			break;
> +		}
> +
> +		fus->fd = ipc_inst_start_ext(SOURCE_WEBSERVER, strlen(mp->file_name), mp->file_name);
> +		if (fus->fd < 0) {
> +			mg_http_send_error(nc, 500, "Failed to queue command");
> +			free(fus);
> +			break;
> +		}
> +
> +		mp->user_data = fus;
> +
> +		break;
> +
> +	case MG_EV_HTTP_PART_DATA:
> +		mp = (struct mg_http_multipart_part *) p;
> +		fus = (struct file_upload_state *) mp->user_data;
> +
> +		if (!fus)
> +			break;
> +
> +		ipc_send_data(fus->fd, (char *) mp->data.p, mp->data.len);
> +		fus->len += mp->data.len;
> +
> +		break;
> +
> +	case MG_EV_HTTP_PART_END:
> +		mp = (struct mg_http_multipart_part *) p;
> +		fus = (struct file_upload_state *) mp->user_data;
> +
> +		if (!fus)
> +			break;
> +
> +		ipc_end(fus->fd);
> +
> +		mg_send_response_line(nc, 200,
> +			"Content-Type: text/plain\r\n"
> +			"Connection: close");
> +		mg_send(nc, "\r\n", 2);
> +		mg_printf(nc, "Ok, %s - %d bytes.\r\n", mp->file_name, (int) fus->len);
> +		nc->flags |= MG_F_SEND_AND_CLOSE;
> +
> +		mp->user_data = NULL;
> +		free(fus);
> +		break;
> +	}
> +}
> +
>  #if defined(CONFIG_MONGOOSE_WEB_API_V1)
> +static void upload_handler_v1(struct mg_connection *nc, int ev, void *p)
> +{
>  	struct mg_str *filename, *data;
>  	struct http_message *hm;
>  	size_t length;
>  	char buf[16];
>  	int fd;
> -#endif

Splitting into two functions makes the code more readable, I agree.

>  
>  	switch (ev) {
> -#if defined(CONFIG_MONGOOSE_WEB_API_V1)
>  	case MG_EV_HTTP_REQUEST:
>  		hm = (struct http_message *) p;
>  
> @@ -161,62 +222,13 @@ static void upload_handler(struct mg_connection *nc, int ev, void *p)
>  		nc->flags |= MG_F_SEND_AND_CLOSE;
>  
>  		break;
> -#endif
> -	case MG_EV_HTTP_PART_BEGIN:
> -		mp = (struct mg_http_multipart_part *) p;
>  
> -		fus = (struct file_upload_state *) calloc(1, sizeof(*fus));
> -		if (fus == NULL) {
> -			mg_http_send_error(nc, 500, "Out of memory");
> -			break;
> -		}
> -
> -		fus->fd = ipc_inst_start_ext(SOURCE_WEBSERVER, strlen(mp->file_name), mp->file_name);
> -		if (fus->fd < 0) {
> -			mg_http_send_error(nc, 500, "Failed to queue command");
> -			free(fus);
> -			break;
> -		}
> -
> -		mp->user_data = fus;
> -
> -		break;
> -
> -	case MG_EV_HTTP_PART_DATA:
> -		mp = (struct mg_http_multipart_part *) p;
> -		fus = (struct file_upload_state *) mp->user_data;
> -
> -		if (!fus)
> -			break;
> -
> -		ipc_send_data(fus->fd, (char *) mp->data.p, mp->data.len);
> -		fus->len += mp->data.len;
> -
> -		break;
> -
> -	case MG_EV_HTTP_PART_END:
> -		mp = (struct mg_http_multipart_part *) p;
> -		fus = (struct file_upload_state *) mp->user_data;
> -
> -		if (!fus)
> -			break;
> -
> -		ipc_end(fus->fd);
> -
> -		mg_send_response_line(nc, 200,
> -			"Content-Type: text/plain\r\n"
> -			"Connection: close");
> -		mg_send(nc, "\r\n", 2);
> -		mg_printf(nc, "Ok, %s - %d bytes.\r\n", mp->file_name, (int) fus->len);
> -		nc->flags |= MG_F_SEND_AND_CLOSE;
> -
> -		mp->user_data = NULL;
> -		free(fus);
> +	default:
> +		upload_handler(nc, ev, p);
>  		break;
>  	}
>  }
>  
> -#if defined(CONFIG_MONGOOSE_WEB_API_V1)
>  static void recovery_status(struct mg_connection *nc, int ev, void *ev_data)
>  {
>  	ipc_message ipc;
> @@ -306,7 +318,9 @@ static void post_update_cmd(struct mg_connection *nc, int ev, void *ev_data)
>  
>  	nc->flags |= MG_F_SEND_AND_CLOSE;
>  }
> -#elif defined(CONFIG_MONGOOSE_WEB_API_V2)
> +#endif
> +
> +#if defined(CONFIG_MONGOOSE_WEB_API_V2)
>  static void restart_handler(struct mg_connection *nc, int ev, void *ev_data)
>  {
>  	struct http_message *hm = (struct http_message *) ev_data;
> @@ -462,14 +476,48 @@ static void *broadcast_progress_thread(void *data)
>  
>  	return NULL;
>  }
> +
> +static int websocket_count(struct mg_mgr *mgr)
> +{
> +	struct mg_connection *c;
> +	int result = 0;
> +
> +	for (c = mg_next(mgr, NULL); c != NULL; c = mg_next(mgr, c)) {
> +		if ((c->flags & MG_F_IS_WEBSOCKET))
> +			result++;
> +	}
> +
> +	return result;
> +}
>  #endif
>  
>  static void ev_handler(struct mg_connection *nc, int ev, void *ev_data)
>  {
> +#if defined(CONFIG_MONGOOSE_WEB_API_V2)
> +	static pthread_t bcast_thread_id;
> +
> +	switch (ev) {
> +	case MG_EV_WEBSOCKET_HANDSHAKE_DONE:
> +		if (websocket_count(nc->mgr) == 1) {
> +			DEBUG("Creating websocket broadcast message thread\n");
> +			bcast_thread_id = (pthread_t)mg_start_thread(broadcast_message_thread, nc->mgr);
> +		}
> +		break;
> +
> +	case MG_EV_CLOSE:
> +		if ((nc->flags & MG_F_IS_WEBSOCKET)
> +		    && websocket_count(nc->mgr) == 0) {
> +			DEBUG("Canceling websocket broadcast message thread\n");
> +			if (!pthread_cancel(bcast_thread_id))
> +				pthread_join(bcast_thread_id, NULL);
> +		}
> +		break;
> +	}
> +#endif
> +
>  	if (ev == MG_EV_HTTP_REQUEST) {
>  		mg_serve_http(nc, ev_data, s_http_server_opts);
>  	}
> -
>  }
>  
>  static int mongoose_settings(void *elem, void  __attribute__ ((__unused__)) *data)
> @@ -505,7 +553,6 @@ static int mongoose_settings(void *elem, void  __attribute__ ((__unused__)) *dat
>  	return 0;
>  }
>  
> -
>  static struct option long_options[] = {
>  	{"listing", no_argument, NULL, 'l'},
>  	{"port", required_argument, NULL, 'p'},
> @@ -611,18 +658,18 @@ int start_mongoose(const char *cfgfname, int argc, char *argv[])
>  	}
>  
>  #if defined(CONFIG_MONGOOSE_WEB_API_V1)
> -	mg_register_http_endpoint(nc, "/handle_post_request", MG_CB(upload_handler, NULL));
> +	mg_register_http_endpoint(nc, "/handle_post_request", MG_CB(upload_handler_v1, NULL));
>  	mg_register_http_endpoint(nc, "/getstatus.json", MG_CB(recovery_status, NULL));
>  	mg_register_http_endpoint(nc, "/rebootTarget", MG_CB(reboot_target, NULL));
>  	mg_register_http_endpoint(nc, "/postUpdateCommand", MG_CB(post_update_cmd, NULL));
> -#elif defined(CONFIG_MONGOOSE_WEB_API_V2)
> -	mg_register_http_endpoint(nc, "/restart", restart_handler);
>  #endif
> +#if defined(CONFIG_MONGOOSE_WEB_API_V2)
> +	mg_register_http_endpoint(nc, "/restart", MG_CB(restart_handler, NULL));
>  	mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL));
> +#endif
>  	mg_set_protocol_http_websocket(nc);
>  
>  #if defined(CONFIG_MONGOOSE_WEB_API_V2)
> -	mg_start_thread(broadcast_message_thread, &mgr);
>  	mg_start_thread(broadcast_progress_thread, &mgr);
>  #endif
>  
> 

You could maybe better explain why we need V1 and V2 at the same time. I
am still missing the reason.

Best regards,
Stefano Babic
Hartikainen, Sami March 2, 2018, 2:47 p.m. UTC | #2
Hi Stefano,

> The first question I have is regarding the use case. I wrote the swuforwarder
> handler to take care when I have a device connected to external network,
> let's say a gateway, and other devices just connected to the previous one in a
> separate and not reachable network. The gateway must be in charge to
> update itself and the other ones, and using again http on the internal netwok
> gives me automatically the possibility to encrypt it with SSL - in cases where
> this is required or high desired.
> 
> But just the gateway nees to have a fancy interface - this is the interface to
> the world and to the operator and runs on the browser. If we have a
> machine-to-machine update, there is no need that it looks nice. This use case
> foresees to have V2 running on the gateway, and V1 on the other devices. I
> do not see issue with it. V1 and V2 are not running on the same machine.
> 
> Now it seems you have a different use case, that you coul maybe better
> explained. In the above concept, it looks you want that all machines are
> connected to the public network, and they could also updated by another
> device as well. There is no hierarchy with gateway / devices as far as I
> understand. Or do you have found an issue with current implementation ?

Hierarchy with "gateway" / devices does exist, but the gateway is just proxying
(after auth etc.) all traffic through to devices. Thus the preference to have the
"nice" interface there.

Future renewal of the "gateway" is expected to run SWUpdate, and with it
the devices shall be updated via swuforwarder.

We are not willing to maintain several different swupdate .deb packages, so if
both v1 and v2 were supported with the same binary, the above scenario would
be possible.

I understand that this is likely an extremely rare use case. We can manage by
sticking with v1.

> > Clients for v1 and v2 should not connect at the *same time*; this
> > restriction is due to having a simple queue for the status messages,
> > consumed by the first reader.
> 
> This is a hard constraint and it is not acceptable. We cannot guarantee what
> the operator is doing, and even less we cannot guarantee that his behavior is
> fair.

Ok.

Some comments below.

> > +#if !defined(CONFIG_MONGOOSE_WEB_API_V1) &&
> > +!defined(CONFIG_MONGOOSE_WEB_API_V2)
> > +#error "WEB API v1 or v2 or both must be defined"
> > +#endif
> 
> This does not make sense and it should be solved by Kconfig. That means, we
> should have V1/V2/V1+V2, and the case does not exist.

I assume you mean to have a third Kconfig entry like:

    config MONGOOSE_WEB_API_V1N2
        bool "Support for both version 1 and 2."
        select MONGOOSE_WEB_API_V1
        select MONGOOSE_WEB_API_V2

That won't work. I did not find a clever way to do it.

> >  #if defined(CONFIG_MONGOOSE_WEB_API_V2)
> >  #define enum_string(x)	[x] = #x
> >  static const char *get_status_string(unsigned int status) @@ -116,16
> > +121,72 @@ static void upload_handler(struct mg_connection *nc, int
> > ev, void *p)  {
> 
> To be consistent: if you introduce upload_handler_v1, this should be
> upload_handler_v2.

The default switch case of the upload_handler_v1 calls this function, and it
would be silly if it called a _v2 function. 

Maybe a third function "upload_handler_common" which is called by both
v1 and v2 upload_handler?

Br, Sami
Stefano Babic March 2, 2018, 4:36 p.m. UTC | #3
Hi Sami,

On 02/03/2018 15:47, Sami.Hartikainen@teleste.com wrote:
> Hi Stefano,
> 
>> The first question I have is regarding the use case. I wrote the swuforwarder
>> handler to take care when I have a device connected to external network,
>> let's say a gateway, and other devices just connected to the previous one in a
>> separate and not reachable network. The gateway must be in charge to
>> update itself and the other ones, and using again http on the internal netwok
>> gives me automatically the possibility to encrypt it with SSL - in cases where
>> this is required or high desired.
>>
>> But just the gateway nees to have a fancy interface - this is the interface to
>> the world and to the operator and runs on the browser. If we have a
>> machine-to-machine update, there is no need that it looks nice. This use case
>> foresees to have V2 running on the gateway, and V1 on the other devices. I
>> do not see issue with it. V1 and V2 are not running on the same machine.
>>
>> Now it seems you have a different use case, that you coul maybe better
>> explained. In the above concept, it looks you want that all machines are
>> connected to the public network, and they could also updated by another
>> device as well. There is no hierarchy with gateway / devices as far as I
>> understand. Or do you have found an issue with current implementation ?
> 
> Hierarchy with "gateway" / devices does exist, but the gateway is just proxying
> (after auth etc.) all traffic through to devices. Thus the preference to have the
> "nice" interface there.

This is also a known use case - and it should be fixed with your
previous patch " www: web-app: Formulate URIs to be reverse proxy
-friendly".

However, I do not understand then why do you still both. If the gateway
is acting as proxy, you do not need the swuforwarder : your gateway has
not SWUpdate running and it is not extracting the SWU images for the
other devices. You should just need V2 on all devices. Am I wrong ?

> 
> Future renewal of the "gateway" is expected to run SWUpdate, and with it
> the devices shall be updated via swuforwarder.

Well, then you will have V2 on gateway and V1 on the devices, but not
both. ipc_get_status()

> 
> We are not willing to maintain several different swupdate .deb packages, so if
> both v1 and v2 were supported with the same binary, the above scenario would
> be possible.

Ok, but if we want to get this we need in a reliable way. The issue with
the queue for the status message should be solved.

The mongoose interface after Stefan's changes is already listening from
the progress interface - or, swuforwarder is changed to support V2 and
then all references to V1 are removed.
> 
> I understand that this is likely an extremely rare use case. We can manage by
> sticking with v1.
> 
>>> Clients for v1 and v2 should not connect at the *same time*; this
>>> restriction is due to having a simple queue for the status messages,
>>> consumed by the first reader.
>>
>> This is a hard constraint and it is not acceptable. We cannot guarantee what
>> the operator is doing, and even less we cannot guarantee that his behavior is
>> fair.
> 
> Ok.
> 
> Some comments below.
> 
>>> +#if !defined(CONFIG_MONGOOSE_WEB_API_V1) &&
>>> +!defined(CONFIG_MONGOOSE_WEB_API_V2)
>>> +#error "WEB API v1 or v2 or both must be defined"
>>> +#endif
>>
>> This does not make sense and it should be solved by Kconfig. That means, we
>> should have V1/V2/V1+V2, and the case does not exist.
> 
> I assume you mean to have a third Kconfig entry like:
> 
>     config MONGOOSE_WEB_API_V1N2
>         bool "Support for both version 1 and 2."
>         select MONGOOSE_WEB_API_V1
>         select MONGOOSE_WEB_API_V2
> 
> That won't work. I did not find a clever way to do it.
> 
>>>  #if defined(CONFIG_MONGOOSE_WEB_API_V2)
>>>  #define enum_string(x)	[x] = #x
>>>  static const char *get_status_string(unsigned int status) @@ -116,16
>>> +121,72 @@ static void upload_handler(struct mg_connection *nc, int
>>> ev, void *p)  {
>>
>> To be consistent: if you introduce upload_handler_v1, this should be
>> upload_handler_v2.
> 
> The default switch case of the upload_handler_v1 calls this function, and it
> would be silly if it called a _v2 function. 
> 
> Maybe a third function "upload_handler_common" which is called by both
> v1 and v2 upload_handler?
> 

Best regards,
Stefano
Hartikainen, Sami March 5, 2018, 8:34 a.m. UTC | #4
Hi Stefano,

> However, I do not understand then why do you still both. If the gateway is
> acting as proxy, you do not need the swuforwarder : your gateway has not
> SWUpdate running and it is not extracting the SWU images for the other
> devices. You should just need V2 on all devices. Am I wrong ?

You are not wrong.

> > Future renewal of the "gateway" is expected to run SWUpdate, and with
> > it the devices shall be updated via swuforwarder.
> 
> Well, then you will have V2 on gateway and V1 on the devices, but not both.

Yes, but that means two different builds of swupdate.

> > We are not willing to maintain several different swupdate .deb
> > packages, so if both v1 and v2 were supported with the same binary,
> > the above scenario would be possible.
> 
> Ok, but if we want to get this we need in a reliable way. The issue with the
> queue for the status message should be solved.

The first patch solved the issue by maintaining an additional queue containing
copies of the messages for v1 clients. It worked but was not an elegant solution.

While it would be beneficial to have both v1 and v2 supported by a single swupdate
binary, a single *instance* of it does not need to support both. For the use case above
it would suffice if the API version enabled at startup could be chosen via webserver
command-line and/or config file option.

Would this be acceptable?

The default API version, if no config option was given, would be chosen by the
Kconfig settings, enabling old .config files that already define e.g. MONGOOSE_WEB_API_V2
to produce the expected functionality.

Br, Sami
Stefano Babic March 5, 2018, 9:37 a.m. UTC | #5
Hi Sami,

On 05/03/2018 09:34, Sami.Hartikainen@teleste.com wrote:
> Hi Stefano,
> 
>> However, I do not understand then why do you still both. If the gateway is
>> acting as proxy, you do not need the swuforwarder : your gateway has not
>> SWUpdate running and it is not extracting the SWU images for the other
>> devices. You should just need V2 on all devices. Am I wrong ?
> 
> You are not wrong.

Good.

> 
>>> Future renewal of the "gateway" is expected to run SWUpdate, and with
>>> it the devices shall be updated via swuforwarder.
>>
>> Well, then you will have V2 on gateway and V1 on the devices, but not both.
> 
> Yes, but that means two different builds of swupdate.
> 

This is a different issue. I am also thinking about that having both
dialects running could introduce some security leaks, because some URLs
are exposed even when they are not required: V1 URLs when V2 is running,...

And introducing websocket support for the SWU forwarder handler seems to
me overkilling.

I thought at this yesterday and I think the best way is to set the web
version at runtime.

>>> We are not willing to maintain several different swupdate .deb
>>> packages, so if both v1 and v2 were supported with the same binary,
>>> the above scenario would be possible.
>>
>> Ok, but if we want to get this we need in a reliable way. The issue with the
>> queue for the status message should be solved.
> 
> The first patch solved the issue by maintaining an additional queue containing
> copies of the messages for v1 clients. It worked but was not an elegant solution.
> 
> While it would be beneficial to have both v1 and v2 supported by a single swupdate
> binary, a single *instance* of it does not need to support both.

Exactly. It is enough to switch between them when SWUpdate is started.

> For the use case above
> it would suffice if the API version enabled at startup could be chosen via webserver
> command-line and/or config file option.
> 
> Would this be acceptable?
> 

I thought the same and I have *already* written a patch for this. Just
tested as V2, it must be still tested.

I will send it now as it is - you could give it a try. I won't merge it
until tests are completed.

> The default API version, if no config option was given, would be chosen by the
> Kconfig settings,

Too much. I dropped all CONFIG_WEB_*, and code results cleaner by
dropping the nasty #ifdef.

Default version is V2 if not set at the startup.

> enabling old .config files that already define e.g. MONGOOSE_WEB_API_V2
> to produce the expected functionality.
> 

Patch will follow.

Best regards,
Stefano Babic
Hartikainen, Sami March 5, 2018, 10:31 a.m. UTC | #6
Hi Stefano,

> > For the use case above
> > it would suffice if the API version enabled at startup could be chosen
> > via webserver command-line and/or config file option.
> >
> > Would this be acceptable?
> >
> 
> I thought the same and I have *already* written a patch for this. Just tested
> as V2, it must be still tested.
> 
> I will send it now as it is - you could give it a try. I won't merge it until tests are
> completed.

Great! I'll test the patch and report back.

Br, Sami
diff mbox series

Patch

diff --git a/mongoose/Config.in b/mongoose/Config.in
index a001247..2d66bf3 100644
--- a/mongoose/Config.in
+++ b/mongoose/Config.in
@@ -14,27 +14,40 @@  choice
 config MONGOOSE
 	bool "mongoose"
 	help
-	  Mongoose embeddded web server
+	  Mongoose embedded web server
 
 endchoice
 
-choice
-	prompt "Web Application Interface"
-	default MONGOOSE_WEB_API_V1
-	help
-	  Choose the bootloader
+menu "Web Application Interface"
 
 config MONGOOSE_WEB_API_V1
 	bool "Version 1 (deprecated)"
+	default y
 	help
-	  Support for version 1
+	  Support for version 1.
+
+	  API version 1 consists of the following URIs:
+	   - /handle_post_request (upload .swu image)
+	   - /getstatus.json (poll update status)
+	   - /rebootTarget
+	   - /postUpdateCommand
 
 config MONGOOSE_WEB_API_V2
 	bool "Version 2"
+	default y
 	help
-	  Support for version 2
+	  Support for version 2.
 
-endchoice
+	  API version 2 consists of the following URIs:
+	   - /upload (upload .swu image)
+	   - /restart
+
+	  The update status is received via WebSocket interface.
+
+comment "Web API v1 or v2 or both required"
+	depends on !MONGOOSE_WEB_API_V1 && !MONGOOSE_WEB_API_V2
+
+endmenu
 
 config MONGOOSEIPV6
 	bool "IPv6 support"
diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c
index 89a51f3..2058ce3 100644
--- a/mongoose/mongoose_interface.c
+++ b/mongoose/mongoose_interface.c
@@ -25,6 +25,7 @@ 
 #include <parselib.h>
 #include <progress_ipc.h>
 #include <swupdate_settings.h>
+#include <util.h>
 
 #include "mongoose.h"
 
@@ -49,6 +50,10 @@  struct file_upload_state {
 
 static struct mg_serve_http_opts s_http_server_opts;
 
+#if !defined(CONFIG_MONGOOSE_WEB_API_V1) && !defined(CONFIG_MONGOOSE_WEB_API_V2)
+#error "WEB API v1 or v2 or both must be defined"
+#endif
+
 #if defined(CONFIG_MONGOOSE_WEB_API_V2)
 #define enum_string(x)	[x] = #x
 static const char *get_status_string(unsigned int status)
@@ -116,16 +121,72 @@  static void upload_handler(struct mg_connection *nc, int ev, void *p)
 {
 	struct mg_http_multipart_part *mp;
 	struct file_upload_state *fus;
+
+	switch (ev) {
+	case MG_EV_HTTP_PART_BEGIN:
+		mp = (struct mg_http_multipart_part *) p;
+
+		fus = (struct file_upload_state *) calloc(1, sizeof(*fus));
+		if (fus == NULL) {
+			mg_http_send_error(nc, 500, "Out of memory");
+			break;
+		}
+
+		fus->fd = ipc_inst_start_ext(SOURCE_WEBSERVER, strlen(mp->file_name), mp->file_name);
+		if (fus->fd < 0) {
+			mg_http_send_error(nc, 500, "Failed to queue command");
+			free(fus);
+			break;
+		}
+
+		mp->user_data = fus;
+
+		break;
+
+	case MG_EV_HTTP_PART_DATA:
+		mp = (struct mg_http_multipart_part *) p;
+		fus = (struct file_upload_state *) mp->user_data;
+
+		if (!fus)
+			break;
+
+		ipc_send_data(fus->fd, (char *) mp->data.p, mp->data.len);
+		fus->len += mp->data.len;
+
+		break;
+
+	case MG_EV_HTTP_PART_END:
+		mp = (struct mg_http_multipart_part *) p;
+		fus = (struct file_upload_state *) mp->user_data;
+
+		if (!fus)
+			break;
+
+		ipc_end(fus->fd);
+
+		mg_send_response_line(nc, 200,
+			"Content-Type: text/plain\r\n"
+			"Connection: close");
+		mg_send(nc, "\r\n", 2);
+		mg_printf(nc, "Ok, %s - %d bytes.\r\n", mp->file_name, (int) fus->len);
+		nc->flags |= MG_F_SEND_AND_CLOSE;
+
+		mp->user_data = NULL;
+		free(fus);
+		break;
+	}
+}
+
 #if defined(CONFIG_MONGOOSE_WEB_API_V1)
+static void upload_handler_v1(struct mg_connection *nc, int ev, void *p)
+{
 	struct mg_str *filename, *data;
 	struct http_message *hm;
 	size_t length;
 	char buf[16];
 	int fd;
-#endif
 
 	switch (ev) {
-#if defined(CONFIG_MONGOOSE_WEB_API_V1)
 	case MG_EV_HTTP_REQUEST:
 		hm = (struct http_message *) p;
 
@@ -161,62 +222,13 @@  static void upload_handler(struct mg_connection *nc, int ev, void *p)
 		nc->flags |= MG_F_SEND_AND_CLOSE;
 
 		break;
-#endif
-	case MG_EV_HTTP_PART_BEGIN:
-		mp = (struct mg_http_multipart_part *) p;
 
-		fus = (struct file_upload_state *) calloc(1, sizeof(*fus));
-		if (fus == NULL) {
-			mg_http_send_error(nc, 500, "Out of memory");
-			break;
-		}
-
-		fus->fd = ipc_inst_start_ext(SOURCE_WEBSERVER, strlen(mp->file_name), mp->file_name);
-		if (fus->fd < 0) {
-			mg_http_send_error(nc, 500, "Failed to queue command");
-			free(fus);
-			break;
-		}
-
-		mp->user_data = fus;
-
-		break;
-
-	case MG_EV_HTTP_PART_DATA:
-		mp = (struct mg_http_multipart_part *) p;
-		fus = (struct file_upload_state *) mp->user_data;
-
-		if (!fus)
-			break;
-
-		ipc_send_data(fus->fd, (char *) mp->data.p, mp->data.len);
-		fus->len += mp->data.len;
-
-		break;
-
-	case MG_EV_HTTP_PART_END:
-		mp = (struct mg_http_multipart_part *) p;
-		fus = (struct file_upload_state *) mp->user_data;
-
-		if (!fus)
-			break;
-
-		ipc_end(fus->fd);
-
-		mg_send_response_line(nc, 200,
-			"Content-Type: text/plain\r\n"
-			"Connection: close");
-		mg_send(nc, "\r\n", 2);
-		mg_printf(nc, "Ok, %s - %d bytes.\r\n", mp->file_name, (int) fus->len);
-		nc->flags |= MG_F_SEND_AND_CLOSE;
-
-		mp->user_data = NULL;
-		free(fus);
+	default:
+		upload_handler(nc, ev, p);
 		break;
 	}
 }
 
-#if defined(CONFIG_MONGOOSE_WEB_API_V1)
 static void recovery_status(struct mg_connection *nc, int ev, void *ev_data)
 {
 	ipc_message ipc;
@@ -306,7 +318,9 @@  static void post_update_cmd(struct mg_connection *nc, int ev, void *ev_data)
 
 	nc->flags |= MG_F_SEND_AND_CLOSE;
 }
-#elif defined(CONFIG_MONGOOSE_WEB_API_V2)
+#endif
+
+#if defined(CONFIG_MONGOOSE_WEB_API_V2)
 static void restart_handler(struct mg_connection *nc, int ev, void *ev_data)
 {
 	struct http_message *hm = (struct http_message *) ev_data;
@@ -462,14 +476,48 @@  static void *broadcast_progress_thread(void *data)
 
 	return NULL;
 }
+
+static int websocket_count(struct mg_mgr *mgr)
+{
+	struct mg_connection *c;
+	int result = 0;
+
+	for (c = mg_next(mgr, NULL); c != NULL; c = mg_next(mgr, c)) {
+		if ((c->flags & MG_F_IS_WEBSOCKET))
+			result++;
+	}
+
+	return result;
+}
 #endif
 
 static void ev_handler(struct mg_connection *nc, int ev, void *ev_data)
 {
+#if defined(CONFIG_MONGOOSE_WEB_API_V2)
+	static pthread_t bcast_thread_id;
+
+	switch (ev) {
+	case MG_EV_WEBSOCKET_HANDSHAKE_DONE:
+		if (websocket_count(nc->mgr) == 1) {
+			DEBUG("Creating websocket broadcast message thread\n");
+			bcast_thread_id = (pthread_t)mg_start_thread(broadcast_message_thread, nc->mgr);
+		}
+		break;
+
+	case MG_EV_CLOSE:
+		if ((nc->flags & MG_F_IS_WEBSOCKET)
+		    && websocket_count(nc->mgr) == 0) {
+			DEBUG("Canceling websocket broadcast message thread\n");
+			if (!pthread_cancel(bcast_thread_id))
+				pthread_join(bcast_thread_id, NULL);
+		}
+		break;
+	}
+#endif
+
 	if (ev == MG_EV_HTTP_REQUEST) {
 		mg_serve_http(nc, ev_data, s_http_server_opts);
 	}
-
 }
 
 static int mongoose_settings(void *elem, void  __attribute__ ((__unused__)) *data)
@@ -505,7 +553,6 @@  static int mongoose_settings(void *elem, void  __attribute__ ((__unused__)) *dat
 	return 0;
 }
 
-
 static struct option long_options[] = {
 	{"listing", no_argument, NULL, 'l'},
 	{"port", required_argument, NULL, 'p'},
@@ -611,18 +658,18 @@  int start_mongoose(const char *cfgfname, int argc, char *argv[])
 	}
 
 #if defined(CONFIG_MONGOOSE_WEB_API_V1)
-	mg_register_http_endpoint(nc, "/handle_post_request", MG_CB(upload_handler, NULL));
+	mg_register_http_endpoint(nc, "/handle_post_request", MG_CB(upload_handler_v1, NULL));
 	mg_register_http_endpoint(nc, "/getstatus.json", MG_CB(recovery_status, NULL));
 	mg_register_http_endpoint(nc, "/rebootTarget", MG_CB(reboot_target, NULL));
 	mg_register_http_endpoint(nc, "/postUpdateCommand", MG_CB(post_update_cmd, NULL));
-#elif defined(CONFIG_MONGOOSE_WEB_API_V2)
-	mg_register_http_endpoint(nc, "/restart", restart_handler);
 #endif
+#if defined(CONFIG_MONGOOSE_WEB_API_V2)
+	mg_register_http_endpoint(nc, "/restart", MG_CB(restart_handler, NULL));
 	mg_register_http_endpoint(nc, "/upload", MG_CB(upload_handler, NULL));
+#endif
 	mg_set_protocol_http_websocket(nc);
 
 #if defined(CONFIG_MONGOOSE_WEB_API_V2)
-	mg_start_thread(broadcast_message_thread, &mgr);
 	mg_start_thread(broadcast_progress_thread, &mgr);
 #endif