| Message ID | 411e1627dce607acf11bd047d8fb24b11e35d70a.1704886405.git.reibax@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Run postupdate on all upgrade sources | expand |
Hi Xabier, Thanks for the patch. Not sure if the TRACE is worth it. Nonetheless, you've probably been following the thread: https://groups.google.com/g/swupdate/c/AywmYTfwe-s. Please don't forget the reported-by tags in V2. Thank you and kind regards, Michael Xabier Marquiegui schrieb am Mittwoch, 10. Januar 2024 um 12:38:53 UTC+1: > mongoose_interface broadcast_progress_thread used to filter postupdate > actions, and run them exclusively for SOURCE_WEBSERVER exclusively, > which worked fine in 2022.05 and previous versions. > > In the latest versions, launching a swupdate on the integrated webserver > results in msg.source containing SOURCE_UNKNOWN value, which prevents > postupgrade actions to be run. > > Signed-off-by: Xabier Marquiegui <rei...@gmail.com> > --- > v1: initial version > --- > mongoose/mongoose_interface.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c > index 4b61acb..ba82f93 100644 > --- a/mongoose/mongoose_interface.c > +++ b/mongoose/mongoose_interface.c > @@ -489,10 +489,13 @@ static void *broadcast_progress_thread(void > __attribute__ ((__unused__)) *data) > broadcast(str); > } > > - if (msg.status == SUCCESS && msg.source == SOURCE_WEBSERVER && > run_postupdate) { > - ipc_message ipc = {}; > - > - ipc_postupdate(&ipc); > + if (msg.status == SUCCESS) { > + if (run_postupdate) { > + ipc_message ipc = {}; > + ipc_postupdate(&ipc); > + } else { > + TRACE("ipc_postupdate disabled"); > + } > } > > if (msg.infolen) { > -- > 2.39.2 > >
Hi Xabier Same issue here. I can confirm that your patch fixes it. Thank you! Regards, Adrian Xabier Marquiegui schrieb am Mittwoch, 10. Januar 2024 um 12:38:53 UTC+1: > mongoose_interface broadcast_progress_thread used to filter postupdate > actions, and run them exclusively for SOURCE_WEBSERVER exclusively, > which worked fine in 2022.05 and previous versions. > > In the latest versions, launching a swupdate on the integrated webserver > results in msg.source containing SOURCE_UNKNOWN value, which prevents > postupgrade actions to be run. > > Signed-off-by: Xabier Marquiegui <rei...@gmail.com> > --- > v1: initial version > --- > mongoose/mongoose_interface.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c > index 4b61acb..ba82f93 100644 > --- a/mongoose/mongoose_interface.c > +++ b/mongoose/mongoose_interface.c > @@ -489,10 +489,13 @@ static void *broadcast_progress_thread(void > __attribute__ ((__unused__)) *data) > broadcast(str); > } > > - if (msg.status == SUCCESS && msg.source == SOURCE_WEBSERVER && > run_postupdate) { > - ipc_message ipc = {}; > - > - ipc_postupdate(&ipc); > + if (msg.status == SUCCESS) { > + if (run_postupdate) { > + ipc_message ipc = {}; > + ipc_postupdate(&ipc); > + } else { > + TRACE("ipc_postupdate disabled"); > + } > } > > if (msg.infolen) { > -- > 2.39.2 > >
Hi Everybody, On 10.01.24 08:26, Adrian Freihofer wrote: > Hi Xabier > > Same issue here. I can confirm that your patch fixes it. > Thank you! > But what about if multiple sources are activated at once ? The postupdate in the mongoose interface is called unconditionally. Best regards, Stefano > Regards, > Adrian > > Xabier Marquiegui schrieb am Mittwoch, 10. Januar 2024 um 12:38:53 UTC+1: > > mongoose_interface broadcast_progress_thread used to filter postupdate > actions, and run them exclusively for SOURCE_WEBSERVER exclusively, > which worked fine in 2022.05 and previous versions. > > In the latest versions, launching a swupdate on the integrated > webserver > results in msg.source containing SOURCE_UNKNOWN value, which prevents > postupgrade actions to be run. > > Signed-off-by: Xabier Marquiegui <rei...@gmail.com> > --- > v1: initial version > --- > mongoose/mongoose_interface.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/mongoose/mongoose_interface.c > b/mongoose/mongoose_interface.c > index 4b61acb..ba82f93 100644 > --- a/mongoose/mongoose_interface.c > +++ b/mongoose/mongoose_interface.c > @@ -489,10 +489,13 @@ static void *broadcast_progress_thread(void > __attribute__ ((__unused__)) *data) > broadcast(str); > } > > - if (msg.status == SUCCESS && msg.source == SOURCE_WEBSERVER && > run_postupdate) { > - ipc_message ipc = {}; > - > - ipc_postupdate(&ipc); > + if (msg.status == SUCCESS) { > + if (run_postupdate) { > + ipc_message ipc = {}; > + ipc_postupdate(&ipc); > + } else { > + TRACE("ipc_postupdate disabled"); > + } > } > > if (msg.infolen) { > -- > 2.39.2 > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/d65e7c50-c73d-4e4a-b0c7-5544c2147b5fn%40googlegroups.com <https://groups.google.com/d/msgid/swupdate/d65e7c50-c73d-4e4a-b0c7-5544c2147b5fn%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hi, >> Same issue here. I can confirm that your patch fixes it. >> Thank you! > > But what about if multiple sources are activated at once ? The postupdate in the mongoose interface is called unconditionally. True. As I wrote in the other thread, the others, i.e., https://github.com/sbabic/swupdate/blob/master/tools/swupdate-client.c#L110 https://github.com/sbabic/swupdate/blob/master/core/install_from_file.c#L59 https://github.com/sbabic/swupdate/blob/master/tools/swupdate-ipc.c#L644 https://github.com/sbabic/swupdate/blob/master/corelib/downloader.c#L81 are also not checking for .source == <something> as they push the update and hence know they're the initiator. Combining mongoose with this patch and one of those, both take postupdate action. If you only ingress from mongoose with this patch and have none else configured, this patch "fixes"/works around the problem. So, as you rightfully proposed in the other thread, the source/mongoose could cache that it has initiated the update and only postupdate-act on its initiated updates. Or the message could be sent with the proper .source information so that mongoose doesn't rely on .source having a value from an earlier message (this is what caused the breakage). Kind regards, Christian
Hi Christian, On 10.01.24 10:44, 'Storm, Christian' via swupdate wrote: > Hi, > >>> Same issue here. I can confirm that your patch fixes it. >>> Thank you! >> >> But what about if multiple sources are activated at once ? The postupdate in the mongoose interface is called unconditionally. > > True. As I wrote in the other thread, the others, i.e., > https://github.com/sbabic/swupdate/blob/master/tools/swupdate-client.c#L110 > https://github.com/sbabic/swupdate/blob/master/core/install_from_file.c#L59 > https://github.com/sbabic/swupdate/blob/master/tools/swupdate-ipc.c#L644 > https://github.com/sbabic/swupdate/blob/master/corelib/downloader.c#L81 > are also not checking for .source == <something> as they push the update and hence know they're the initiator. Right, they are the initiator, they do not need to check. But I can suppose about use cases where the application is reading progress messages and can decide to do something different on depend of the interface. so propagating the source seems the right way. > Combining mongoose with this patch and one of those, both take postupdate action. Right, not correct. > If you only ingress from mongoose with this patch and have none else configured, this patch "fixes"/works around the problem. IMHO the issue is generated because the source is unconditionally reset in the commit d116870e943de2a. By reverting this commit, issue is solved, too. However, I do not remember which issue this commit of yours should fix. The source is not reset after downloaded, but it is set when an update is started again. IMHO it should not reset unconditionally. Can you explain again the original issue ? I am currently tending to revert the mentioned patch. > > > So, as you rightfully proposed in the other thread, the source/mongoose could cache that it has initiated the update and only postupdate-act on its initiated updates. > Or the message could be sent with the proper .source information so that mongoose doesn't rely on .source having a value from an earlier message (this is what caused the breakage). Reverting the patch, .source contains the correct information and post-update is called. > > > > Kind regards, > Christian > Best regards, Stefano
Hi Stefano, >>>> Same issue here. I can confirm that your patch fixes it. >>>> Thank you! >>> >>> But what about if multiple sources are activated at once ? The postupdate in the mongoose interface is called unconditionally. >> True. As I wrote in the other thread, the others, i.e., >> https://github.com/sbabic/swupdate/blob/master/tools/swupdate-client.c#L110 >> https://github.com/sbabic/swupdate/blob/master/core/install_from_file.c#L59 >> https://github.com/sbabic/swupdate/blob/master/tools/swupdate-ipc.c#L644 >> https://github.com/sbabic/swupdate/blob/master/corelib/downloader.c#L81 >> are also not checking for .source == <something> as they push the update and hence know they're the initiator. > > Right, they are the initiator, they do not need to check. But I can suppose about use cases where the application is reading progress messages and can decide to do something different on depend of the interface. so propagating the source seems the right way. Yes, that would be the best option. swupdate-progress would then skip all handling of which it knows a source has methods for it so that only and exactly one entity is handling this. >> Combining mongoose with this patch and one of those, both take postupdate action. > > Right, not correct. > >> If you only ingress from mongoose with this patch and have none else configured, this patch "fixes"/works around the problem. > > IMHO the issue is generated because the source is unconditionally reset in the commit d116870e943de2a. By reverting this commit, issue is solved, too. Yes, but it would also be fixed if SWUpdate core would (re-)set .source properly for each message, i.e., eliminating state (for the .source field). Implicitly or explicitly relying on a previous message having set .source and that not being properly (re-)set for each (new) message is the main culprit here as I see it. > However, I do not remember which issue this commit of yours should fix. The source is not reset after downloaded, but it is set when an update is started again. IMHO it should not reset unconditionally. Can you explain again the original issue ? I am currently tending to revert the mentioned patch. Patch 8fccd13 "progress: Pass through source on download progress" was the original patch to pass through sourcetype on download progress notification so that progress clients can filter on this (this can be used, e.g., in suricatta/Lua to only report "relevant" information to the remote backend or rate-limiting it). That, however, didn't work under all conditions, exactly due to the state problem: If .source was (re-)set in-flight, filtering didn't apply correctly or has to be codified broader. Hence d116870 "progress: Clear source after reported download progress" which clears .source unconditionally so that this filtering does work more broadly. >> So, as you rightfully proposed in the other thread, the source/mongoose could cache that it has initiated the update and only postupdate-act on its initiated updates. >> Or the message could be sent with the proper .source information so that mongoose doesn't rely on .source having a value from an earlier message (this is what caused the breakage). > > Reverting the patch, .source contains the correct information and post-update is called. Yes, but I consider this a workaround as well since mongoose is currently relying on .source == SOURCE_WEBSERVER which is set by a *previous* download progress message. So, it relies on message order and state set by a previous message. I'd rather prefer mongoose keeping track of it being the source and acting accordingly as you suggested since that would fix this implicit reliance on order and information for mongoose. Or .source is properly set for each message by SWUpdate core, then this problem vanishes as well. Kind regards, Christian
Hi Christian, On 11.01.24 10:46, 'Storm, Christian' via swupdate wrote: > Hi Stefano, > >>>>> Same issue here. I can confirm that your patch fixes it. >>>>> Thank you! >>>> >>>> But what about if multiple sources are activated at once ? The postupdate in the mongoose interface is called unconditionally. >>> True. As I wrote in the other thread, the others, i.e., >>> https://github.com/sbabic/swupdate/blob/master/tools/swupdate-client.c#L110 >>> https://github.com/sbabic/swupdate/blob/master/core/install_from_file.c#L59 >>> https://github.com/sbabic/swupdate/blob/master/tools/swupdate-ipc.c#L644 >>> https://github.com/sbabic/swupdate/blob/master/corelib/downloader.c#L81 >>> are also not checking for .source == <something> as they push the update and hence know they're the initiator. >> >> Right, they are the initiator, they do not need to check. But I can suppose about use cases where the application is reading progress messages and can decide to do something different on depend of the interface. so propagating the source seems the right way. > > Yes, that would be the best option. swupdate-progress would then skip all handling of which it knows a source has methods for it so that only and exactly one entity is handling this. > > >>> Combining mongoose with this patch and one of those, both take postupdate action. >> >> Right, not correct. >> >>> If you only ingress from mongoose with this patch and have none else configured, this patch "fixes"/works around the problem. >> >> IMHO the issue is generated because the source is unconditionally reset in the commit d116870e943de2a. By reverting this commit, issue is solved, too. > > Yes, but it would also be fixed if SWUpdate core would (re-)set .source properly for each message, i.e., eliminating state (for the .source field). Implicitly or explicitly relying on a previous message having set .source and that not being properly (re-)set for each (new) message is the main culprit here as I see it. State is already saved outside progress interface, the swupdate_progress_init caches it again in own state. > > >> However, I do not remember which issue this commit of yours should fix. The source is not reset after downloaded, but it is set when an update is started again. IMHO it should not reset unconditionally. Can you explain again the original issue ? I am currently tending to revert the mentioned patch. > > Patch 8fccd13 "progress: Pass through source on download progress" was the original patch to pass through sourcetype on download progress notification so that progress clients can filter on this (this can be used, e.g., in suricatta/Lua to only report "relevant" information to the remote backend or rate-limiting it). That, however, didn't work under all conditions, exactly due to the state problem: If .source was (re-)set in-flight, filtering didn't apply correctly or has to be codified broader. Hence d116870 "progress: Clear source after reported download progress" which clears .source unconditionally so that this filtering does work more broadly. But both of them are quite a work-around. They set the source without knowing if an update is already running from one of the sources. So d116870e943de2a just fixes an issue generated by 8fccd13, but they do not solve the original issue. > > >>> So, as you rightfully proposed in the other thread, the source/mongoose could cache that it has initiated the update and only postupdate-act on its initiated updates. >>> Or the message could be sent with the proper .source information so that mongoose doesn't rely on .source having a value from an earlier message (this is what caused the breakage). >> >> Reverting the patch, .source contains the correct information and post-update is called. > > Yes, but I consider this a workaround as well since mongoose is currently relying on .source == SOURCE_WEBSERVER which is set by a *previous* download progress message. So, it relies on message order and state set by a previous message. I'd rather prefer mongoose keeping track of it being the source and acting accordingly as you suggested since that would fix this implicit reliance on order and information for mongoose. Or .source is properly set for each message by SWUpdate core, then this problem vanishes as well. Agree - source should be get in core and retrieved before sending a progress message. Processes calling the progress API shouldn't know which is the source because this is stored by the core. I send patches for this, I will ask you to test them to check if issue is solved (I am not currently in my office and I can't test a lot). Best regards, Stefano > > > Kind regards, > Christian >
diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c index 4b61acb..ba82f93 100644 --- a/mongoose/mongoose_interface.c +++ b/mongoose/mongoose_interface.c @@ -489,10 +489,13 @@ static void *broadcast_progress_thread(void __attribute__ ((__unused__)) *data) broadcast(str); } - if (msg.status == SUCCESS && msg.source == SOURCE_WEBSERVER && run_postupdate) { - ipc_message ipc = {}; - - ipc_postupdate(&ipc); + if (msg.status == SUCCESS) { + if (run_postupdate) { + ipc_message ipc = {}; + ipc_postupdate(&ipc); + } else { + TRACE("ipc_postupdate disabled"); + } } if (msg.infolen) {
mongoose_interface broadcast_progress_thread used to filter postupdate actions, and run them exclusively for SOURCE_WEBSERVER exclusively, which worked fine in 2022.05 and previous versions. In the latest versions, launching a swupdate on the integrated webserver results in msg.source containing SOURCE_UNKNOWN value, which prevents postupgrade actions to be run. Signed-off-by: Xabier Marquiegui <reibax@gmail.com> --- v1: initial version --- mongoose/mongoose_interface.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)