| Message ID | 20170906072324.15734-1-christian.storm@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/6] socket split: make socket paths configurable | expand |
Hi Christian, On 06/09/2017 09:23, Christian Storm wrote: > Make the solely internally used corelib/network_thread.c use > the kconfig setting for SOCKET_CTRL_PATH directly, which > defaults to /tmp/sockinstctrl in order to maintain backwards > compatibility. > > As include/network_ipc.h / ipc/network_ipc.c is used > internally as well as externally, make them use the kconfig > setting for SOCKET_CTRL_PATH when used internally and the > currently established socket path /tmp/sockinstctrl per > default when used externally to maintain backwards > compatibility. > When deviating from the default socket path, external > clients should be adapted accordingly or, preferable, be > made configurable via a command line parameter pointing to > the socket. This is not true for the ctrl IPC. We agree that progress client should be adaptewd with command line parameters. But client sending images to be installed, that is the ones pushing to sockinstctr, should use the client interface. That means, ipc_* functions and swupdate_image_write / swupdate_async_start. They should never use the socket, and we can enforce this if we hide the path of the socket. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > corelib/network_thread.c | 3 ++- > include/network_ipc.h | 2 +- > ipc/network_ipc.c | 6 ++++++ > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/corelib/network_thread.c b/corelib/network_thread.c > index e925360..6c34614 100644 > --- a/corelib/network_thread.c > +++ b/corelib/network_thread.c > @@ -42,6 +42,7 @@ > #include "installer.h" > #include "swupdate.h" > #include "pctl.h" > +#include "generated/autoconf.h" > > #define LISTENQ 1024 > > @@ -193,7 +194,7 @@ void *network_thread (void *data) > register_notifier(network_notifier); > > /* Initialize and bind to UDS */ > - ctrllisten = listener_create(SOCKET_CTRL_PATH, SOCK_STREAM); > + ctrllisten = listener_create((char*)CONFIG_SOCKET_CTRL_PATH, SOCK_STREAM); > if (ctrllisten < 0 ) { > TRACE("Error creating IPC sockets"); > exit(2); > diff --git a/include/network_ipc.h b/include/network_ipc.h > index e0d6672..a969f4c 100644 > --- a/include/network_ipc.h > +++ b/include/network_ipc.h > @@ -24,7 +24,7 @@ > #include "swupdate_status.h" > > #define IPC_MAGIC 0x14052001 > -#define SOCKET_CTRL_PATH "/tmp/sockinstctrl" > +extern char* SOCKET_CTRL_PATH; > > typedef enum { > REQ_INSTALL, > diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c > index 6629461..13554f5 100644 > --- a/ipc/network_ipc.c > +++ b/ipc/network_ipc.c > @@ -35,6 +35,12 @@ > > #include "network_ipc.h" > > +#ifdef CONFIG_SOCKET_CTRL_PATH > +char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; > +#else > +char* SOCKET_CTRL_PATH = (char*)"/tmp/sockinstctrl"; There are two functions using the socket: network_thread (you change it) and prepare_ipc(). But prepare_ipc() is not exported and client must call ipc_inst_start*. We can drop SOCKET_CTRL_PATH from the header and fix prepare_ipc() as you do for network_thread. Best regards, Stefano
Hi Stefano, > On 06/09/2017 09:23, Christian Storm wrote: > > Make the solely internally used corelib/network_thread.c use > > the kconfig setting for SOCKET_CTRL_PATH directly, which > > defaults to /tmp/sockinstctrl in order to maintain backwards > > compatibility. > > > > As include/network_ipc.h / ipc/network_ipc.c is used > > internally as well as externally, make them use the kconfig > > setting for SOCKET_CTRL_PATH when used internally and the > > currently established socket path /tmp/sockinstctrl per > > default when used externally to maintain backwards > > compatibility. > > When deviating from the default socket path, external > > clients should be adapted accordingly or, preferable, be > > made configurable via a command line parameter pointing to > > the socket. > > This is not true for the ctrl IPC. We agree that progress client should > be adaptewd with command line parameters. But client sending images to > be installed, that is the ones pushing to sockinstctr, should use the > client interface. That means, ipc_* functions and swupdate_image_write / > swupdate_async_start. They should never use the socket, Agreed, they shouldn't use it but nevertheless maybe need to set the socket path -- e.g. if the configuration is not the default one. Please see the rationale at then end. > and we can enforce this if we hide the path of the socket. Hm, I think you cannot really enforce one not to use the socket directly by simply hiding its location as this information is either well-known (/tmp/sockinstctrl), in the .config, or a $ strings swupdate away :) In essence, one should be urged to use the methods provided for sanity's sake but there's nothing that hinders you from talking to the socket directly... > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > --- > > corelib/network_thread.c | 3 ++- > > include/network_ipc.h | 2 +- > > ipc/network_ipc.c | 6 ++++++ > > 3 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/corelib/network_thread.c b/corelib/network_thread.c > > index e925360..6c34614 100644 > > --- a/corelib/network_thread.c > > +++ b/corelib/network_thread.c > > @@ -42,6 +42,7 @@ > > #include "installer.h" > > #include "swupdate.h" > > #include "pctl.h" > > +#include "generated/autoconf.h" > > > > #define LISTENQ 1024 > > > > @@ -193,7 +194,7 @@ void *network_thread (void *data) > > register_notifier(network_notifier); > > > > /* Initialize and bind to UDS */ > > - ctrllisten = listener_create(SOCKET_CTRL_PATH, SOCK_STREAM); > > + ctrllisten = listener_create((char*)CONFIG_SOCKET_CTRL_PATH, SOCK_STREAM); > > if (ctrllisten < 0 ) { > > TRACE("Error creating IPC sockets"); > > exit(2); > > diff --git a/include/network_ipc.h b/include/network_ipc.h > > index e0d6672..a969f4c 100644 > > --- a/include/network_ipc.h > > +++ b/include/network_ipc.h > > @@ -24,7 +24,7 @@ > > #include "swupdate_status.h" > > > > #define IPC_MAGIC 0x14052001 > > -#define SOCKET_CTRL_PATH "/tmp/sockinstctrl" > > +extern char* SOCKET_CTRL_PATH; > > > > typedef enum { > > REQ_INSTALL, > > diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c > > index 6629461..13554f5 100644 > > --- a/ipc/network_ipc.c > > +++ b/ipc/network_ipc.c > > @@ -35,6 +35,12 @@ > > > > #include "network_ipc.h" > > > > +#ifdef CONFIG_SOCKET_CTRL_PATH > > +char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; > > +#else > > +char* SOCKET_CTRL_PATH = (char*)"/tmp/sockinstctrl"; > > There are two functions using the socket: network_thread (you change it) > and prepare_ipc(). But prepare_ipc() is not exported and client must > call ipc_inst_start*. We can drop SOCKET_CTRL_PATH from the header and > fix prepare_ipc() as you do for network_thread. That means, e.g., dropping `extern char* SOCKET_CTRL_PATH;` in include/network_ipc.h and making it `static char* SOCKET_CTRL_PATH = ...` in ipc/network_ipc. Or moving the `SOCKET_CTRL_PATH` directly into `prepare_ipc()`, either way serves the illustration. As a result, `SOCKET_CTRL_PATH` is hidden from the interface consumers (as it's now private within the .o) and they're left with calling `ipc_inst_*`, which they should have done in the first place, right. Consider having done this, ipc/network_ipc.c sets `SOCKET_CTRL_PATH` to `CONFIG_SOCKET_CTRL_PATH` if compiled within SWUpdate (include/generated/autoconf.h available) or to `/tmp/sockinstctrl` otherwise. As a result, the client rightfully calling the `ipc_inst_*` methods always has to be linked with the .o files reflecting the correct socket path location. So, if you configure SWUpdate to use, say, CONFIG_SOCKET_CTRL_PATH=/tmp/XXXControl, then calling the `ipc_inst_*` methods only works if the client is linked against the .o files having this location configured, i.e., you cannot extract network_ipc.{c,h} etc. from the SWUpdate sources and compile clients outside of SWUpdate without having the means to configure the socket location. TL;DR: Just because you have the means to "see" the socket doesn't mean that it's encouraged to talk to it directly :) That said, I'm fine with either way. What do you think how we should proceed on this? Kind regards, Christian
Hi Christian, On 06/09/2017 16:59, Christian Storm wrote: > Hi Stefano, > >> On 06/09/2017 09:23, Christian Storm wrote: >>> Make the solely internally used corelib/network_thread.c use >>> the kconfig setting for SOCKET_CTRL_PATH directly, which >>> defaults to /tmp/sockinstctrl in order to maintain backwards >>> compatibility. >>> >>> As include/network_ipc.h / ipc/network_ipc.c is used >>> internally as well as externally, make them use the kconfig >>> setting for SOCKET_CTRL_PATH when used internally and the >>> currently established socket path /tmp/sockinstctrl per >>> default when used externally to maintain backwards >>> compatibility. >>> When deviating from the default socket path, external >>> clients should be adapted accordingly or, preferable, be >>> made configurable via a command line parameter pointing to >>> the socket. >> >> This is not true for the ctrl IPC. We agree that progress client should >> be adaptewd with command line parameters. But client sending images to >> be installed, that is the ones pushing to sockinstctr, should use the >> client interface. That means, ipc_* functions and swupdate_image_write / >> swupdate_async_start. They should never use the socket, > > Agreed, they shouldn't use it but nevertheless maybe need to set the > socket path -- e.g. if the configuration is not the default one. > Please see the rationale at then end. > >> and we can enforce this if we hide the path of the socket. > > Hm, I think you cannot really enforce one not to use the socket directly > by simply hiding its location as this information is either well-known > (/tmp/sockinstctrl), Let me explain better: we do not guarantee that still works if they do not use. The path for the socket is not exported, this does not mean that someone tries to do on his own. > in the .config, or a $ strings swupdate away :) > In essence, one should be urged to use the methods provided for > sanity's sake but there's nothing that hinders you from talking to the > socket directly... Of course > > >>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>> --- >>> corelib/network_thread.c | 3 ++- >>> include/network_ipc.h | 2 +- >>> ipc/network_ipc.c | 6 ++++++ >>> 3 files changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/corelib/network_thread.c b/corelib/network_thread.c >>> index e925360..6c34614 100644 >>> --- a/corelib/network_thread.c >>> +++ b/corelib/network_thread.c >>> @@ -42,6 +42,7 @@ >>> #include "installer.h" >>> #include "swupdate.h" >>> #include "pctl.h" >>> +#include "generated/autoconf.h" >>> >>> #define LISTENQ 1024 >>> >>> @@ -193,7 +194,7 @@ void *network_thread (void *data) >>> register_notifier(network_notifier); >>> >>> /* Initialize and bind to UDS */ >>> - ctrllisten = listener_create(SOCKET_CTRL_PATH, SOCK_STREAM); >>> + ctrllisten = listener_create((char*)CONFIG_SOCKET_CTRL_PATH, SOCK_STREAM); >>> if (ctrllisten < 0 ) { >>> TRACE("Error creating IPC sockets"); >>> exit(2); >>> diff --git a/include/network_ipc.h b/include/network_ipc.h >>> index e0d6672..a969f4c 100644 >>> --- a/include/network_ipc.h >>> +++ b/include/network_ipc.h >>> @@ -24,7 +24,7 @@ >>> #include "swupdate_status.h" >>> >>> #define IPC_MAGIC 0x14052001 >>> -#define SOCKET_CTRL_PATH "/tmp/sockinstctrl" >>> +extern char* SOCKET_CTRL_PATH; >>> >>> typedef enum { >>> REQ_INSTALL, >>> diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c >>> index 6629461..13554f5 100644 >>> --- a/ipc/network_ipc.c >>> +++ b/ipc/network_ipc.c >>> @@ -35,6 +35,12 @@ >>> >>> #include "network_ipc.h" >>> >>> +#ifdef CONFIG_SOCKET_CTRL_PATH >>> +char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; >>> +#else >>> +char* SOCKET_CTRL_PATH = (char*)"/tmp/sockinstctrl"; >> >> There are two functions using the socket: network_thread (you change it) >> and prepare_ipc(). But prepare_ipc() is not exported and client must >> call ipc_inst_start*. We can drop SOCKET_CTRL_PATH from the header and >> fix prepare_ipc() as you do for network_thread. > > That means, e.g., dropping `extern char* SOCKET_CTRL_PATH;` in > include/network_ipc.h and making it `static char* SOCKET_CTRL_PATH = ...` > in ipc/network_ipc. Or moving the `SOCKET_CTRL_PATH` directly into > `prepare_ipc()`, either way serves the illustration. Right. > As a result, `SOCKET_CTRL_PATH` is hidden from the interface consumers > (as it's now private within the .o) and they're left with calling > `ipc_inst_*`, which they should have done in the first place, right. Right. > > Consider having done this, ipc/network_ipc.c sets `SOCKET_CTRL_PATH` to > `CONFIG_SOCKET_CTRL_PATH` if compiled within SWUpdate > (include/generated/autoconf.h available) or to `/tmp/sockinstctrl` > otherwise. I guess that in future the ipc library should be exported in sysroots (-dev package). Like a "libswupdate.a". Clients should link with it. > As a result, the client rightfully calling the `ipc_inst_*` > methods always has to be linked with the .o files reflecting the correct > socket path location. If someone does on his own, it should take care of it, right. In Yocto we can ensure that everything is consistent. > So, if you configure SWUpdate to use, say, > CONFIG_SOCKET_CTRL_PATH=/tmp/XXXControl, then calling the `ipc_inst_*` > methods only works if the client is linked against the .o files having > this location configured, i.e., you cannot extract network_ipc.{c,h} > etc. from the SWUpdate sources and compile clients outside of SWUpdate > without having the means to configure the socket location. Right - but again, it is not good practise to get sources from a project and to compile / link with the own application. The originatin project (here SWUpdate) should export headers / libraries that other part of software can simply use. > > TL;DR: Just because you have the means to "see" the socket doesn't mean > that it's encouraged to talk to it directly :) Exactly. > > > That said, I'm fine with either way. What do you think how we should > proceed on this? I should drop completely SOCKET_CTRL_PATH (useless), replacing it with CONFIG_SOCKET_CTRL_PATH. Best regards, Stefano
Hi Stefano, > [...] > > I guess that in future the ipc library should be exported in sysroots > (-dev package). Like a "libswupdate.a". Clients should link with it. > > > As a result, the client rightfully calling the `ipc_inst_*` > > methods always has to be linked with the .o files reflecting the correct > > socket path location. > > If someone does on his own, it should take care of it, right. In Yocto > we can ensure that everything is consistent. > > > So, if you configure SWUpdate to use, say, > > CONFIG_SOCKET_CTRL_PATH=/tmp/XXXControl, then calling the `ipc_inst_*` > > methods only works if the client is linked against the .o files having > > this location configured, i.e., you cannot extract network_ipc.{c,h} > > etc. from the SWUpdate sources and compile clients outside of SWUpdate > > without having the means to configure the socket location. > > Right - but again, it is not good practise to get sources from a project > and to compile / link with the own application. The originatin project > (here SWUpdate) should export headers / libraries that other part of > software can simply use. > > [...] > > > > > That said, I'm fine with either way. What do you think how we should > > proceed on this? > > I should drop completely SOCKET_CTRL_PATH (useless), replacing it with > CONFIG_SOCKET_CTRL_PATH. Thanks, agreed. I'll send a v2 of the patch series.. Kind regards, Christian
diff --git a/Kconfig b/Kconfig index 07c394a..b77850f 100644 --- a/Kconfig +++ b/Kconfig @@ -110,6 +110,29 @@ config SW_VERSIONS_FILE but in some cases it can be required to do it. Having a check, the risky-component is not always updated. +menu "Socket Paths" + +config SOCKET_CTRL_PATH + string "SWUpdate control socket path" + default "/tmp/sockinstctrl" + help + Path to SWUpdate's IPC socket. + +config SOCKET_PROGRESS_PATH + string "SWUpdate progress socket path" + default "/tmp/swupdateprog" + help + Path to the socket progress information is sent to. + +config SOCKET_REMOTE_HANDLER_DIRECTORY + string "SWUpdate remote handler socket directory" + default "/tmp/" + help + Directory where sockets to remote handler processes + are expected to be found. + +endmenu + config MTD bool "MTD support" default y
Make socket paths configurable via config system. Per default, the currently established paths are used to maintain backwards compatibility. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- Kconfig | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)