| Message ID | 1506547723-26571-1-git-send-email-sbabic@denx.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Scripts are stored in /tmp as all iamges | expand |
Hi Stefano, > commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of > get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really > unused in code. That generates the following error when a script is > executed: > > Execution bit cannot be set for > > because it tries to set file in TMPDIR/scripts while the file is stored > into TMPDIR. I can reproduce this behavior, which is good :) The problem is that scripts are not extracted to $TMPDIR/scripts/<script> but to $TMPDIR/<script>, hence setting bits and executing the script cannot succeed because the file $TMPDIR/scripts/<script> is not existent. From a quick glance, it seems that this has never worked, i.e., $TMPDIR/SCRIPTS_DIR prior to or $TMPDIR/SCRIPTS_DIR_SUFFIX after the patch was not properly used, and the patch surfaced this. For making it work, either (1) the extraction process has to be adapted to respect $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or (2) we ditch this altogether and plainly use $TMPDIR/ for everything. Then, however, we do not have separation of the various types of artifacts.. So, what do you think we should go for? Kind regards, Christian
> > commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of > > get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really > > unused in code. That generates the following error when a script is > > executed: > > > > Execution bit cannot be set for > > > > because it tries to set file in TMPDIR/scripts while the file is stored > > into TMPDIR. > > I can reproduce this behavior, which is good :) > The problem is that scripts are not extracted to $TMPDIR/scripts/<script> > but to $TMPDIR/<script>, hence setting bits and executing the script cannot > succeed because the file $TMPDIR/scripts/<script> is not existent. > > From a quick glance, it seems that this has never worked, i.e., > $TMPDIR/SCRIPTS_DIR prior to or $TMPDIR/SCRIPTS_DIR_SUFFIX after the > patch was not properly used, and the patch surfaced this. > For making it work, either > (1) the extraction process has to be adapted to respect > $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or For this choice, the following works: --- a/corelib/installer.c +++ b/corelib/installer.c @@ -257,9 +257,11 @@ int install_images(struct swupdate_cfg *sw, int fdsw, int fromfile) /* Extract all scripts, preinstall scripts must be run now */ if (fromfile) { - ret = extract_script(fdsw, &sw->scripts, TMPDIR); + char* TMPDIR_SCRIPTS = alloca(strlen(TMPDIR)+strlen(SCRIPTS_DIR_SUFFIX)+1); + sprintf(TMPDIR_SCRIPTS, "%s%s", TMPDIR, SCRIPTS_DIR_SUFFIX); + ret = extract_script(fdsw, &sw->scripts, TMPDIR_SCRIPTS); if (ret) { - ERROR("extracting script to TMPDIR failed"); + ERROR("extracting script to %s failed", TMPDIR_SCRIPTS); return ret; } } If we opt for this, I'll prepare a proper patch, just let me know... Kind regards, Christian
On 28/09/2017 09:47, Christian Storm wrote: > Hi Stefano, > >> commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of >> get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really >> unused in code. That generates the following error when a script is >> executed: >> >> Execution bit cannot be set for >> >> because it tries to set file in TMPDIR/scripts while the file is stored >> into TMPDIR. > > I can reproduce this behavior, which is good :) > The problem is that scripts are not extracted to $TMPDIR/scripts/<script> > but to $TMPDIR/<script>, hence setting bits and executing the script cannot > succeed because the file $TMPDIR/scripts/<script> is not existent. > That's right. > From a quick glance, it seems that this has never worked, i.e., > $TMPDIR/SCRIPTS_DIR prior to or $TMPDIR/SCRIPTS_DIR_SUFFIX after the > patch was not properly used, and the patch surfaced this. Agree. In fact, it seems I had the initial idea to put scripts / images in separate directories, and later I changed my mind without cleaning up the created directory. In fact: do we really need to split up the artifacts in separate directory ? Why ? It looks like that the initial idea is wrong and just adds not necessary code for creating and removing. > For making it work, either > (1) the extraction process has to be adapted to respect > $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or I have an issue with this approach. This is ok when scripts are put in the "scripts" entry, as they are supposed (initially) to be. But what about : images { filename = "myscript"; type = "shellscript"; } This is also correct. It is an image with a correct type, and SWUpdate handles this as usual without beeing aware that is a script (of course, streaming is not allowed). This could be useful (I never used it, anyway) in case we need to run a script after installing a single artifact, because it belongs to the list of images and not to the scripts. If it is not managed in a different way as the rest, that is it is temporary stored in TMPDIR, this works. If we specialize the destination directory, it does not. > (2) we ditch this altogether and plainly use $TMPDIR/ for everything. > Then, however, we do not have separation of the various types of > artifacts.. But again: do we need it ? So I vote for 2), what do you think ? I am checking patches you sent yesterday, it seems you post for 1), but some patches are related to different issues. > > So, what do you think we should go for? > > Best regards, Stefano
On 28/09/2017 10:04, [ext] Christian Storm wrote: > >>> commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of >>> get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really >>> unused in code. That generates the following error when a script is >>> executed: >>> >>> Execution bit cannot be set for >>> >>> because it tries to set file in TMPDIR/scripts while the file is stored >>> into TMPDIR. >> >> I can reproduce this behavior, which is good :) >> The problem is that scripts are not extracted to $TMPDIR/scripts/<script> >> but to $TMPDIR/<script>, hence setting bits and executing the script cannot >> succeed because the file $TMPDIR/scripts/<script> is not existent. >> >> From a quick glance, it seems that this has never worked, i.e., >> $TMPDIR/SCRIPTS_DIR prior to or $TMPDIR/SCRIPTS_DIR_SUFFIX after the >> patch was not properly used, and the patch surfaced this. >> For making it work, either >> (1) the extraction process has to be adapted to respect >> $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or > > For this choice, the following works: > > --- a/corelib/installer.c > +++ b/corelib/installer.c > @@ -257,9 +257,11 @@ int install_images(struct swupdate_cfg *sw, int fdsw, int fromfile) > > /* Extract all scripts, preinstall scripts must be run now */ > if (fromfile) { > - ret = extract_script(fdsw, &sw->scripts, TMPDIR); > + char* TMPDIR_SCRIPTS = alloca(strlen(TMPDIR)+strlen(SCRIPTS_DIR_SUFFIX)+1); > + sprintf(TMPDIR_SCRIPTS, "%s%s", TMPDIR, SCRIPTS_DIR_SUFFIX); > + ret = extract_script(fdsw, &sw->scripts, TMPDIR_SCRIPTS); > if (ret) { > - ERROR("extracting script to TMPDIR failed"); > + ERROR("extracting script to %s failed", TMPDIR_SCRIPTS); > return ret; > } > } > > > If we opt for this, I'll prepare a proper patch, just let me know... My feeling is for 2), see my previous answer, if you cannot convince me that 1) is better. Regards, Stefano
Hi Stefano, > >> commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of > >> get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really > >> unused in code. That generates the following error when a script is > >> executed: > >> > >> Execution bit cannot be set for > >> > >> because it tries to set file in TMPDIR/scripts while the file is stored > >> into TMPDIR. > > > > I can reproduce this behavior, which is good :) > > The problem is that scripts are not extracted to $TMPDIR/scripts/<script> > > but to $TMPDIR/<script>, hence setting bits and executing the script cannot > > succeed because the file $TMPDIR/scripts/<script> is not existent. > > > > That's right. > > > From a quick glance, it seems that this has never worked, i.e., > > $TMPDIR/SCRIPTS_DIR prior to or $TMPDIR/SCRIPTS_DIR_SUFFIX after the > > patch was not properly used, and the patch surfaced this. > > Agree. In fact, it seems I had the initial idea to put scripts / images > in separate directories, and later I changed my mind without cleaning up > the created directory. Ah, I see. I was already wondering whether I messed something up in the TMPDIR patch series but as these changes were just syntactical I dug deeper... > In fact: do we really need to split up the artifacts in separate > directory ? Why ? It looks like that the initial idea is wrong and just > adds not necessary code for creating and removing. See my comment at the end on this. > > For making it work, either > > (1) the extraction process has to be adapted to respect > > $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or > > I have an issue with this approach. This is ok when scripts are put in > the "scripts" entry, as they are supposed (initially) to be. But what > about : > > images { > filename = "myscript"; > type = "shellscript"; > } > > This is also correct. It is an image with a correct type, and SWUpdate > handles this as usual without beeing aware that is a script (of course, > streaming is not allowed). > > This could be useful (I never used it, anyway) in case we need to run a > script after installing a single artifact, because it belongs to the > list of images and not to the scripts. Yes, it is syntactically correct but we can argue about the semantics :) For this example use case, you're relying on the order of the artifacts being processed resembling their order in the sw-description, and this is very implicit. Then, one cannot change the order of processing them in SWUpdate, for whatever reason, without breaking existing deployments. I'm not sure I like this. Hence, I would even go further and blacklist processing an image of type script. What I already thought about and what may come in handy here is using a custom Lua handler for this with the extension that a custom Lua handler can call other handlers, for example: I write a custom Lua handler that calls, e.g., raw_handler with the artifact and then does things afterwards. Only thing we have to do for realizing this is making the registered handlers available to Lua for being called. What do you think of this? Another solution would be to use embedded-script = ".." for this use case. > If it is not managed in a different way as the rest, that is it is > temporary stored in TMPDIR, this works. If we specialize the destination > directory, it does not. > > > > (2) we ditch this altogether and plainly use $TMPDIR/ for everything. > > Then, however, we do not have separation of the various types of > > artifacts.. > > But again: do we need it ? So I vote for 2), what do you think ? > > I am checking patches you sent yesterday, it seems you post for 1), Yes, I do. I opt for separation because this way I only have to mount $TMPDIR/scripts -o exec. The other artifacts (temporarily) stored in $TMPDIR/... are just data and need not to be executable, i.e., these directories can be mounted -o noexec. I would consider this added value. And since the code is already in place -- fixed with the patch -- I'd like to just cash in the added value :) Eventually, I'd like to see get_tmpdir() to return (configurable) different paths -- not necessarily prefixed with $TMPDIR or /tmp -- for the different types of artifacts, i.e., more separation, not less. > but some patches are related to different issues. Yes, I flushed my patch queue :) Kind regards, Christian
Hi Christian, On 29/09/2017 15:32, Christian Storm wrote: > Hi Stefano, > >>>> commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of >>>> get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really >>>> unused in code. That generates the following error when a script is >>>> executed: >>>> >>>> Execution bit cannot be set for >>>> >>>> because it tries to set file in TMPDIR/scripts while the file is stored >>>> into TMPDIR. >>> >>> I can reproduce this behavior, which is good :) >>> The problem is that scripts are not extracted to $TMPDIR/scripts/<script> >>> but to $TMPDIR/<script>, hence setting bits and executing the script cannot >>> succeed because the file $TMPDIR/scripts/<script> is not existent. >>> >> >> That's right. >> >>> From a quick glance, it seems that this has never worked, i.e., >>> $TMPDIR/SCRIPTS_DIR prior to or $TMPDIR/SCRIPTS_DIR_SUFFIX after the >>> patch was not properly used, and the patch surfaced this. >> >> Agree. In fact, it seems I had the initial idea to put scripts / images >> in separate directories, and later I changed my mind without cleaning up >> the created directory. > > Ah, I see. I was already wondering whether I messed something up in the > TMPDIR patch series but as these changes were just syntactical I dug deeper... :-) > > >> In fact: do we really need to split up the artifacts in separate >> directory ? Why ? It looks like that the initial idea is wrong and just >> adds not necessary code for creating and removing. > > See my comment at the end on this. > > >>> For making it work, either >>> (1) the extraction process has to be adapted to respect >>> $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or >> >> I have an issue with this approach. This is ok when scripts are put in >> the "scripts" entry, as they are supposed (initially) to be. But what >> about : >> >> images { >> filename = "myscript"; >> type = "shellscript"; >> } >> >> This is also correct. It is an image with a correct type, and SWUpdate >> handles this as usual without beeing aware that is a script (of course, >> streaming is not allowed). >> >> This could be useful (I never used it, anyway) in case we need to run a >> script after installing a single artifact, because it belongs to the >> list of images and not to the scripts. > > Yes, it is syntactically correct but we can argue about the semantics :) > For this example use case, you're relying on the order of the artifacts > being processed resembling their order in the sw-description, and this > is very implicit. I have never had this use case. However, this is currently correct. I would like to avoid to have to blacklist handlers or to have dependencies from handlers. We agree that the case is syntactically correct. Script will be expanden into TMPDIR, as other images. Just at run-time we get an error, that cannot tells us anything more. We can just have a "File not found" or "Cannot set execution bit", but nothing that it was put in the wrong place (images instead of scripts). To return a correct error, the parser should be aware if an handler can be put in a specific section (images, files, scripts, bootloader). > Then, one cannot change the order of processing them > in SWUpdate, for whatever reason, without breaking existing deployments. Yes, this is what I always avoided - see my answer to a previous post. SWUpdate guarantees that installations are always in the same order, but not that the order is preserved from sw-description. Not that this cannot be reached, but I have always found it very fragile. > I'm not sure I like this. Hence, I would even go further and blacklist > processing an image of type script. Anyway, I do not like to have a run time error when it should be clear that sw-description is wrong. But on the other end, just the handler should know how it should run, and the parser should be unaware of it - that is, no blacklist hardcoded in the parser. One way to fix thsi will be if we add additional information when a handler register itself, telling the parser which sections are allowed for it. And the parser can check this. > > What I already thought about and what may come in handy here is using a > custom Lua handler for this with the extension that a custom Lua handler > can call other handlers, for example: I write a custom Lua handler that > calls, e.g., raw_handler with the artifact and then does things afterwards. > Only thing we have to do for realizing this is making the registered > handlers available to Lua for being called. > > What do you think of this? I confess that chaining handlers (not explicitely LUA) is something I have already thought - for some other reason. For example, I can imagine use case where the "archive" handler is called first, and after that the result is iterated on all extracted files calling another handler. But at the moment, it is just a thought... > > > Another solution would be to use embedded-script = ".." for this use case. Do not think twice - I have not seen this use case ! And because we install the whole system, and we have not a packaging system, it is perfectly plausible to have just pre- and post- install scripts. A script running in between was not foreseen, but anyware, putting scripts into SCRIPTS_DIR creates an incompatibility. At least, I wanted to remark and track in the ML that the usage is discouraged and it is a side-effect. > > >> If it is not managed in a different way as the rest, that is it is >> temporary stored in TMPDIR, this works. If we specialize the destination >> directory, it does not. >> >> >>> (2) we ditch this altogether and plainly use $TMPDIR/ for everything. >>> Then, however, we do not have separation of the various types of >>> artifacts.. >> >> But again: do we need it ? So I vote for 2), what do you think ? >> >> I am checking patches you sent yesterday, it seems you post for 1), > > Yes, I do. I opt for separation because this way I only have to mount > $TMPDIR/scripts -o exec. The other artifacts (temporarily) stored in > $TMPDIR/... are just data and need not to be executable, i.e., these > directories can be mounted -o noexec. ok, I see. As I said, it is not that I am against 1), but I wanted to remark here what it does mean. I agree we can go on on this, but I would like to have later a mechanism to report the error at parsing time, not when installing. > > I would consider this added value. And since the code is already in > place -- fixed with the patch -- I'd like to just cash in the added > value :) Fine with me. > > Eventually, I'd like to see get_tmpdir() to return (configurable) > different paths -- not necessarily prefixed with $TMPDIR or /tmp -- for > the different types of artifacts, i.e., more separation, not less. > > >> but some patches are related to different issues. > > Yes, I flushed my patch queue :) Ok, I check them, then :-) Best regards, Stefano
Hi Stefano, > >>> [...] > >>> For making it work, either > >>> (1) the extraction process has to be adapted to respect > >>> $TMPDIR/SCRIPTS_DIR_SUFFIX for extracting scripts or > >> > >> I have an issue with this approach. This is ok when scripts are put in > >> the "scripts" entry, as they are supposed (initially) to be. But what > >> about : > >> > >> images { > >> filename = "myscript"; > >> type = "shellscript"; > >> } > >> > >> This is also correct. It is an image with a correct type, and SWUpdate > >> handles this as usual without beeing aware that is a script (of course, > >> streaming is not allowed). > >> > >> This could be useful (I never used it, anyway) in case we need to run a > >> script after installing a single artifact, because it belongs to the > >> list of images and not to the scripts. > > > > Yes, it is syntactically correct but we can argue about the semantics :) > > For this example use case, you're relying on the order of the artifacts > > being processed resembling their order in the sw-description, and this > > is very implicit. > > I have never had this use case. However, this is currently correct. I > would like to avoid to have to blacklist handlers or to have > dependencies from handlers. > > We agree that the case is syntactically correct. Script will be expanden > into TMPDIR, as other images. Just at run-time we get an error, that > cannot tells us anything more. We can just have a "File not found" or > "Cannot set execution bit", but nothing that it was put in the wrong > place (images instead of scripts). > > To return a correct error, the parser should be aware if an handler can > be put in a specific section (images, files, scripts, bootloader). > > > Then, one cannot change the order of processing them > > in SWUpdate, for whatever reason, without breaking existing deployments. > > Yes, this is what I always avoided - see my answer to a previous post. > SWUpdate guarantees that installations are always in the same order, but > not that the order is preserved from sw-description. Not that this > cannot be reached, but I have always found it very fragile. > > > I'm not sure I like this. Hence, I would even go further and blacklist > > processing an image of type script. > > Anyway, I do not like to have a run time error when it should be clear > that sw-description is wrong. But on the other end, just the handler > should know how it should run, and the parser should be unaware of it - > that is, no blacklist hardcoded in the parser. > > One way to fix thsi will be if we add additional information when a > handler register itself, telling the parser which sections are allowed > for it. And the parser can check this. Yes, this is a good solution in my opinion. > > What I already thought about and what may come in handy here is using a > > custom Lua handler for this with the extension that a custom Lua handler > > can call other handlers, for example: I write a custom Lua handler that > > calls, e.g., raw_handler with the artifact and then does things afterwards. > > Only thing we have to do for realizing this is making the registered > > handlers available to Lua for being called. > > > > What do you think of this? > > I confess that chaining handlers (not explicitely LUA) is something I > have already thought - for some other reason. > > For example, I can imagine use case where the "archive" handler is > called first, and after that the result is iterated on all extracted > files calling another handler. But at the moment, it is just a thought... Problem is, how to specify this behavior? Either you (1) code a handler having this behavior, or you (2) extend sw-description format in this regard, or you (3) rely on a Lua handler having this behavior. Option (1) is not flexible enough as it's just for one (chaining) purpose. Option (2) requires a proper DSL formulated for this purpose. It mustn't necessarily be an extension of sw-description but it may as well be an extra artifact in the .swu of type "chainedhandler" (or whatever name) and this handler does interpret it. Option (3) is a low hanging fruit :) If, eventually, option (2) is implemented, there's still a right to exist for option (3) because of offering more flexibility (i.e., do whatever you like beyond "just" the chaining of handlers). Kind regards, Christian
Hi Christian, On 29/09/2017 16:37, Christian Storm wrote: >> One way to fix thsi will be if we add additional information when a >> handler register itself, telling the parser which sections are allowed >> for it. And the parser can check this. > > Yes, this is a good solution in my opinion. > > >>> What I already thought about and what may come in handy here is using a >>> custom Lua handler for this with the extension that a custom Lua handler >>> can call other handlers, for example: I write a custom Lua handler that >>> calls, e.g., raw_handler with the artifact and then does things afterwards. >>> Only thing we have to do for realizing this is making the registered >>> handlers available to Lua for being called. >>> >>> What do you think of this? >> >> I confess that chaining handlers (not explicitely LUA) is something I >> have already thought - for some other reason. >> >> For example, I can imagine use case where the "archive" handler is >> called first, and after that the result is iterated on all extracted >> files calling another handler. But at the moment, it is just a thought... > > Problem is, how to specify this behavior? Either you > (1) code a handler having this behavior, or you No > (2) extend sw-description format in this regard, or you This is my intention, yes. > (3) rely on a Lua handler having this behavior. > > Option (1) is not flexible enough as it's just for one (chaining) > purpose. Right, we can discharge it. > > Option (2) requires a proper DSL formulated for this purpose. It mustn't > necessarily be an extension of sw-description but it may as well be an > extra artifact in the .swu of type "chainedhandler" (or whatever name) > and this handler does interpret it. Something in this direction - I am not against any changes to sw-description, but compatibility must be ensured. > > Option (3) is a low hanging fruit :) > If, eventually, option (2) is implemented, there's still a right to > exist for option (3) because of offering more flexibility (i.e., do > whatever you like beyond "just" the chaining of handlers). Exactly, so (3) can be always be implemented - it is not that (2) closes the door to (3). However, if I go only to (3), the feature can be misunderstood or less used. In fact, I have the feeling (from my projects, this ML, etc.) that just a few project uses LUA, most of them not. And having a LUA handler, that is not strict ruled because we have full freedom when we use LUA handler, makes things for most people difficult. So my preference is to (2), but without blocking (3). Best regards, Stefano
diff --git a/handlers/lua_scripthandler.c b/handlers/lua_scripthandler.c index 1793878..b842e1b 100644 --- a/handlers/lua_scripthandler.c +++ b/handlers/lua_scripthandler.c @@ -68,7 +68,7 @@ static int start_lua_script(struct img_type *img, void *data) } snprintf(filename, sizeof(filename), - "%s%s%s", TMPDIR, SCRIPTS_DIR_SUFFIX, img->fname); + "%s%s", TMPDIR, img->fname); TRACE("Calling Lua %s", filename); luaL_openlibs(L); /* opens the standard libraries */ diff --git a/handlers/shell_scripthandler.c b/handlers/shell_scripthandler.c index 9095a5f..3506f6f 100644 --- a/handlers/shell_scripthandler.c +++ b/handlers/shell_scripthandler.c @@ -46,13 +46,13 @@ static int execute_shell_script(struct img_type *img, const char *fnname) strlen(SCRIPTS_DIR_SUFFIX) + strlen("postinst") + 2]; snprintf(shellscript, sizeof(shellscript), - "%s%s%s", TMPDIR, SCRIPTS_DIR_SUFFIX, img->fname); + "%s%s", TMPDIR, img->fname); if (chmod(shellscript, S_IRUSR | S_IWUSR | S_IXUSR)) { ERROR("Execution bit cannot be set for %s", shellscript); return -1; } snprintf(shellscript, sizeof(shellscript), - "%s%s%s %s", TMPDIR, SCRIPTS_DIR_SUFFIX, img->fname, fnname); + "%s%s %s", TMPDIR, img->fname, fnname); ret = system(shellscript); if (WIFEXITED(ret)) {
commit 97e3f8a0bc6afc1dda1c8579e196498c2177c699 makes usage of get_tmpdir(), but relies on the SCRIPTS_DIR_SUFFIX, that is really unused in code. That generates the following error when a script is executed: Execution bit cannot be set for because it tries to set file in TMPDIR/scripts while the file is stored into TMPDIR. Signed-off-by: Stefano Babic <sbabic@denx.de> Reported-by: Arun Sooraj PS <arunsoorajj@gmail.com> CC: Arun Sooraj PS <arunsoorajj@gmail.com> CC: Christian Storm <christian.storm@siemens.com> --- handlers/lua_scripthandler.c | 2 +- handlers/shell_scripthandler.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)