| Message ID | 20170818082704.11873-1-christian.storm@siemens.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Christian, On 18/08/2017 10:27, Christian Storm wrote: > Allow aborting suricatta_wait() via IPC by server_ipc() > returning SERVER_OK_WAKEUP. > The commit message is misleading because there is a mix of Abort and Wake-up. It is not clear if something is aborted or just waken-up. Anyway, I am missing the goal because this seems just a hack to ping the process without returning any information and without any communication with the hawkbit server. One poit is that the IPC to the subprocesses (mainly suricatta) is poorly documented, and you can maybe already obtain your information from the already implemented IPC. I start to describe it - this should then land into documentation. We have currently two IPC commands: - CMD_ACTIVATION: this is used when the confirm for a successful update is done outside SWUpdate. Use case: everything was fine and board reboots. After reboot, an application starts with its own checks - it knows if everything is fine and it has to communicate this to SWUpdate. SWUPdate ist startet with "-u -c 6" ==> WAIT State That means that SWUpdate just waits until a process tells him what he has to do. If you are using your patch as synchronization to check if SWUPdate is running, you are just quite good served. SWUpdate won't go on and it stops until you send a IPC. The CMD_ACTIVATION will then send the result of the last update, and this result is forwarded to Hawkbit, moving the state machine. SWUpdate will leave the WAIT state and go the "normal" (check for update state". - CMD_CONFIG: used to tune some parameters. Up now, it is just for polling, but it can be extended with other cases. Your use case seems just to know "is SWUpdate running ?", and it looks too less to introduce a new (undocumented) IPC command. > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > include/network_ipc.h | 3 ++- > include/suricatta/suricatta.h | 1 + > suricatta/server_hawkbit.c | 23 ++++++++++------------- > suricatta/suricatta.c | 6 +++++- > 4 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/include/network_ipc.h b/include/network_ipc.h > index e0d6672..9cd947b 100644 > --- a/include/network_ipc.h > +++ b/include/network_ipc.h > @@ -37,7 +37,8 @@ typedef enum { > > enum { > CMD_ACTIVATION, > - CMD_CONFIG > + CMD_CONFIG, > + CMD_WAKEUP > }; > > typedef union { > diff --git a/include/suricatta/suricatta.h b/include/suricatta/suricatta.h > index c04aff4..2419d8c 100644 > --- a/include/suricatta/suricatta.h > +++ b/include/suricatta/suricatta.h > @@ -44,6 +44,7 @@ typedef enum { > > typedef enum { > SERVER_OK, > + SERVER_OK_WAKEUP, > SERVER_EERR, > SERVER_EBADMSG, > SERVER_EINIT, > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > index 605733f..6550835 100644 > --- a/suricatta/server_hawkbit.c > +++ b/suricatta/server_hawkbit.c > @@ -871,6 +871,7 @@ static server_op_res_t handle_feedback(int action_id, server_op_res_t result, > > switch (result) { > case SERVER_OK: > + case SERVER_OK_WAKEUP: > case SERVER_ID_REQUESTED: > case SERVER_UPDATE_CANCELED: > case SERVER_NO_UPDATE_AVAILABLE: > @@ -1193,6 +1194,7 @@ server_op_res_t server_install_update(void) > case SERVER_UPDATE_AVAILABLE: > case SERVER_ID_REQUESTED: > case SERVER_OK: > + case SERVER_OK_WAKEUP: > break; > case SERVER_EERR: > case SERVER_EBADMSG: > @@ -1916,12 +1918,11 @@ static server_op_res_t server_configuration_ipc(ipc_message *msg) > server_op_res_t server_ipc(int fd) > { > ipc_message msg; > - server_op_res_t result = SERVER_OK; > - int ret; > + server_op_res_t result; > > - ret = read(fd, &msg, sizeof(msg)); > - if (ret != sizeof(msg)) > + if (read(fd, &msg, sizeof(msg)) != sizeof(msg)) { > return SERVER_EERR; > + } > > switch (msg.data.instmsg.cmd) { > case CMD_ACTIVATION: > @@ -1930,23 +1931,19 @@ server_op_res_t server_ipc(int fd) > case CMD_CONFIG: > result = server_configuration_ipc(&msg); > break; > + case CMD_WAKEUP: > + result = SERVER_OK_WAKEUP; > + break; > default: > result = SERVER_EERR; > break; > } > > - if (result == SERVER_EERR) { > - msg.type = NACK; > - } else > - msg.type = ACK; > - > + msg.type = (result == SERVER_EERR) ? NACK : ACK; > msg.data.instmsg.len = 0; > - > if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) { > TRACE("IPC ERROR: sending back msg"); > } > > - /* Send ipc back */ > - > - return SERVER_OK; > + return result; > } > diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c > index b520978..e1b161b 100644 > --- a/suricatta/suricatta.c > +++ b/suricatta/suricatta.c > @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) > } > if (retval && FD_ISSET(sw_sockfd, &readfds)) { > TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); > - if (server.ipc(sw_sockfd) != SERVER_OK){ > + server_op_res_t result = server.ipc(sw_sockfd); > + if (result == SERVER_OK_WAKEUP) { > + return 0; > + } I think that this breaks the concept. Suricatta (as standalone daemone) should not know what the single servers (even if we have just server_hawkbit) are doing. The logic should be in server hawkbit and not here, if necessary. Best regards, Stefano
Hi Stefano, > On 18/08/2017 10:27, Christian Storm wrote: > > Allow aborting suricatta_wait() via IPC by server_ipc() > > returning SERVER_OK_WAKEUP. > > > > The commit message is misleading because there is a mix of Abort and > Wake-up. It is not clear if something is aborted or just waken-up. > > Anyway, I am missing the goal because this seems just a hack to ping the > process without returning any information and without any communication > with the hawkbit server. One poit is that the IPC to the subprocesses > (mainly suricatta) is poorly documented, and you can maybe already > obtain your information from the already implemented IPC. > > I start to describe it - this should then land into documentation. > > We have currently two IPC commands: > > - CMD_ACTIVATION: this is used when the confirm for a successful update > is done outside SWUpdate. > > Use case: everything was fine and board reboots. After reboot, an > application starts with its own checks - it knows if everything is fine > and it has to communicate this to SWUpdate. > > SWUPdate ist startet with "-u -c 6" ==> WAIT State > > That means that SWUpdate just waits until a process tells him what he > has to do. If you are using your patch as synchronization to check if > SWUPdate is running, you are just quite good served. SWUpdate won't go > on and it stops until you send a IPC. > > The CMD_ACTIVATION will then send the result of the last update, and > this result is forwarded to Hawkbit, moving the state machine. SWUpdate > will leave the WAIT state and go the "normal" (check for update state". > > - CMD_CONFIG: used to tune some parameters. Up now, it is just for > polling, but it can be extended with other cases. > > Your use case seems just to know "is SWUpdate running ?", and it looks > too less to introduce a new (undocumented) IPC command. Ah, OK, the commit message is misleading :) The intention for this patch was to create the ability to send a wake up call to suricatta while it is sleeping in order to abort the sleep and resume work immediately. Something along the lines of waking up a daemon via UNIX signal but for suricatta and via its IPC. The motivation was that, say, suricatta sleeps and then some external trigger comes in so that suricatta should resume operation immediately. > [....] > > diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c > > index b520978..e1b161b 100644 > > --- a/suricatta/suricatta.c > > +++ b/suricatta/suricatta.c > > @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) > > } > > if (retval && FD_ISSET(sw_sockfd, &readfds)) { > > TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); > > - if (server.ipc(sw_sockfd) != SERVER_OK){ > > + server_op_res_t result = server.ipc(sw_sockfd); > > + if (result == SERVER_OK_WAKEUP) { > > + return 0; > > + } > > I think that this breaks the concept. Suricatta (as standalone daemone) > should not know what the single servers (even if we have just > server_hawkbit) are doing. The logic should be in server hawkbit and not > here, if necessary. OK, then suricatta has to become a supervisor process for the server processes, right? Currently, suricatta.c sleeps and calls out to the server implementation, i.e., for now hawkbit. This way, suricatta.c can then forward such IPC to the multiple server implementations. Kind regards, Christian
Hi Christian, On 21/08/2017 15:45, Christian Storm wrote: > Hi Stefano, > >> On 18/08/2017 10:27, Christian Storm wrote: >>> Allow aborting suricatta_wait() via IPC by server_ipc() >>> returning SERVER_OK_WAKEUP. >>> >> >> The commit message is misleading because there is a mix of Abort and >> Wake-up. It is not clear if something is aborted or just waken-up. >> >> Anyway, I am missing the goal because this seems just a hack to ping the >> process without returning any information and without any communication >> with the hawkbit server. One poit is that the IPC to the subprocesses >> (mainly suricatta) is poorly documented, and you can maybe already >> obtain your information from the already implemented IPC. >> >> I start to describe it - this should then land into documentation. >> >> We have currently two IPC commands: >> >> - CMD_ACTIVATION: this is used when the confirm for a successful update >> is done outside SWUpdate. >> >> Use case: everything was fine and board reboots. After reboot, an >> application starts with its own checks - it knows if everything is fine >> and it has to communicate this to SWUpdate. >> >> SWUPdate ist startet with "-u -c 6" ==> WAIT State >> >> That means that SWUpdate just waits until a process tells him what he >> has to do. If you are using your patch as synchronization to check if >> SWUPdate is running, you are just quite good served. SWUpdate won't go >> on and it stops until you send a IPC. >> >> The CMD_ACTIVATION will then send the result of the last update, and >> this result is forwarded to Hawkbit, moving the state machine. SWUpdate >> will leave the WAIT state and go the "normal" (check for update state". >> >> - CMD_CONFIG: used to tune some parameters. Up now, it is just for >> polling, but it can be extended with other cases. >> >> Your use case seems just to know "is SWUpdate running ?", and it looks >> too less to introduce a new (undocumented) IPC command. > > > Ah, OK, the commit message is misleading :) > The intention for this patch was to create the ability to send a wake up > call to suricatta while it is sleeping in order to abort the sleep and > resume work immediately. Something along the lines of waking up a daemon > via UNIX signal but for suricatta and via its IPC. right - instead of UNIX signals, the server is triggered via IPC. > The motivation was that, > say, suricatta sleeps and then some external trigger comes in so that > suricatta should resume operation immediately. I do this in two ways: - at the startup: swupdate started with -u "-c 6" (WAIT) and it blocks until I send a IPC message. - during "normal" activities: I send a "CONFIG" ipc (that wakes up suricatta immediately) with a low value for "polling". Value is active after server_hawkbit process the IPC. And when I do not need that suricatta processes immediately, Isend a further CONFIG increasing the polling (or the "polling" value set by hawkbit will be taken). > > >> [....] >>> diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c >>> index b520978..e1b161b 100644 >>> --- a/suricatta/suricatta.c >>> +++ b/suricatta/suricatta.c >>> @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) >>> } >>> if (retval && FD_ISSET(sw_sockfd, &readfds)) { >>> TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); >>> - if (server.ipc(sw_sockfd) != SERVER_OK){ >>> + server_op_res_t result = server.ipc(sw_sockfd); >>> + if (result == SERVER_OK_WAKEUP) { >>> + return 0; >>> + } >> >> I think that this breaks the concept. Suricatta (as standalone daemone) >> should not know what the single servers (even if we have just >> server_hawkbit) are doing. The logic should be in server hawkbit and not >> here, if necessary. > > OK, then suricatta has to become a supervisor process for the server > processes, right? Right, this is the concept. It is just the daemon / supervisor without any knowledge of the current server implementation. > Currently, suricatta.c sleeps and calls out to the > server implementation, i.e., for now hawkbit. For now just Hawkbit. In the future, we can add multiple implementations and we could also think if all these implementations are allowed to run at the same time. IMHO several implementatiosn is just a question of time, when someone will require further backend to be supported. Multiple instances at once is maybe not a use case. > This way, suricatta.c can > then forward such IPC to the multiple server implementations. Right. Best regards, Stefano
Hi Stefano, > > [...] > > Ah, OK, the commit message is misleading :) > > The intention for this patch was to create the ability to send a wake up > > call to suricatta while it is sleeping in order to abort the sleep and > > resume work immediately. Something along the lines of waking up a daemon > > via UNIX signal but for suricatta and via its IPC. > > right - instead of UNIX signals, the server is triggered via IPC. > > > The motivation was that, > > say, suricatta sleeps and then some external trigger comes in so that > > suricatta should resume operation immediately. > > I do this in two ways: > > - at the startup: swupdate started with -u "-c 6" (WAIT) and it blocks > until I send a IPC message. > > - during "normal" activities: I send a "CONFIG" ipc (that wakes up > suricatta immediately) with a low value for "polling". Value is active > after server_hawkbit process the IPC. And when I do not need that > suricatta processes immediately, Isend a further CONFIG increasing the > polling (or the "polling" value set by hawkbit will be taken). Yes, right, any IPC wakes up suricatta, so does "CONFIG". I think (ab)using "CONFIG" for this purpose is a bit to implicit for me and/or demands for proper documentation how to use the "CONFIG" IPC feature for waking up suricatta. I like to have it explicit and hence came up with this proposal. But I'm also fine with documenting the "wakeup via CONFIG" feature and dropping this patch. > >> [....] > >>> diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c > >>> index b520978..e1b161b 100644 > >>> --- a/suricatta/suricatta.c > >>> +++ b/suricatta/suricatta.c > >>> @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) > >>> } > >>> if (retval && FD_ISSET(sw_sockfd, &readfds)) { > >>> TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); > >>> - if (server.ipc(sw_sockfd) != SERVER_OK){ > >>> + server_op_res_t result = server.ipc(sw_sockfd); > >>> + if (result == SERVER_OK_WAKEUP) { > >>> + return 0; > >>> + } > >> > >> I think that this breaks the concept. Suricatta (as standalone daemone) > >> should not know what the single servers (even if we have just > >> server_hawkbit) are doing. The logic should be in server hawkbit and not > >> here, if necessary. > > > > OK, then suricatta has to become a supervisor process for the server > > processes, right? > > Right, this is the concept. It is just the daemon / supervisor without > any knowledge of the current server implementation. > > > Currently, suricatta.c sleeps and calls out to the > > server implementation, i.e., for now hawkbit. > > For now just Hawkbit. In the future, we can add multiple implementations > and we could also think if all these implementations are allowed to run > at the same time. IMHO several implementatiosn is just a question of > time, when someone will require further backend to be supported. > Multiple instances at once is maybe not a use case. Hm, I can imagine installing two "types" of artifacts via SWUpdate, one being firmware and one being, e.g., applications in a broader sense, given that SWUpdate is used to update distributions as I recently read on this list :) And those may come from two different backends, meaning either two instances of a suricatta server implementation running or two separate SWUpdate processes running, one for each backend. > > This way, suricatta.c can > > then forward such IPC to the multiple server implementations. > > Right. OK, so then we go forward with dropping this and implementing the suricatta.c supervisor feature? This would be fine with me... Kind regards, Christian
Hi Christian, On 22/08/2017 13:51, Christian Storm wrote: > Hi Stefano, > >>> [...] >>> Ah, OK, the commit message is misleading :) >>> The intention for this patch was to create the ability to send a wake up >>> call to suricatta while it is sleeping in order to abort the sleep and >>> resume work immediately. Something along the lines of waking up a daemon >>> via UNIX signal but for suricatta and via its IPC. >> >> right - instead of UNIX signals, the server is triggered via IPC. >> >>> The motivation was that, >>> say, suricatta sleeps and then some external trigger comes in so that >>> suricatta should resume operation immediately. >> >> I do this in two ways: >> >> - at the startup: swupdate started with -u "-c 6" (WAIT) and it blocks >> until I send a IPC message. >> >> - during "normal" activities: I send a "CONFIG" ipc (that wakes up >> suricatta immediately) with a low value for "polling". Value is active >> after server_hawkbit process the IPC. And when I do not need that >> suricatta processes immediately, Isend a further CONFIG increasing the >> polling (or the "polling" value set by hawkbit will be taken). > > Yes, right, any IPC wakes up suricatta, so does "CONFIG". I think > (ab)using "CONFIG" for this purpose is a bit to implicit for me and/or > demands for proper documentation how to use the "CONFIG" IPC feature for > waking up suricatta. I like to have it explicit and hence came up > with this proposal. The weird thing for me is that the "WAKE" is not really a IPC command, but the side-effect is to wake-up the process. I think it is nicer to have such as "GET_CONFIG" or whatever doing a real IPC, while your proposal asks the responder in server_hawkbit to simply come back, because the goal was already reached. > But I'm also fine with documenting the "wakeup via > CONFIG" feature and dropping this patch. > > >>>> [....] >>>>> diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c >>>>> index b520978..e1b161b 100644 >>>>> --- a/suricatta/suricatta.c >>>>> +++ b/suricatta/suricatta.c >>>>> @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) >>>>> } >>>>> if (retval && FD_ISSET(sw_sockfd, &readfds)) { >>>>> TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); >>>>> - if (server.ipc(sw_sockfd) != SERVER_OK){ >>>>> + server_op_res_t result = server.ipc(sw_sockfd); >>>>> + if (result == SERVER_OK_WAKEUP) { >>>>> + return 0; >>>>> + } >>>> >>>> I think that this breaks the concept. Suricatta (as standalone daemone) >>>> should not know what the single servers (even if we have just >>>> server_hawkbit) are doing. The logic should be in server hawkbit and not >>>> here, if necessary. >>> >>> OK, then suricatta has to become a supervisor process for the server >>> processes, right? >> >> Right, this is the concept. It is just the daemon / supervisor without >> any knowledge of the current server implementation. >> >>> Currently, suricatta.c sleeps and calls out to the >>> server implementation, i.e., for now hawkbit. >> >> For now just Hawkbit. In the future, we can add multiple implementations >> and we could also think if all these implementations are allowed to run >> at the same time. IMHO several implementatiosn is just a question of >> time, when someone will require further backend to be supported. >> Multiple instances at once is maybe not a use case. > > Hm, I can imagine installing two "types" of artifacts via SWUpdate, one > being firmware and one being, e.g., applications in a broader sense, > given that SWUpdate is used to update distributions as I recently read > on this list :) > And those may come from two different backends, meaning either two > instances of a suricatta server implementation running or two separate > SWUpdate processes running, one for each backend. > >>> This way, suricatta.c can >>> then forward such IPC to the multiple server implementations. >> >> Right. > > OK, so then we go forward with dropping this and implementing the > suricatta.c supervisor feature? This would be fine with me... I think it is fine if I have correctly interpreted hwat you mean with supervisor feature. IMHO suricatta is just the proxy-interface that let to communicate with the implementation (hawkbit, whatever,...) of the backend. Is this what you mind ? Best regards, Stefano
Hi Stefano, > >>> [...] > >>> Ah, OK, the commit message is misleading :) > >>> The intention for this patch was to create the ability to send a wake up > >>> call to suricatta while it is sleeping in order to abort the sleep and > >>> resume work immediately. Something along the lines of waking up a daemon > >>> via UNIX signal but for suricatta and via its IPC. > >> > >> right - instead of UNIX signals, the server is triggered via IPC. > >> > >>> The motivation was that, > >>> say, suricatta sleeps and then some external trigger comes in so that > >>> suricatta should resume operation immediately. > >> > >> I do this in two ways: > >> > >> - at the startup: swupdate started with -u "-c 6" (WAIT) and it blocks > >> until I send a IPC message. > >> > >> - during "normal" activities: I send a "CONFIG" ipc (that wakes up > >> suricatta immediately) with a low value for "polling". Value is active > >> after server_hawkbit process the IPC. And when I do not need that > >> suricatta processes immediately, Isend a further CONFIG increasing the > >> polling (or the "polling" value set by hawkbit will be taken). > > > > Yes, right, any IPC wakes up suricatta, so does "CONFIG". I think > > (ab)using "CONFIG" for this purpose is a bit to implicit for me and/or > > demands for proper documentation how to use the "CONFIG" IPC feature for > > waking up suricatta. I like to have it explicit and hence came up > > with this proposal. > > The weird thing for me is that the "WAKE" is not really a IPC command, > but the side-effect is to wake-up the process. Yes, essentially this command is a NOP as the side effect as you called it does the intended operation, namely waking up. But this is purely due to the implementation and not "visible" to the user. > I think it is nicer to have such as "GET_CONFIG" or whatever doing a > real IPC, while your proposal asks the responder in server_hawkbit to > simply come back, because the goal was already reached. OK, but then we should document that whenever someone wants to wake up suricatta "IPC:GET_CONFIG > /dev/null" can be used for this purpose. OK, then let's drop this and implement it when suricatta has become a supervisor of it's backend implementations (see below)? > > But I'm also fine with documenting the "wakeup via > > CONFIG" feature and dropping this patch. > > > > > >>>> [....] > >>>>> diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c > >>>>> index b520978..e1b161b 100644 > >>>>> --- a/suricatta/suricatta.c > >>>>> +++ b/suricatta/suricatta.c > >>>>> @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) > >>>>> } > >>>>> if (retval && FD_ISSET(sw_sockfd, &readfds)) { > >>>>> TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); > >>>>> - if (server.ipc(sw_sockfd) != SERVER_OK){ > >>>>> + server_op_res_t result = server.ipc(sw_sockfd); > >>>>> + if (result == SERVER_OK_WAKEUP) { > >>>>> + return 0; > >>>>> + } > >>>> > >>>> I think that this breaks the concept. Suricatta (as standalone daemone) > >>>> should not know what the single servers (even if we have just > >>>> server_hawkbit) are doing. The logic should be in server hawkbit and not > >>>> here, if necessary. > >>> > >>> OK, then suricatta has to become a supervisor process for the server > >>> processes, right? > >> > >> Right, this is the concept. It is just the daemon / supervisor without > >> any knowledge of the current server implementation. > >> > >>> Currently, suricatta.c sleeps and calls out to the > >>> server implementation, i.e., for now hawkbit. > >> > >> For now just Hawkbit. In the future, we can add multiple implementations > >> and we could also think if all these implementations are allowed to run > >> at the same time. IMHO several implementatiosn is just a question of > >> time, when someone will require further backend to be supported. > >> Multiple instances at once is maybe not a use case. > > > > Hm, I can imagine installing two "types" of artifacts via SWUpdate, one > > being firmware and one being, e.g., applications in a broader sense, > > given that SWUpdate is used to update distributions as I recently read > > on this list :) > > And those may come from two different backends, meaning either two > > instances of a suricatta server implementation running or two separate > > SWUpdate processes running, one for each backend. > > > >>> This way, suricatta.c can > >>> then forward such IPC to the multiple server implementations. > >> > >> Right. > > > > OK, so then we go forward with dropping this and implementing the > > suricatta.c supervisor feature? This would be fine with me... > > I think it is fine if I have correctly interpreted hwat you mean with > supervisor feature. IMHO suricatta is just the proxy-interface that let > to communicate with the implementation (hawkbit, whatever,...) of the > backend. Is this what you mind ? Yes, just like swupdate itself spawns processes, suricatta then spawns processes and supervises them. Suricatta should supervise those backend implementation processes (hawkbit,...) and relay IPC to/from them. But it is agnostic of the concrete implementations, i.e. rely on IPC only. Whether suricatta spawns those processes itself or whether it's done via swupdate/core means is to be discussed/designed. I guess this is what is meant by the roadmap.rst section "Support for multiple Servers simultaneously", right? Kind regards, Christian
Hi Christian, On 24/08/2017 09:07, Christian Storm wrote: > Hi Stefano, > >>>>> [...] >>>>> Ah, OK, the commit message is misleading :) >>>>> The intention for this patch was to create the ability to send a wake up >>>>> call to suricatta while it is sleeping in order to abort the sleep and >>>>> resume work immediately. Something along the lines of waking up a daemon >>>>> via UNIX signal but for suricatta and via its IPC. >>>> >>>> right - instead of UNIX signals, the server is triggered via IPC. >>>> >>>>> The motivation was that, >>>>> say, suricatta sleeps and then some external trigger comes in so that >>>>> suricatta should resume operation immediately. >>>> >>>> I do this in two ways: >>>> >>>> - at the startup: swupdate started with -u "-c 6" (WAIT) and it blocks >>>> until I send a IPC message. >>>> >>>> - during "normal" activities: I send a "CONFIG" ipc (that wakes up >>>> suricatta immediately) with a low value for "polling". Value is active >>>> after server_hawkbit process the IPC. And when I do not need that >>>> suricatta processes immediately, Isend a further CONFIG increasing the >>>> polling (or the "polling" value set by hawkbit will be taken). >>> >>> Yes, right, any IPC wakes up suricatta, so does "CONFIG". I think >>> (ab)using "CONFIG" for this purpose is a bit to implicit for me and/or >>> demands for proper documentation how to use the "CONFIG" IPC feature for >>> waking up suricatta. I like to have it explicit and hence came up >>> with this proposal. >> >> The weird thing for me is that the "WAKE" is not really a IPC command, >> but the side-effect is to wake-up the process. > > Yes, essentially this command is a NOP as the side effect as you called > it does the intended operation, namely waking up. But this is purely due > to the implementation and not "visible" to the user. > >> I think it is nicer to have such as "GET_CONFIG" or whatever doing a >> real IPC, while your proposal asks the responder in server_hawkbit to >> simply come back, because the goal was already reached. > > OK, but then we should document that whenever someone wants to wake up > suricatta "IPC:GET_CONFIG > /dev/null" can be used for this purpose. Right. > > OK, then let's drop this and implement it when suricatta has become a > supervisor of it's backend implementations (see below)? +1 > > >>> But I'm also fine with documenting the "wakeup via >>> CONFIG" feature and dropping this patch. >>> >>> >>>>>> [....] >>>>>>> diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c >>>>>>> index b520978..e1b161b 100644 >>>>>>> --- a/suricatta/suricatta.c >>>>>>> +++ b/suricatta/suricatta.c >>>>>>> @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) >>>>>>> } >>>>>>> if (retval && FD_ISSET(sw_sockfd, &readfds)) { >>>>>>> TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); >>>>>>> - if (server.ipc(sw_sockfd) != SERVER_OK){ >>>>>>> + server_op_res_t result = server.ipc(sw_sockfd); >>>>>>> + if (result == SERVER_OK_WAKEUP) { >>>>>>> + return 0; >>>>>>> + } >>>>>> >>>>>> I think that this breaks the concept. Suricatta (as standalone daemone) >>>>>> should not know what the single servers (even if we have just >>>>>> server_hawkbit) are doing. The logic should be in server hawkbit and not >>>>>> here, if necessary. >>>>> >>>>> OK, then suricatta has to become a supervisor process for the server >>>>> processes, right? >>>> >>>> Right, this is the concept. It is just the daemon / supervisor without >>>> any knowledge of the current server implementation. >>>> >>>>> Currently, suricatta.c sleeps and calls out to the >>>>> server implementation, i.e., for now hawkbit. >>>> >>>> For now just Hawkbit. In the future, we can add multiple implementations >>>> and we could also think if all these implementations are allowed to run >>>> at the same time. IMHO several implementatiosn is just a question of >>>> time, when someone will require further backend to be supported. >>>> Multiple instances at once is maybe not a use case. >>> >>> Hm, I can imagine installing two "types" of artifacts via SWUpdate, one >>> being firmware and one being, e.g., applications in a broader sense, >>> given that SWUpdate is used to update distributions as I recently read >>> on this list :) >>> And those may come from two different backends, meaning either two >>> instances of a suricatta server implementation running or two separate >>> SWUpdate processes running, one for each backend. >>> >>>>> This way, suricatta.c can >>>>> then forward such IPC to the multiple server implementations. >>>> >>>> Right. >>> >>> OK, so then we go forward with dropping this and implementing the >>> suricatta.c supervisor feature? This would be fine with me... >> >> I think it is fine if I have correctly interpreted hwat you mean with >> supervisor feature. IMHO suricatta is just the proxy-interface that let >> to communicate with the implementation (hawkbit, whatever,...) of the >> backend. Is this what you mind ? > > Yes, just like swupdate itself spawns processes, suricatta then spawns > processes and supervises them. Suricatta should supervise those backend > implementation processes (hawkbit,...) and relay IPC to/from them. Right - if we really need to spawn processes or suricatta simply scans all backends in the list, can be decided by design. > But > it is agnostic of the concrete implementations, i.e. rely on IPC only. Right - this is like the "main" and it must not need the implementation. Backends register itself with suricatta and suricatta handles them in the same way. > Whether suricatta spawns those processes itself or whether it's done via > swupdate/core means is to be discussed/designed. Right. > I guess this is what is meant by the roadmap.rst section "Support for > multiple Servers simultaneously", right? Exactly. Best regards, Stefano
diff --git a/include/network_ipc.h b/include/network_ipc.h index e0d6672..9cd947b 100644 --- a/include/network_ipc.h +++ b/include/network_ipc.h @@ -37,7 +37,8 @@ typedef enum { enum { CMD_ACTIVATION, - CMD_CONFIG + CMD_CONFIG, + CMD_WAKEUP }; typedef union { diff --git a/include/suricatta/suricatta.h b/include/suricatta/suricatta.h index c04aff4..2419d8c 100644 --- a/include/suricatta/suricatta.h +++ b/include/suricatta/suricatta.h @@ -44,6 +44,7 @@ typedef enum { typedef enum { SERVER_OK, + SERVER_OK_WAKEUP, SERVER_EERR, SERVER_EBADMSG, SERVER_EINIT, diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c index 605733f..6550835 100644 --- a/suricatta/server_hawkbit.c +++ b/suricatta/server_hawkbit.c @@ -871,6 +871,7 @@ static server_op_res_t handle_feedback(int action_id, server_op_res_t result, switch (result) { case SERVER_OK: + case SERVER_OK_WAKEUP: case SERVER_ID_REQUESTED: case SERVER_UPDATE_CANCELED: case SERVER_NO_UPDATE_AVAILABLE: @@ -1193,6 +1194,7 @@ server_op_res_t server_install_update(void) case SERVER_UPDATE_AVAILABLE: case SERVER_ID_REQUESTED: case SERVER_OK: + case SERVER_OK_WAKEUP: break; case SERVER_EERR: case SERVER_EBADMSG: @@ -1916,12 +1918,11 @@ static server_op_res_t server_configuration_ipc(ipc_message *msg) server_op_res_t server_ipc(int fd) { ipc_message msg; - server_op_res_t result = SERVER_OK; - int ret; + server_op_res_t result; - ret = read(fd, &msg, sizeof(msg)); - if (ret != sizeof(msg)) + if (read(fd, &msg, sizeof(msg)) != sizeof(msg)) { return SERVER_EERR; + } switch (msg.data.instmsg.cmd) { case CMD_ACTIVATION: @@ -1930,23 +1931,19 @@ server_op_res_t server_ipc(int fd) case CMD_CONFIG: result = server_configuration_ipc(&msg); break; + case CMD_WAKEUP: + result = SERVER_OK_WAKEUP; + break; default: result = SERVER_EERR; break; } - if (result == SERVER_EERR) { - msg.type = NACK; - } else - msg.type = ACK; - + msg.type = (result == SERVER_EERR) ? NACK : ACK; msg.data.instmsg.len = 0; - if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) { TRACE("IPC ERROR: sending back msg"); } - /* Send ipc back */ - - return SERVER_OK; + return result; } diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c index b520978..e1b161b 100644 --- a/suricatta/suricatta.c +++ b/suricatta/suricatta.c @@ -47,7 +47,11 @@ int suricatta_wait(int seconds) } if (retval && FD_ISSET(sw_sockfd, &readfds)) { TRACE("Suricatta woke up for IPC at %ld seconds", tv.tv_sec); - if (server.ipc(sw_sockfd) != SERVER_OK){ + server_op_res_t result = server.ipc(sw_sockfd); + if (result == SERVER_OK_WAKEUP) { + return 0; + } + if (result != SERVER_OK){ DEBUG("Handling IPC failed!"); } return (int)tv.tv_sec;
Allow aborting suricatta_wait() via IPC by server_ipc() returning SERVER_OK_WAKEUP. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- include/network_ipc.h | 3 ++- include/suricatta/suricatta.h | 1 + suricatta/server_hawkbit.c | 23 ++++++++++------------- suricatta/suricatta.c | 6 +++++- 4 files changed, 18 insertions(+), 15 deletions(-)