| Message ID | 20201120155525.hbbgsdh5k23zr5dm@MD1ZFJVC.ad001.siemens.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dry-run mode is broken | expand |
Hi Christian, On 20.11.20 16:55, Christian Storm wrote: > Hi Stefano, > > on current HEAD, the SWUpdate --dry-run option is broken for (at least) > the downloader. Running something like: > ./swupdate -v -l10 --dry-run -d '-u http://localhost:8080/download/root.swu' > does not perform a dry run but an actual installation! > > With this printf-debugging patch applied > > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -504,6 +504,7 @@ void *network_initializer(void *data) > /* > * Check if the dry run flag is overwritten > */ > + INFO(">>> dry-run: %s global: %s", req->dry_run ? "true" : "false", software->globals.dry_run == 1 ? "true": "false"); > if (req->dry_run) > software->globals.dry_run = 1; > else > > you'll see > > [INFO ] : SWUPDATE running : [network_initializer] : >>> dry-run: false global: true > > for the above command. > > This is because the downloader didn't send dry_run in its request and > that overrules the global setting. So either the downloader is adapted > to send dry_run in its request (meaning it must access the global swcfg) ..and it is not the best... > or the dry_run setting logics needs to be overhauled. this is the way to do - I look into this. > > By the way, software->globals.dry_run is set to the current request's > dry_run setting but never reset to its original value prior to the > current request. So the current state of SWUpdate's idea of being in > dry run mode or not depends on the last request. I do not think this is > a good idea in particular for non one-shot modules like suricatta... This must be fixed as well, agree. > > > > In addition to that, pre and postupdate commands are run in dry run > mode. Is there a reasoning for this? Just because when I introduced dry-run mode I had no pre- or post- install scripts (I generally tend to not use them). > I would rather like to not run > them in dry run, as in the following patch: > > > diff --git a/core/installer.c b/core/installer.c > index 82b5d60..981a21d 100644 > --- a/core/installer.c > +++ b/core/installer.c > @@ -469,8 +469,12 @@ void cleanup_files(struct swupdate_cfg *software) { > int preupdatecmd(struct swupdate_cfg *swcfg) > { > if (swcfg) { > - DEBUG("Running Pre-update command"); > - return run_system_cmd(swcfg->globals.preupdatecmd); > + if (swcfg->globals.dry_run) { > + DEBUG("Dry run, skipping Pre-update command"); > + } else { > + DEBUG("Running Pre-update command"); > + return run_system_cmd(swcfg->globals.preupdatecmd); > + } > } > Agree. > return 0; > @@ -481,8 +485,13 @@ int postupdate(struct swupdate_cfg *swcfg, const char *info) > swupdate_progress_done(info); > > if (swcfg) { > - DEBUG("Running Post-update command"); > - return run_system_cmd(swcfg->globals.postupdatecmd); > + if (swcfg->globals.dry_run) { > + DEBUG("Dry run, skipping Post-update command"); > + } else { > + DEBUG("Running Post-update command"); > + return run_system_cmd(swcfg->globals.postupdatecmd); > + } > + > } > > return 0; > > If applicable, I can send this as separate patch. > +1 > > > Then, one more thing... :) > > In dry run mode, the BOOTVAR_TRANSACTION and STATE_KEY are also set. > I consider this being wrong and can send a patch fixing this if we > agree on it being wrong, that is. It is wrong. Regards, Stefano
Hi Stefano, > > on current HEAD, the SWUpdate --dry-run option is broken for (at least) > > the downloader. Running something like: > > ./swupdate -v -l10 --dry-run -d '-u http://localhost:8080/download/root.swu' > > does not perform a dry run but an actual installation! > > > > With this printf-debugging patch applied > > > > --- a/core/stream_interface.c > > +++ b/core/stream_interface.c > > @@ -504,6 +504,7 @@ void *network_initializer(void *data) > > /* > > * Check if the dry run flag is overwritten > > */ > > + INFO(">>> dry-run: %s global: %s", req->dry_run ? "true" : "false", software->globals.dry_run == 1 ? "true": "false"); > > if (req->dry_run) > > software->globals.dry_run = 1; > > else > > > > you'll see > > > > [INFO ] : SWUPDATE running : [network_initializer] : >>> dry-run: false global: true > > > > for the above command. > > > > This is because the downloader didn't send dry_run in its request and > > that overrules the global setting. So either the downloader is adapted > > to send dry_run in its request (meaning it must access the global swcfg) > > ..and it is not the best... > > > or the dry_run setting logics needs to be overhauled. > > this is the way to do - I look into this. > > > > > By the way, software->globals.dry_run is set to the current request's > > dry_run setting but never reset to its original value prior to the > > current request. So the current state of SWUpdate's idea of being in > > dry run mode or not depends on the last request. I do not think this is > > a good idea in particular for non one-shot modules like suricatta... > > This must be fixed as well, agree. > > > > > > > > > In addition to that, pre and postupdate commands are run in dry run > > mode. Is there a reasoning for this? > > Just because when I introduced dry-run mode I had no pre- or post- > install scripts (I generally tend to not use them). Yes, me neither. I just stumbled over it while investigating why the bootloader is touched in dry run mode... > > I would rather like to not run > > them in dry run, as in the following patch: > > > > > > diff --git a/core/installer.c b/core/installer.c > > index 82b5d60..981a21d 100644 > > --- a/core/installer.c > > +++ b/core/installer.c > > @@ -469,8 +469,12 @@ void cleanup_files(struct swupdate_cfg *software) { > > int preupdatecmd(struct swupdate_cfg *swcfg) > > { > > if (swcfg) { > > - DEBUG("Running Pre-update command"); > > - return run_system_cmd(swcfg->globals.preupdatecmd); > > + if (swcfg->globals.dry_run) { > > + DEBUG("Dry run, skipping Pre-update command"); > > + } else { > > + DEBUG("Running Pre-update command"); > > + return run_system_cmd(swcfg->globals.preupdatecmd); > > + } > > } > > > > Agree. > > > return 0; > > @@ -481,8 +485,13 @@ int postupdate(struct swupdate_cfg *swcfg, const char *info) > > swupdate_progress_done(info); > > > > if (swcfg) { > > - DEBUG("Running Post-update command"); > > - return run_system_cmd(swcfg->globals.postupdatecmd); > > + if (swcfg->globals.dry_run) { > > + DEBUG("Dry run, skipping Post-update command"); > > + } else { > > + DEBUG("Running Post-update command"); > > + return run_system_cmd(swcfg->globals.postupdatecmd); > > + } > > + > > } > > > > return 0; > > > > If applicable, I can send this as separate patch. > > > > +1 > > > > > > > Then, one more thing... :) > > > > In dry run mode, the BOOTVAR_TRANSACTION and STATE_KEY are also set. > > I consider this being wrong and can send a patch fixing this if we > > agree on it being wrong, that is. > > It is wrong. Just sent the two patches. One thing I also noticed while doing these patches is that when installing via swupdate -i <file> the bootloader's STATE_KEY is not set as with the "other" install method. Is there a reason behind this? Or wouldn't it be better to just get rid of the swupdate -i <file> code path and instead use the "other" for this case as well since they're doing the same thing just for different ingress channels? Kind regards, Christian
Hi Christian, On 20.11.20 20:11, Christian Storm wrote: > Hi Stefano, > >>> on current HEAD, the SWUpdate --dry-run option is broken for (at least) >>> the downloader. Running something like: >>> ./swupdate -v -l10 --dry-run -d '-u http://localhost:8080/download/root.swu' >>> does not perform a dry run but an actual installation! >>> >>> With this printf-debugging patch applied >>> >>> --- a/core/stream_interface.c >>> +++ b/core/stream_interface.c >>> @@ -504,6 +504,7 @@ void *network_initializer(void *data) >>> /* >>> * Check if the dry run flag is overwritten >>> */ >>> + INFO(">>> dry-run: %s global: %s", req->dry_run ? "true" : "false", software->globals.dry_run == 1 ? "true": "false"); >>> if (req->dry_run) >>> software->globals.dry_run = 1; >>> else >>> >>> you'll see >>> >>> [INFO ] : SWUPDATE running : [network_initializer] : >>> dry-run: false global: true >>> >>> for the above command. >>> >>> This is because the downloader didn't send dry_run in its request and >>> that overrules the global setting. So either the downloader is adapted >>> to send dry_run in its request (meaning it must access the global swcfg) >> >> ..and it is not the best... >> >>> or the dry_run setting logics needs to be overhauled. >> >> this is the way to do - I look into this. >> >>> >>> By the way, software->globals.dry_run is set to the current request's >>> dry_run setting but never reset to its original value prior to the >>> current request. So the current state of SWUpdate's idea of being in >>> dry run mode or not depends on the last request. I do not think this is >>> a good idea in particular for non one-shot modules like suricatta... >> >> This must be fixed as well, agree. >> >>> >>> >>> >>> In addition to that, pre and postupdate commands are run in dry run >>> mode. Is there a reasoning for this? >> >> Just because when I introduced dry-run mode I had no pre- or post- >> install scripts (I generally tend to not use them). > > Yes, me neither. I just stumbled over it while investigating why the > bootloader is touched in dry run mode... So it looks like the handle of dry-run is broken since it was added to IPC. A dry-run set by IPC should not survive after the single install terminates. IMHO we need: - to store the value from the command line --dry-run. This becomes the default when a request does not contain the value. - this leads to identify if a request has set dry-run or simply ask to apply default. - default value should always be applied if request does not contain the dry-run. I write a patch in this way. > > >>> I would rather like to not run >>> them in dry run, as in the following patch: >>> >>> >>> diff --git a/core/installer.c b/core/installer.c >>> index 82b5d60..981a21d 100644 >>> --- a/core/installer.c >>> +++ b/core/installer.c >>> @@ -469,8 +469,12 @@ void cleanup_files(struct swupdate_cfg *software) { >>> int preupdatecmd(struct swupdate_cfg *swcfg) >>> { >>> if (swcfg) { >>> - DEBUG("Running Pre-update command"); >>> - return run_system_cmd(swcfg->globals.preupdatecmd); >>> + if (swcfg->globals.dry_run) { >>> + DEBUG("Dry run, skipping Pre-update command"); >>> + } else { >>> + DEBUG("Running Pre-update command"); >>> + return run_system_cmd(swcfg->globals.preupdatecmd); >>> + } >>> } >>> >> >> Agree. >> >>> return 0; >>> @@ -481,8 +485,13 @@ int postupdate(struct swupdate_cfg *swcfg, const char *info) >>> swupdate_progress_done(info); >>> >>> if (swcfg) { >>> - DEBUG("Running Post-update command"); >>> - return run_system_cmd(swcfg->globals.postupdatecmd); >>> + if (swcfg->globals.dry_run) { >>> + DEBUG("Dry run, skipping Post-update command"); >>> + } else { >>> + DEBUG("Running Post-update command"); >>> + return run_system_cmd(swcfg->globals.postupdatecmd); >>> + } >>> + >>> } >>> >>> return 0; >>> >>> If applicable, I can send this as separate patch. >>> >> >> +1 >> >>> >>> >>> Then, one more thing... :) >>> >>> In dry run mode, the BOOTVAR_TRANSACTION and STATE_KEY are also set. >>> I consider this being wrong and can send a patch fixing this if we >>> agree on it being wrong, that is. >> >> It is wrong. > > Just sent the two patches. One thing I also noticed while doing these > patches is that when installing via > swupdate -i <file> > the bootloader's STATE_KEY is not set as with the "other" install > method. > Is there a reason behind this? At the early beginning, SWUpdate did not support streaming-mode. The separate workflow with "-i" was to allow to use seek() and to not copy into tmpfs. The same can be reached today if all artefacts have the "installed-directly" flag set and swupdate-client is used. If we drop the work-flow through "-i" reusing the one via IPC like streaming-client, there is a compatibility issue for all SWUs, updated locally, that not set installed-directly. Not sure which impact this can have. > Or wouldn't it be better to just > get rid of the swupdate -i <file> code path and instead use the "other" > for this case as well since they're doing the same thing just for > different ingress channels? I am thinking since a while about dropping this path, I have compatibility concerns as I explained above. Regards, Stefano > > > Kind regards, > Christian >
Hi Stefano, > > > > on current HEAD, the SWUpdate --dry-run option is broken for (at least) > > > > the downloader. Running something like: > > > > ./swupdate -v -l10 --dry-run -d '-u http://localhost:8080/download/root.swu' > > > > does not perform a dry run but an actual installation! > > > > > > > > With this printf-debugging patch applied > > > > > > > > --- a/core/stream_interface.c > > > > +++ b/core/stream_interface.c > > > > @@ -504,6 +504,7 @@ void *network_initializer(void *data) > > > > /* > > > > * Check if the dry run flag is overwritten > > > > */ > > > > + INFO(">>> dry-run: %s global: %s", req->dry_run ? "true" : "false", software->globals.dry_run == 1 ? "true": "false"); > > > > if (req->dry_run) > > > > software->globals.dry_run = 1; > > > > else > > > > > > > > you'll see > > > > > > > > [INFO ] : SWUPDATE running : [network_initializer] : >>> dry-run: false global: true > > > > > > > > for the above command. > > > > > > > > This is because the downloader didn't send dry_run in its request and > > > > that overrules the global setting. So either the downloader is adapted > > > > to send dry_run in its request (meaning it must access the global swcfg) > > > > > > ..and it is not the best... > > > > > > > or the dry_run setting logics needs to be overhauled. > > > > > > this is the way to do - I look into this. > > > > > > > > > > > By the way, software->globals.dry_run is set to the current request's > > > > dry_run setting but never reset to its original value prior to the > > > > current request. So the current state of SWUpdate's idea of being in > > > > dry run mode or not depends on the last request. I do not think this is > > > > a good idea in particular for non one-shot modules like suricatta... > > > > > > This must be fixed as well, agree. > > > > > > > > > > > > > > > > > > > In addition to that, pre and postupdate commands are run in dry run > > > > mode. Is there a reasoning for this? > > > > > > Just because when I introduced dry-run mode I had no pre- or post- > > > install scripts (I generally tend to not use them). > > > > Yes, me neither. I just stumbled over it while investigating why the > > bootloader is touched in dry run mode... > > So it looks like the handle of dry-run is broken since it was added to IPC. A > dry-run set by IPC should not survive after the single install terminates. Yes. > IMHO we need: > > - to store the value from the command line --dry-run. This becomes the default > when a request does not contain the value. Yes. > - this leads to identify if a request has set dry-run or simply ask to apply > default. Yes, if a request has set dry-run, it overrides the global default just for this one request, not touching the global setting. > - default value should always be applied if request does not contain the > dry-run. Isn't this the same as the first point? You have a global default dry run setting (either set via command line or maybe modified at runtime via an explicit IPC call to do so) and this is the default dry run mode for all requests except a request that has dry run set explicitly. > I write a patch in this way. Thanks! > > > > I would rather like to not run > > > > them in dry run, as in the following patch: > > > > > > > > > > > > diff --git a/core/installer.c b/core/installer.c > > > > index 82b5d60..981a21d 100644 > > > > --- a/core/installer.c > > > > +++ b/core/installer.c > > > > @@ -469,8 +469,12 @@ void cleanup_files(struct swupdate_cfg *software) { > > > > int preupdatecmd(struct swupdate_cfg *swcfg) > > > > { > > > > if (swcfg) { > > > > - DEBUG("Running Pre-update command"); > > > > - return run_system_cmd(swcfg->globals.preupdatecmd); > > > > + if (swcfg->globals.dry_run) { > > > > + DEBUG("Dry run, skipping Pre-update command"); > > > > + } else { > > > > + DEBUG("Running Pre-update command"); > > > > + return run_system_cmd(swcfg->globals.preupdatecmd); > > > > + } > > > > } > > > > > > Agree. > > > > > > > return 0; > > > > @@ -481,8 +485,13 @@ int postupdate(struct swupdate_cfg *swcfg, const char *info) > > > > swupdate_progress_done(info); > > > > if (swcfg) { > > > > - DEBUG("Running Post-update command"); > > > > - return run_system_cmd(swcfg->globals.postupdatecmd); > > > > + if (swcfg->globals.dry_run) { > > > > + DEBUG("Dry run, skipping Post-update command"); > > > > + } else { > > > > + DEBUG("Running Post-update command"); > > > > + return run_system_cmd(swcfg->globals.postupdatecmd); > > > > + } > > > > + > > > > } > > > > return 0; > > > > > > > > If applicable, I can send this as separate patch. > > > > > > > > > > +1 > > > > > > > > > > > > > > > Then, one more thing... :) > > > > > > > > In dry run mode, the BOOTVAR_TRANSACTION and STATE_KEY are also set. > > > > I consider this being wrong and can send a patch fixing this if we > > > > agree on it being wrong, that is. > > > > > > It is wrong. > > > > Just sent the two patches. One thing I also noticed while doing these > > patches is that when installing via > > swupdate -i <file> > > the bootloader's STATE_KEY is not set as with the "other" install > > method. > > Is there a reason behind this? > > At the early beginning, SWUpdate did not support streaming-mode. The separate > workflow with "-i" was to allow to use seek() and to not copy into tmpfs. The > same can be reached today if all artefacts have the "installed-directly" flag > set and swupdate-client is used. > > If we drop the work-flow through "-i" reusing the one via IPC like > streaming-client, there is a compatibility issue for all SWUs, updated > locally, that not set installed-directly. Not sure which impact this can have. > > > Or wouldn't it be better to just > > get rid of the swupdate -i <file> code path and instead use the "other" > > for this case as well since they're doing the same thing just for > > different ingress channels? > > I am thinking since a while about dropping this path, I have compatibility > concerns as I explained above. Hm, what about modifying the parsed sw-description representation in SWUpdate to set the "installed-directly" flag when using -i and then passing it to the "other" workflow? Then, there's no breakage in the "user interface" but we can get rid of the -i workflow code path? Kind regards, Christian
Hi Christian, On 23.11.20 09:37, Christian Storm wrote: > Hi Stefano, > >>>>> on current HEAD, the SWUpdate --dry-run option is broken for (at least) >>>>> the downloader. Running something like: >>>>> ./swupdate -v -l10 --dry-run -d '-u http://localhost:8080/download/root.swu' >>>>> does not perform a dry run but an actual installation! >>>>> >>>>> With this printf-debugging patch applied >>>>> >>>>> --- a/core/stream_interface.c >>>>> +++ b/core/stream_interface.c >>>>> @@ -504,6 +504,7 @@ void *network_initializer(void *data) >>>>> /* >>>>> * Check if the dry run flag is overwritten >>>>> */ >>>>> + INFO(">>> dry-run: %s global: %s", req->dry_run ? "true" : "false", software->globals.dry_run == 1 ? "true": "false"); >>>>> if (req->dry_run) >>>>> software->globals.dry_run = 1; >>>>> else >>>>> >>>>> you'll see >>>>> >>>>> [INFO ] : SWUPDATE running : [network_initializer] : >>> dry-run: false global: true >>>>> >>>>> for the above command. >>>>> >>>>> This is because the downloader didn't send dry_run in its request and >>>>> that overrules the global setting. So either the downloader is adapted >>>>> to send dry_run in its request (meaning it must access the global swcfg) >>>> >>>> ..and it is not the best... >>>> >>>>> or the dry_run setting logics needs to be overhauled. >>>> >>>> this is the way to do - I look into this. >>>> >>>>> >>>>> By the way, software->globals.dry_run is set to the current request's >>>>> dry_run setting but never reset to its original value prior to the >>>>> current request. So the current state of SWUpdate's idea of being in >>>>> dry run mode or not depends on the last request. I do not think this is >>>>> a good idea in particular for non one-shot modules like suricatta... >>>> >>>> This must be fixed as well, agree. >>>> >>>>> >>>>> >>>>> >>>>> In addition to that, pre and postupdate commands are run in dry run >>>>> mode. Is there a reasoning for this? >>>> >>>> Just because when I introduced dry-run mode I had no pre- or post- >>>> install scripts (I generally tend to not use them). >>> >>> Yes, me neither. I just stumbled over it while investigating why the >>> bootloader is touched in dry run mode... >> >> So it looks like the handle of dry-run is broken since it was added to IPC. A >> dry-run set by IPC should not survive after the single install terminates. > > Yes. > >> IMHO we need: >> >> - to store the value from the command line --dry-run. This becomes the default >> when a request does not contain the value. > > Yes. > >> - this leads to identify if a request has set dry-run or simply ask to apply >> default. > > Yes, if a request has set dry-run, it overrides the global default just > for this one request, not touching the global setting. > >> - default value should always be applied if request does not contain the >> dry-run. > > Isn't this the same as the first point? You have a global default dry > run setting (either set via command line or maybe modified at runtime > via an explicit IPC call to do so) and this is the default dry run mode > for all requests except a request that has dry run set explicitly. Yes, it is the same. > > > >> I write a patch in this way. > > Thanks! > > >>>>> I would rather like to not run >>>>> them in dry run, as in the following patch: >>>>> >>>>> >>>>> diff --git a/core/installer.c b/core/installer.c >>>>> index 82b5d60..981a21d 100644 >>>>> --- a/core/installer.c >>>>> +++ b/core/installer.c >>>>> @@ -469,8 +469,12 @@ void cleanup_files(struct swupdate_cfg *software) { >>>>> int preupdatecmd(struct swupdate_cfg *swcfg) >>>>> { >>>>> if (swcfg) { >>>>> - DEBUG("Running Pre-update command"); >>>>> - return run_system_cmd(swcfg->globals.preupdatecmd); >>>>> + if (swcfg->globals.dry_run) { >>>>> + DEBUG("Dry run, skipping Pre-update command"); >>>>> + } else { >>>>> + DEBUG("Running Pre-update command"); >>>>> + return run_system_cmd(swcfg->globals.preupdatecmd); >>>>> + } >>>>> } >>>> >>>> Agree. >>>> >>>>> return 0; >>>>> @@ -481,8 +485,13 @@ int postupdate(struct swupdate_cfg *swcfg, const char *info) >>>>> swupdate_progress_done(info); >>>>> if (swcfg) { >>>>> - DEBUG("Running Post-update command"); >>>>> - return run_system_cmd(swcfg->globals.postupdatecmd); >>>>> + if (swcfg->globals.dry_run) { >>>>> + DEBUG("Dry run, skipping Post-update command"); >>>>> + } else { >>>>> + DEBUG("Running Post-update command"); >>>>> + return run_system_cmd(swcfg->globals.postupdatecmd); >>>>> + } >>>>> + >>>>> } >>>>> return 0; >>>>> >>>>> If applicable, I can send this as separate patch. >>>>> >>>> >>>> +1 >>>> >>>>> >>>>> >>>>> Then, one more thing... :) >>>>> >>>>> In dry run mode, the BOOTVAR_TRANSACTION and STATE_KEY are also set. >>>>> I consider this being wrong and can send a patch fixing this if we >>>>> agree on it being wrong, that is. >>>> >>>> It is wrong. >>> >>> Just sent the two patches. One thing I also noticed while doing these >>> patches is that when installing via >>> swupdate -i <file> >>> the bootloader's STATE_KEY is not set as with the "other" install >>> method. >>> Is there a reason behind this? >> >> At the early beginning, SWUpdate did not support streaming-mode. The separate >> workflow with "-i" was to allow to use seek() and to not copy into tmpfs. The >> same can be reached today if all artefacts have the "installed-directly" flag >> set and swupdate-client is used. >> >> If we drop the work-flow through "-i" reusing the one via IPC like >> streaming-client, there is a compatibility issue for all SWUs, updated >> locally, that not set installed-directly. Not sure which impact this can have. >> >>> Or wouldn't it be better to just >>> get rid of the swupdate -i <file> code path and instead use the "other" >>> for this case as well since they're doing the same thing just for >>> different ingress channels? >> >> I am thinking since a while about dropping this path, I have compatibility >> concerns as I explained above. > > Hm, what about modifying the parsed sw-description representation in > SWUpdate to set the "installed-directly" flag when using -i and then > passing it to the "other" workflow? Then, there's no breakage in the > "user interface" but we can get rid of the -i workflow code path? I do not like to "constrain" and execute some live patching on sw-description - but yes, this is an option. Anyway, release 2020.11 is foreseen before the month is over, and I will postpone this changes after then. I have already verified that I should touch a lot of files, surely making code much cleaner, but it looks dangerous now. Best regards, Stefano
Hi Stefano, > >>> [...] > >>> Just sent the two patches. One thing I also noticed while doing these > >>> patches is that when installing via > >>> swupdate -i <file> > >>> the bootloader's STATE_KEY is not set as with the "other" install > >>> method. > >>> Is there a reason behind this? > >> > >> At the early beginning, SWUpdate did not support streaming-mode. The separate > >> workflow with "-i" was to allow to use seek() and to not copy into tmpfs. The > >> same can be reached today if all artefacts have the "installed-directly" flag > >> set and swupdate-client is used. > >> > >> If we drop the work-flow through "-i" reusing the one via IPC like > >> streaming-client, there is a compatibility issue for all SWUs, updated > >> locally, that not set installed-directly. Not sure which impact this can have. > >> > >>> Or wouldn't it be better to just > >>> get rid of the swupdate -i <file> code path and instead use the "other" > >>> for this case as well since they're doing the same thing just for > >>> different ingress channels? > >> > >> I am thinking since a while about dropping this path, I have compatibility > >> concerns as I explained above. > > > > Hm, what about modifying the parsed sw-description representation in > > SWUpdate to set the "installed-directly" flag when using -i and then > > passing it to the "other" workflow? Then, there's no breakage in the > > "user interface" but we can get rid of the -i workflow code path? > > I do not like to "constrain" and execute some live patching on > sw-description - but yes, this is an option. I do agree, this is "magic" SWUpdate does on its own while not explicitly asked to do so. However, I do not consider this "overuse of magic" to stay in the picture. Aside from testing I do not see much downsides, rather the opposite, benefits in maintenance as we don't have to maintain two paths. > Anyway, release 2020.11 is foreseen before the month is over, and > I will postpone this changes after then. I have already verified that > I should touch a lot of files, surely making code much cleaner, but it > looks dangerous now. Fine with me, looking forward to the release :) Kind regards, Christian
--- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -504,6 +504,7 @@ void *network_initializer(void *data) /* * Check if the dry run flag is overwritten */ + INFO(">>> dry-run: %s global: %s", req->dry_run ? "true" : "false", software->globals.dry_run == 1 ? "true": "false"); if (req->dry_run) software->globals.dry_run = 1; else you'll see [INFO ] : SWUPDATE running : [network_initializer] : >>> dry-run: false global: true for the above command. This is because the downloader didn't send dry_run in its request and that overrules the global setting. So either the downloader is adapted to send dry_run in its request (meaning it must access the global swcfg) or the dry_run setting logics needs to be overhauled. By the way, software->globals.dry_run is set to the current request's dry_run setting but never reset to its original value prior to the current request. So the current state of SWUpdate's idea of being in dry run mode or not depends on the last request. I do not think this is a good idea in particular for non one-shot modules like suricatta... In addition to that, pre and postupdate commands are run in dry run mode. Is there a reasoning for this? I would rather like to not run them in dry run, as in the following patch: diff --git a/core/installer.c b/core/installer.c index 82b5d60..981a21d 100644 --- a/core/installer.c +++ b/core/installer.c @@ -469,8 +469,12 @@ void cleanup_files(struct swupdate_cfg *software) { int preupdatecmd(struct swupdate_cfg *swcfg) { if (swcfg) { - DEBUG("Running Pre-update command"); - return run_system_cmd(swcfg->globals.preupdatecmd); + if (swcfg->globals.dry_run) { + DEBUG("Dry run, skipping Pre-update command"); + } else { + DEBUG("Running Pre-update command"); + return run_system_cmd(swcfg->globals.preupdatecmd); + } } return 0; @@ -481,8 +485,13 @@ int postupdate(struct swupdate_cfg *swcfg, const char *info) swupdate_progress_done(info); if (swcfg) { - DEBUG("Running Post-update command"); - return run_system_cmd(swcfg->globals.postupdatecmd); + if (swcfg->globals.dry_run) { + DEBUG("Dry run, skipping Post-update command"); + } else { + DEBUG("Running Post-update command"); + return run_system_cmd(swcfg->globals.postupdatecmd); + } + } return 0;