Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 11/20/2012 01:45 PM, Reshetova, Elena wrote: The most important use-case is to make it possible to run scripts when even /bin/sh is not available. Such as %pretrans scripts on initial install - there simply nothing to exec() in that environment. Or if the package containing /bin/sh or one of its dependencies needs scripts, there's no other choice than an >embedded script during initial install. Related to that, one can avoid problematic external dependencies in the early "bootstrap" stages of initial install. It also allows some things that aren't really possible otherwise, such as manipulating macros and "communicating" with other packages through the global Lua environment (and in theory, accessing rpmdb but that's not implemented), which would not be possible in an externally executed script. It would be possible to fork() for the execution of embedded scripts without compromising the main use-case of early install, but it would break some possible uses of the second group. Whether that's a good or bad thing... is a different question. I dunno if anybody is actually using the current possibilities >in that area. Thank you for explaining! Now I start to understand this a bit better. Providing better interfaces and possibilities for trusted packages to do needed things is always good, I guess the only thing would be to have the possibility for a plugin to implement restrictions if necessary. I agree that it would be better to make some decision on per script/package basis whenever this lua script is allowed to run and with what privileges instead of simply saying "disable lua at compile time" . I think the solution that plugin would return an error code n pre-script execution if it thinks that lua scipts aren't allowed is actually an acceptable one for now. Plugins being able to deny scriptlet execution on per-package (or scriptlet) basis seems entirely reasonable to me, considering the goals discussed here. Regardless of whether its embedded lua or a "normal" scriptlet. The better one would be only to setup the security context but this would require the fork(). I'm starting to think we'll want the ability to fork lua execution anyway. I've just been pondering over the details: simply always forking Lua scriptlets would make a whole lot of sense and eliminate the need to try and protect rpm from the things lua scripts can do. But like said, that would break any lua scriptlets depending on the in-process execution. Which might actually be just a good thing in a way. Another possibility is making the forking configurable, but its fairly ugly and wouldn't allow us to get rid of the fragile "try save and restore any damage lua might do to us" stuff. Good question... symmetry is usually good, so I guess the post-hook should be always called if pre-hook was called. Script path + return code as arguments seems like a reasonably starting point at least. (I'm not really even dreaming of getting every single detail 100% right the first time :) Ok, I will do it this way. I actually have these changes ready, but wanted to finish with three main filesystem hooks at the same time. I am hoping to do it this week. And like said, I think the plugin hooks should get called with embedded scripts too. It'll probably need another argument to let the plugins know whether an internal or external script is about to be executed - you could eg just deny execution of internal scripts from non-trusted sources then, without disabling lua entirely. Yes, I agree, this is a better alternative to a complete lua disabling. I will do it this way. Do you want the script changes in separate path then without any filesystem hooks? Smaller sets are always preferred as they're easier to review. If you have the scriptlet stuff ready, just fire away :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
>The most important use-case is to make it possible to run scripts when even /bin/sh is not available. Such as %pretrans scripts on initial install - there simply nothing to exec() in that environment. Or if the package containing /bin/sh or one of its dependencies needs scripts, there's no other choice than an >embedded script during initial install. >Related to that, one can avoid problematic external dependencies in the early "bootstrap" stages of initial install. >It also allows some things that aren't really possible otherwise, such as manipulating macros and "communicating" with other packages through the global Lua environment (and in theory, accessing rpmdb but that's not implemented), which would not be possible in an externally executed script. >It would be possible to fork() for the execution of embedded scripts without compromising the main use-case of early install, but it would break some possible uses of the second group. Whether that's a good or bad thing... is a different question. I dunno if anybody is actually using the current possibilities >in that area. Thank you for explaining! Now I start to understand this a bit better. Providing better interfaces and possibilities for trusted packages to do needed things is always good, I guess the only thing would be to have the possibility for a plugin to implement restrictions if necessary. I agree that it would be better to make some decision on per script/package basis whenever this lua script is allowed to run and with what privileges instead of simply saying "disable lua at compile time" . I think the solution that plugin would return an error code n pre-script execution if it thinks that lua scipts aren't allowed is actually an acceptable one for now. The better one would be only to setup the security context but this would require the fork(). >Good question... symmetry is usually good, so I guess the post-hook >should be always called if pre-hook was called. Script path + return >code as arguments seems like a reasonably starting point at least. (I'm >not really even dreaming of getting every single detail 100% right the >first time :) Ok, I will do it this way. I actually have these changes ready, but wanted to finish with three main filesystem hooks at the same time. I am hoping to do it this week. >And like said, I think the plugin hooks should get called with embedded >scripts too. It'll probably need another argument to let the plugins >know whether an internal or external script is about to be executed - >you could eg just deny execution of internal scripts from non-trusted >sources then, without disabling lua entirely. Yes, I agree, this is a better alternative to a complete lua disabling. I will do it this way. Do you want the script changes in separate path then without any filesystem hooks? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 11/12/2012 10:50 AM, Reshetova, Elena wrote: Scripts run with all the powers that rpm itself has, and there's not a whole lot that can be done about it: scriptlets expect to, and often need to, do all sorts of crazy things. True, but we would actually like to be able to limit things that scripts can do. It is ok for a script to create a new file or to call ldconfig, but in our setup it is not ok for script to start relabeling the xattrs on filesystem (you can do this with CAP_MAC_ADMIN for example) or rewriting important system files (you can do this with CAP_MAC_OVERRIDE and CAP_DAC_OVERRIDE). Understood, and I see nothing wrong with wanting to limit the damage rogue/runaway scriptlets can do. If you control the entire package set and compatibility with arbitrary 3rd party "big vendor" packages (which are the ones that traditionally do utterly crazy things) isn't an issue then certainly some level of added control is possible. The differences from regular exec()'ed scripts and Lua-ones are basically: - As Lua scripts run in the rpm process space, some extra protection is needed, like saving + restoring current working directory, umask and preventing Lua scriptlet from exec()'ing or exit()'ing the process. Just out of interest, is there are reason for them actually to run in rpm process space as opposite to doing the same as for regular scripts? The most important use-case is to make it possible to run scripts when even /bin/sh is not available. Such as %pretrans scripts on initial install - there simply nothing to exec() in that environment. Or if the package containing /bin/sh or one of its dependencies needs scripts, there's no other choice than an embedded script during initial install. Related to that, one can avoid problematic external dependencies in the early "bootstrap" stages of initial install. It also allows some things that aren't really possible otherwise, such as manipulating macros and "communicating" with other packages through the global Lua environment (and in theory, accessing rpmdb but that's not implemented), which would not be possible in an externally executed script. It would be possible to fork() for the execution of embedded scripts without compromising the main use-case of early install, but it would break some possible uses of the second group. Whether that's a good or bad thing... is a different question. I dunno if anybody is actually using the current possibilities in that area. - There are no context switches for Lua scripts in SELinux world currently: Lua scripts run with rpm_exec_t context, "regular" scripts run with rpm_script_t. But both are practically omnipotent contexts, because they have to be. Stephen, do you have any plans related to Lua-scripts? I guess the reason it wasn't touched so far is the same as for us: when script is run in the same context, very little can be done to make it with little priviledge and still be able to continue rpm operation. See above, I suppose we could add a configuration/compile-time switch or such to fork() the embedded script execution and then you wouldn't have to worry about getting the privileges back. Oh, I meant the common pre- and post-hook pattern seems like it would make sense for scripts too in any case, regardless of whether it helps solving the context issue. It *might* in the SELinux case, but a post-hook could have some other entirely different uses, and it'd make the whole thing a bit more >symmetric :) Sure, I can make the hooks to follow the same principle. Just I would need to understand how the post-hook would be used in order to figure out proper arguments and decide if we want it to be called if script fails or not. I guess I would rather call post-hook then even if script has failed, but juts indicate the failure. What do you think should be the argument of post-script hook? Return code from the script (to know if it failed or not) and maybe script path again then? Good question... symmetry is usually good, so I guess the post-hook should be always called if pre-hook was called. Script path + return code as arguments seems like a reasonably starting point at least. (I'm not really even dreaming of getting every single detail 100% right the first time :) And like said, I think the plugin hooks should get called with embedded scripts too. It'll probably need another argument to let the plugins know whether an internal or external script is about to be executed - you could eg just deny execution of internal scripts from non-trusted sources then, without disabling lua entirely. - Panu - Best Regards, Elena. ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
>Scripts run with all the powers that rpm itself has, and there's not a whole lot that can be done about it: scriptlets expect to, and often need to, do all sorts of crazy things. True, but we would actually like to be able to limit things that scripts can do. It is ok for a script to create a new file or to call ldconfig, but in our setup it is not ok for script to start relabeling the xattrs on filesystem (you can do this with CAP_MAC_ADMIN for example) or rewriting important system files (you can do this with CAP_MAC_OVERRIDE and CAP_DAC_OVERRIDE). >The differences from regular exec()'ed scripts and Lua-ones are basically: >- As Lua scripts run in the rpm process space, some extra protection is needed, like saving + restoring current working directory, umask and preventing Lua scriptlet from exec()'ing or exit()'ing the process. Just out of interest, is there are reason for them actually to run in rpm process space as opposite to doing the same as for regular scripts? >- There are no context switches for Lua scripts in SELinux world >currently: Lua scripts run with rpm_exec_t context, "regular" scripts run with rpm_script_t. But both are practically omnipotent contexts, because they have to be. Stephen, do you have any plans related to Lua-scripts? I guess the reason it wasn't touched so far is the same as for us: when script is run in the same context, very little can be done to make it with little priviledge and still be able to continue rpm operation. >Oh, I meant the common pre- and post-hook pattern seems like it would make sense for scripts too in any case, regardless of whether it helps solving the context issue. It *might* in the SELinux case, but a post-hook could have some other entirely different uses, and it'd make the whole thing a bit more >symmetric :) Sure, I can make the hooks to follow the same principle. Just I would need to understand how the post-hook would be used in order to figure out proper arguments and decide if we want it to be called if script fails or not. I guess I would rather call post-hook then even if script has failed, but juts indicate the failure. What do you think should be the argument of post-script hook? Return code from the script (to know if it failed or not) and maybe script path again then? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 11/09/2012 02:27 PM, Reshetova, Elena wrote: Oh and one other thing I noticed just now that'll need further thought: currently the script setup hook only runs for external scripts, but not the embedded Lua-scripts. Which are getting more and more common... They'll obviously need to be handled quite differently as they run within the rpm process itself, ie fork() + exec() does not occur. Yes, I don't currently have a very good idea how this case should be handled. The idea of script hook is that it sets the needed security context, but we obviously can't do this for lua case unless we want to drop the whole rpm security context. As a temporal and draconic measure we can compile rpm without lua support to close this hole, but it is no-go in the future since it is getting more and more usage. I guess this is one of the things that I need to think more about. In case of SELinux, AFAICS a process can change its own context back and forth, IF permitted by the policy. So at least in theory it should possible to switch to a different context while executing a scriptlet and then switch back to the original context. It isn't so easy for Smack for example (and for a good reason) and I believe SELinux won't use this functionality for this kind of purpose. Note "in theory" in the above :) libselinux has an API for it, I dont know if there's anything that's actually allowed to flip the context back and forth: /* Set the current security context to con. Note that use of this function requires that the entire application be trusted to maintain any desired separation between the old and new security contexts, unlike exec-based transitions performed via setexeccon. When possible, decompose your application and use setexeccon()+execve() instead. Note that the application may lose access to its open descriptors as a result of a setcon() unless policy allows it to use descriptors opened by the old context. */ extern int setcon(const security_context_t con); extern int setcon_raw(const security_context_t con); A process can change its security context in Smack, if it has CAP_MAC_ADMIN, so rpm (a trusted process) can have this cap and then drop the credentials when running lua script from untrusted package. However, we also want to drop CAP_MAC_ADMIN (and all the rest of sensitive caps) for such lua script (otherwise it can setup whatever context it wants for itself), and after such drop, and after lua script execution is done, rpm can't get its own security context back (because CAP_MAC_ADMIN is lost). This actually makes sense since once you run untrusted code in the same process space you can't really guarantee that you can safely come back and run (in the same process) a trusted code with high privileges. Same logic would apply if we don't speak about LSMs, but traditional Linux DAC. Rpm usually runs as root with all POSIX caps, but suppose we want to drop the sensitive caps while running untrusted lua scripts or even simply run them under different uid/gid. How do we do this with lua scripts currently? Scripts run with all the powers that rpm itself has, and there's not a whole lot that can be done about it: scriptlets expect to, and often need to, do all sorts of crazy things. The differences from regular exec()'ed scripts and Lua-ones are basically: - As Lua scripts run in the rpm process space, some extra protection is needed, like saving + restoring current working directory, umask and preventing Lua scriptlet from exec()'ing or exit()'ing the process. - There are no context switches for Lua scripts in SELinux world currently: Lua scripts run with rpm_exec_t context, "regular" scripts run with rpm_script_t. But both are practically omnipotent contexts, because they have to be. Perhaps the script hook should just follow the common pre/post-hook pattern of the other hooks afterall: pre-hook would just replace the current setup hook, and post-hook would run after the script got executed. If we add an extra argument to notify the hooks whether it's an internal or external script >(or a more generic "flags" argument to allow passing more such bits later), the plugin(s) should be able to figure out what to do about it. This would be good, if we can indeed manipulate the context freely, but because of above we really can't. Oh, I meant the common pre- and post-hook pattern seems like it would make sense for scripts too in any case, regardless of whether it helps solving the context issue. It *might* in the SELinux case, but a post-hook could have some other entirely different uses, and it'd make the whole thing a bit more symmetric :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
>> Oh and one other thing I noticed just now that'll need further thought: >> currently the script setup hook only runs for external scripts, but >> not the > embedded Lua-scripts. Which are getting more and more common... >> They'll obviously need to be handled quite differently as they run >> within > the rpm process itself, ie fork() + exec() does not occur. > > Yes, I don't currently have a very good idea how this case should be > handled. The idea of script hook is that it sets the needed security > context, but we obviously can't do this for lua case unless we want to > drop the whole rpm security context. > As a temporal and draconic measure we can compile rpm without lua > support to close this hole, but it is no-go in the future since it is > getting more and more usage. I guess this is one of the things that I > need to think more about. >In case of SELinux, AFAICS a process can change its own context back and forth, IF permitted by the policy. So at least in theory it should possible to switch to a different context while executing a scriptlet and then switch back to the original context. It isn't so easy for Smack for example (and for a good reason) and I believe SELinux won't use this functionality for this kind of purpose. A process can change its security context in Smack, if it has CAP_MAC_ADMIN, so rpm (a trusted process) can have this cap and then drop the credentials when running lua script from untrusted package. However, we also want to drop CAP_MAC_ADMIN (and all the rest of sensitive caps) for such lua script (otherwise it can setup whatever context it wants for itself), and after such drop, and after lua script execution is done, rpm can't get its own security context back (because CAP_MAC_ADMIN is lost). This actually makes sense since once you run untrusted code in the same process space you can't really guarantee that you can safely come back and run (in the same process) a trusted code with high privileges. Same logic would apply if we don't speak about LSMs, but traditional Linux DAC. Rpm usually runs as root with all POSIX caps, but suppose we want to drop the sensitive caps while running untrusted lua scripts or even simply run them under different uid/gid. How do we do this with lua scripts currently? >Perhaps the script hook should just follow the common pre/post-hook pattern of the other hooks afterall: pre-hook would just replace the current setup hook, and post-hook would run after the script got executed. If we add an extra argument to notify the hooks whether it's an internal or external script >(or a more generic "flags" argument to allow passing more such bits later), the plugin(s) should be able to figure out what to do about it. This would be good, if we can indeed manipulate the context freely, but because of above we really can't. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 11/08/2012 01:01 PM, Reshetova, Elena wrote: Okay then, done and pushed. Now that I looked closer, I spotted (and fixed) a couple of more "issues": a tiny memleak from early rpmtsSetupTransactionPlugins() return and some further cosmetics (two soft-tabs instead of one hard-tab, trailing whitespace etc), but nothing dramatic. Thank you! I will seriously try to improve my style. I am not using vim for code edits, but I think I should probably reconsider it or get some kind of editor that shows all symbols explicitly. Pain to read but I get it right at the end :) If you dont otherwise use vim, might be easier to figure out how to configure your preferred editor to honor the rpm style - since its essentially just K&R I'd assume pretty much any coding-oriented editor can deal with that. Vi(m) is a fairly strange beast initially :) Oh and one other thing I noticed just now that'll need further thought: currently the script setup hook only runs for external scripts, but not the embedded Lua-scripts. Which are getting more and more common... They'll obviously need to be handled quite differently as they run within the rpm process itself, ie fork() + exec() does not occur. Yes, I don't currently have a very good idea how this case should be handled. The idea of script hook is that it sets the needed security context, but we obviously can't do this for lua case unless we want to drop the whole rpm security context. As a temporal and draconic measure we can compile rpm without lua support to close this hole, but it is no-go in the future since it is getting more and more usage. I guess this is one of the things that I need to think more about. In case of SELinux, AFAICS a process can change its own context back and forth, IF permitted by the policy. So at least in theory it should possible to switch to a different context while executing a scriptlet and then switch back to the original context. Perhaps the script hook should just follow the common pre/post-hook pattern of the other hooks afterall: pre-hook would just replace the current setup hook, and post-hook would run after the script got executed. If we add an extra argument to notify the hooks whether it's an internal or external script (or a more generic "flags" argument to allow passing more such bits later), the plugin(s) should be able to figure out what to do about it. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
> Okay then, done and pushed. Now that I looked closer, I spotted (and > fixed) a couple of more "issues": a tiny memleak from early > rpmtsSetupTransactionPlugins() return and some further cosmetics (two soft-tabs instead of one hard-tab, trailing whitespace etc), but nothing dramatic. Thank you! I will seriously try to improve my style. I am not using vim for code edits, but I think I should probably reconsider it or get some kind of editor that shows all symbols explicitly. Pain to read but I get it right at the end :) >Oh and one other thing I noticed just now that'll need further thought: >currently the script setup hook only runs for external scripts, but not the embedded Lua-scripts. Which are getting more and more common... >They'll obviously need to be handled quite differently as they run within the rpm process itself, ie fork() + exec() does not occur. Yes, I don't currently have a very good idea how this case should be handled. The idea of script hook is that it sets the needed security context, but we obviously can't do this for lua case unless we want to drop the whole rpm security context. As a temporal and draconic measure we can compile rpm without lua support to close this hole, but it is no-go in the future since it is getting more and more usage. I guess this is one of the things that I need to think more about. > Cool. And thanks for all the work so far :) I hope this is only the beginning, I am really interested in security part of rpm! Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 11/08/2012 08:35 AM, Reshetova, Elena wrote: Hi, Sorry for the late reply: I was on holidays. No worries. Lucky you :) Sure, go ahead with rpmlog and indentation changes, if it doesn't bother you to do this! Okay then, done and pushed. Now that I looked closer, I spotted (and fixed) a couple of more "issues": a tiny memleak from early rpmtsSetupTransactionPlugins() return and some further cosmetics (two soft-tabs instead of one hard-tab, trailing whitespace etc), but nothing dramatic. Oh and one other thing I noticed just now that'll need further thought: currently the script setup hook only runs for external scripts, but not the embedded Lua-scripts. Which are getting more and more common... They'll obviously need to be handled quite differently as they run within the rpm process itself, ie fork() + exec() does not occur. I will then start concentrating on rest of the stuff: need to do some more thinking on it to begin with. Cool. And thanks for all the work so far :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
Hi,
Sorry for the late reply: I was on holidays. Sure, go ahead with rpmlog and
indentation changes, if it doesn't bother you to do this!
I will then start concentrating on rest of the stuff: need to do some more
thinking on it to begin with.
Best Regards,
Elena.
-Original Message-
From: Panu Matilainen [mailto:pmati...@laiskiainen.org]
Sent: Thursday, November 01, 2012 10:22 AM
To: Reshetova, Elena
Cc: rpm-maint@lists.rpm.org
Subject: Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 10/26/2012 12:56 PM, Reshetova, Elena wrote:
> Hi,
>
> Please find the corrected patch in the attachment.
> I really spend quite some time fighting indentation :( , but let's
> hope it succeeded now (I am not that sure since it so much changing
> when I open it with different editors/settings).
> I was trying to use only soft tabs: 4 spaces, but in places where I
> had to integrated in existing code it behaves unpredictable because of
> hard tabs that I think in some places aren't of same size (but maybe
> this is my inability to use editors).
It's much better now, thanks. There are a couple of misindentations still
but I can just as well tweak those while applying, and its not as if the
whole codebase is perfectly indented, I just want new things to land in a
shape that doesn't need immediate whitespace "fixes" as they mask away real
changes from eg "git blame". I dunno what editor(s) you use, vi(m) is what I
live and breath by - for sure its arcane but at least it doesn't mess with
the formatting you choose :) The only setting I have wrt C in ~/.vimrc is
"autocmd BufRead,BufNewFile *.[chi] set sw=4"
Apart from indentation-trivia, there's just one issue that I see:
+if (!plugins || rstreq(plugins, "")) {
+rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_plugins
macro\n"));
+ return rc;
+}
This still causes superfluous/bogus messages to be emitted on what I
consider to be the default case of no plugins being configured. I'd rather
just remove the rpmlog() line from there for now, or at least change it to
RPMLOG_DEBUG so it wont show up on regular uses like 'rpm -Uvh'.
If you dont mind me doing a couple of indentation fixes and the rpmlog()
change, I'll just go ahead and commit it with those tweaks. FWIW I'm just
about to branch off for rpm-4.11 (sooner than I originally planned but...),
the plugin changes will at least initially go to master only to give us time
and freedom to fiddle with the details.
- Panu -
smime.p7s
Description: S/MIME cryptographic signature
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 10/26/2012 12:56 PM, Reshetova, Elena wrote:
Hi,
Please find the corrected patch in the attachment.
I really spend quite some time fighting indentation :( , but let's hope it
succeeded now (I am not that sure since it so much changing when I open it
with different editors/settings).
I was trying to use only soft tabs: 4 spaces, but in places where I had to
integrated in existing code it behaves unpredictable because of hard tabs
that I think in some places aren't of same size (but maybe this is my
inability to use editors).
It's much better now, thanks. There are a couple of misindentations
still but I can just as well tweak those while applying, and its not as
if the whole codebase is perfectly indented, I just want new things to
land in a shape that doesn't need immediate whitespace "fixes" as they
mask away real changes from eg "git blame". I dunno what editor(s) you
use, vi(m) is what I live and breath by - for sure its arcane but at
least it doesn't mess with the formatting you choose :) The only setting
I have wrt C in ~/.vimrc is "autocmd BufRead,BufNewFile *.[chi] set sw=4"
Apart from indentation-trivia, there's just one issue that I see:
+if (!plugins || rstreq(plugins, "")) {
+rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_plugins
macro\n"));
+ return rc;
+}
This still causes superfluous/bogus messages to be emitted on what I
consider to be the default case of no plugins being configured. I'd
rather just remove the rpmlog() line from there for now, or at least
change it to RPMLOG_DEBUG so it wont show up on regular uses like 'rpm
-Uvh'.
If you dont mind me doing a couple of indentation fixes and the rpmlog()
change, I'll just go ahead and commit it with those tweaks. FWIW I'm
just about to branch off for rpm-4.11 (sooner than I originally planned
but...), the plugin changes will at least initially go to master only to
give us time and freedom to fiddle with the details.
- Panu -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
Hi,
Please find the corrected patch in the attachment.
I really spend quite some time fighting indentation :( , but let's hope it
succeeded now (I am not that sure since it so much changing when I open it
with different editors/settings).
I was trying to use only soft tabs: 4 spaces, but in places where I had to
integrated in existing code it behaves unpredictable because of hard tabs
that I think in some places aren't of same size (but maybe this is my
inability to use editors).
Best Regards,
Elena.
-Original Message-
From: rpm-maint-boun...@lists.rpm.org
[mailto:rpm-maint-boun...@lists.rpm.org] On Behalf Of Reshetova, Elena
Sent: Monday, October 22, 2012 3:49 PM
To: Panu Matilainen
Cc: rpm-maint@lists.rpm.org
Subject: Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
Hi Panu,
Thank you for the comments! I will post the next version of patch with
corrections soon. My comments for non-cosmetic issues are below and of
course I will fix cosmetics also!
-Original Message-
From: Panu Matilainen [mailto:pmati...@laiskiainen.org]
Sent: Thursday, October 18, 2012 11:44 AM
>AFAICS, with the exception of the actual macro names used, this is
identical to rpmpluginsAddCollectionPlugin(). While not a real showstopper,
I've grown very weary of mopping up the consequences of copy-paste coding in
rpm codebase over the course of the last five years... Unless you foresee
this >significantly diverging from the collection plugin setup, I'd prefer
having these combined or at least made to use a common internal helper from
the start to avoid creeping copy-pasteism :)
Agree, will redo in the next submission.
>Probable copy-paste error, the description should be "pre", right? :)
Oh, yes, sorry, will fix.
> diff --git a/lib/rpmte.c b/lib/rpmte.c index 35b8e3e..5972505 100644
> --- a/lib/rpmte.c
> +++ b/lib/rpmte.c
> @@ -941,7 +941,7 @@ int rpmteProcess(rpmte te, pkgGoal goal)
> int scriptstage = (goal != PKG_INSTALL && goal != PKG_ERASE);
> int test = (rpmtsFlags(te->ts) & RPMTRANS_FLAG_TEST);
> int reset_fi = (scriptstage == 0 && test == 0);
> -int failed = 1;
> +int failed = 0;
>This isn't right, failure from rpmteOpen() would return success from
>rpmteProcess() with this change.
True, a nasty error, will fix.
>
> /* Dont bother opening for elements without pre/posttrans scripts */
> if (goal == PKG_PRETRANS || goal == PKG_POSTTRANS) { @@ -955,7
> +955,17 @@ int rpmteProcess(rpmte te, pkgGoal goal)
> }
>
> if (rpmteOpen(te, reset_fi)) {
> - failed = rpmpsmRun(te->ts, te, goal);
> +
> + /* Run pre transaction element hook for all plugins */
> + if (goal != PKG_PRETRANS && goal != PKG_POSTTRANS) {
> + failed = rpmpluginsCallPsmPre(rpmtsPlugins(te->ts), te);
> + }
> + if (!failed) {
> + failed = rpmpsmRun(te->ts, te, goal);
> + /* Run post transaction element hook for all plugins*/
> + if (goal != PKG_PRETRANS && goal != PKG_POSTTRANS)
> + failed = rpmpluginsCallPsmPost(rpmtsPlugins(te->ts),
te);
> + }
>Hmm, this might be simpler to handle inside rpmpsmRun() where the
pre/posttrans/verify cruft is already weeded out to a separate case. If left
here, I'd think the condition should be on "scriptstage" instead of
PKG_PRETRANS/POSTTRANS to avoid calling these hooks on verification.
Yes, I guess I'll better move them to psm.c inside rpmpsmRun(). Will look
cleaner since no additional conditions will be needed.
>FWIW, the collection hooks kinda need to be here as they should be
>called
even if rpmteOpen() for the last collection elements failed (the logic is a
bit suspect but that's an unrelated issue), but these new hooks are
different.
Yes, I didn't touch the collection hooks, since it looks like the needs are
different between them and the new hooks.
>> +static rpmRC rpmteSetupTransactionPlugins(rpmts ts)
>Shouldn't this be rpmtsSetup... not, rpmte? :) Probably just a
>copy-paste
leftover from collection plugins which are on the transaction elements.
Yeah, another copy-paste one, sorry :(
> +{
> +rpmRC rc = 0;
> +char *plugins = NULL, *plugin = NULL;
> +char delims[] = ",";
> +
> +plugins = rpmExpand("%{?__transaction_plugins}", NULL);
> +if (!plugins || rstreq(plugins, "")) {
> +rpmlog(RPMLOG_INFO, _("Failed to expand
> + %%__transaction_plugins
macro\n"));
> + return -1;
> +}
>Not having any plugins configured is not an error and should not
>complain,
in a "normal" setup. Obviously this has to do with wanting to enforce
security plugins being loaded but we need
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
Hi Panu,
Thank you for the comments! I will post the next version of patch with
corrections soon. My comments for non-cosmetic issues are below and of
course I will fix cosmetics also!
-Original Message-
From: Panu Matilainen [mailto:pmati...@laiskiainen.org]
Sent: Thursday, October 18, 2012 11:44 AM
>AFAICS, with the exception of the actual macro names used, this is
identical to rpmpluginsAddCollectionPlugin(). While not a real showstopper,
I've grown very weary of mopping up the consequences of copy-paste coding in
rpm codebase over the course of the last five years... Unless you foresee
this >significantly diverging from the collection plugin setup, I'd prefer
having these combined or at least made to use a common internal helper from
the start to avoid creeping copy-pasteism :)
Agree, will redo in the next submission.
>Probable copy-paste error, the description should be "pre", right? :)
Oh, yes, sorry, will fix.
> diff --git a/lib/rpmte.c b/lib/rpmte.c index 35b8e3e..5972505 100644
> --- a/lib/rpmte.c
> +++ b/lib/rpmte.c
> @@ -941,7 +941,7 @@ int rpmteProcess(rpmte te, pkgGoal goal)
> int scriptstage = (goal != PKG_INSTALL && goal != PKG_ERASE);
> int test = (rpmtsFlags(te->ts) & RPMTRANS_FLAG_TEST);
> int reset_fi = (scriptstage == 0 && test == 0);
> -int failed = 1;
> +int failed = 0;
>This isn't right, failure from rpmteOpen() would return success from
>rpmteProcess() with this change.
True, a nasty error, will fix.
>
> /* Dont bother opening for elements without pre/posttrans scripts */
> if (goal == PKG_PRETRANS || goal == PKG_POSTTRANS) { @@ -955,7
> +955,17 @@ int rpmteProcess(rpmte te, pkgGoal goal)
> }
>
> if (rpmteOpen(te, reset_fi)) {
> - failed = rpmpsmRun(te->ts, te, goal);
> +
> + /* Run pre transaction element hook for all plugins */
> + if (goal != PKG_PRETRANS && goal != PKG_POSTTRANS) {
> + failed = rpmpluginsCallPsmPre(rpmtsPlugins(te->ts), te);
> + }
> + if (!failed) {
> + failed = rpmpsmRun(te->ts, te, goal);
> + /* Run post transaction element hook for all plugins*/
> + if (goal != PKG_PRETRANS && goal != PKG_POSTTRANS)
> + failed = rpmpluginsCallPsmPost(rpmtsPlugins(te->ts),
te);
> + }
>Hmm, this might be simpler to handle inside rpmpsmRun() where the
pre/posttrans/verify cruft is already weeded out to a separate case. If left
here, I'd think the condition should be on "scriptstage" instead of
PKG_PRETRANS/POSTTRANS to avoid calling these hooks on verification.
Yes, I guess I'll better move them to psm.c inside rpmpsmRun(). Will look
cleaner since no additional conditions will be needed.
>FWIW, the collection hooks kinda need to be here as they should be called
even if rpmteOpen() for the last collection elements failed (the logic is a
bit suspect but that's an unrelated issue), but these new hooks are
different.
Yes, I didn't touch the collection hooks, since it looks like the needs are
different between them and the new hooks.
>> +static rpmRC rpmteSetupTransactionPlugins(rpmts ts)
>Shouldn't this be rpmtsSetup... not, rpmte? :) Probably just a copy-paste
leftover from collection plugins which are on the transaction elements.
Yeah, another copy-paste one, sorry :(
> +{
> +rpmRC rc = 0;
> +char *plugins = NULL, *plugin = NULL;
> +char delims[] = ",";
> +
> +plugins = rpmExpand("%{?__transaction_plugins}", NULL);
> +if (!plugins || rstreq(plugins, "")) {
> +rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_plugins
macro\n"));
> + return -1;
> +}
>Not having any plugins configured is not an error and should not complain,
in a "normal" setup. Obviously this has to do with wanting to enforce
security plugins being loaded but we need to find some other way to deal
with it. For now, "%{?__transaction_plugins}" evaluating to empty should
just quietly >return with success. (I do see the return code is ignored in
the caller, but the output from here is ugly and causes most of the
test-suite to fail)
Yes, actually this return code is a leftover from the previous case when I
tried to enforce the loading. It is amazing how much you can miss when
looking to the code after many times. Fresh eye is a bliss here!
> +
> +plugin = strtok(plugins, delims);
> +while(plugin != NULL) {
> + rpmlog(RPMLOG_DEBUG, _("plugin is %s\n"), plugin);
> +if (!rpmpluginsPluginAdded(ts->plugins, (const char*)plugin)) {
> + rc = rpmpluginsAddTransactionPlugin(ts->plugins, (const
char*)plugin);
> +}
> +plugin = strtok(NULL, delims);
> +}
> +free(plugins);
> +return rc;
> +}
>...but any configured plugins failing should indeed abort the thing, so the
caller should take not of the return code.
Yes.
>Once these mostly minor nits are fixed I think we're good to go. I'm
actually quite eager to have this in so I can start moving the sel
Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
On 10/17/2012 03:50 PM, Elena Reshetova wrote:
This change adds a new type of the rpm plugin, called transaction plugin and a
set of initial hooks for this plugin.
The hooks are:
PLUGINHOOK_TSM_PRE
Pre-transaction hook that is called before an rpm transaction begins
PLUGINHOOK_TSM_POST
Post-transaction hook that is called after an rpm transaction ends
PLUGINHOOK_PSM_PRE
Pre-transaction-element hook that is called before an rpm transaction-element
is processed
PLUGINHOOK_PSM_POST
Post-transaction-element hook that is called after an rpm transaction-element
is processed
PLUGINHOOK_SCRIPT_SETUP
Per-script hook that is called once for each rpm mainainers script that is
present in the package
Each hook is called for every plugin that have this hook registered.
The avaliable transaction plugins can be specified in macros.in via
transaction_plugins element.
Okay, as this seems like more of an actual patch submission than
preliminary thing for sake of discussion, I'm switching into
nitpick-mode :) Overall this looks quite okay, most of the nits are on
cosmetic details that I previously either didn't notice or simply didn't
mention as it wasn't clear what parts you were going to further polish,
but there are a couple of "real" issues too.
diff --git a/lib/rpmplugins.c b/lib/rpmplugins.c
index 9098aa5..bb44f34 100644
--- a/lib/rpmplugins.c
+++ b/lib/rpmplugins.c
@@ -110,6 +110,39 @@ rpmRC rpmpluginsAddCollectionPlugin(rpmPlugins plugins,
const char *name)
return rc;
}
+rpmRC rpmpluginsAddTransactionPlugin(rpmPlugins plugins, const char *name)
+{
+char *path;
+char *options;
+int rc = RPMRC_FAIL;
+
+path = rpmExpand("%{?__transaction_",name, "}", NULL);
+if (!path || rstreq(path, "")) {
+ rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_%s macro\n"),
name);
+ goto exit;
+}
+
+/* split the options from the path */
+#define SKIPSPACE(s){ while (*(s) && risspace(*(s))) (s)++; }
+#define SKIPNONSPACE(s) { while (*(s) && !risspace(*(s))) (s)++; }
+options = path;
+SKIPNONSPACE(options);
+if (risspace(*options)) {
+ *options = '\0';
+ options++;
+ SKIPSPACE(options);
+}
+if (*options == '\0') {
+ options = NULL;
+}
+
+rc = rpmpluginsAdd(plugins, name, path, options);
+
+ exit:
+_free(path);
+return rc;
+}
+
AFAICS, with the exception of the actual macro names used, this is
identical to rpmpluginsAddCollectionPlugin(). While not a real
showstopper, I've grown very weary of mopping up the consequences of
copy-paste coding in rpm codebase over the course of the last five
years... Unless you foresee this significantly diverging from the
collection plugin setup, I'd prefer having these combined or at least
made to use a common internal helper from the start to avoid creeping
copy-pasteism :)
+
+rpmRC rpmpluginsCallTsmPre(rpmPlugins plugins, rpmts ts)
+{
+rpmRC (*hookFunc)(rpmts);
+int i;
+rpmRC rc = RPMRC_OK;
+const char *name = NULL;
+
+for (i = 0; i < plugins->count; i++) {
+ name = plugins->names[i];
+ RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_PRE);
+ if ( hookFunc(ts) == RPMRC_FAIL )
+ rc = RPMRC_FAIL;
+}
+
+return rc;
+}
+
+rpmRC rpmpluginsCallTsmPost(rpmPlugins plugins, rpmts ts)
+{
+rpmRC (*hookFunc)(rpmts);
+int i;
+rpmRC rc = RPMRC_OK;
+const char *name = NULL;
+
+for (i = 0; i < plugins->count; i++) {
+ name = plugins->names[i];
+ RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_POST);
+ if ( hookFunc(ts) == RPMRC_FAIL )
+ rc = RPMRC_FAIL;
Indentation is off for the entire for-block in this and all the other
rpmpluginsCall*() functions added (which seem to cry for a common helper
function but since macros are involved...) The rpm indentation style is
basically a combination of soft tabs (of width 4) and hard tabs with
minimal number of each to get to the desired indentation level, which is
four characters deeper than the previous indentation.
Also spaces dont belong before or after parenthesis (in the if).
'indent -kr' fixes these but makes "wrong" changes on some other aspects
as rpm isn't "pure" K&R style. One of these days I'll need to figure out
the magic incantation of indent switches to have it output rpm style.
Would make everybody's lives easier :)
+/** \ingroup rpmplugins
+ * Call the post transaction element plugin hook
+ * @param plugins plugins structure
+ * @param te processed transaction element
+ * @return RPMRC_OK on success, RPMRC_FAIL otherwise
+ */
+rpmRC rpmpluginsCallPsmPre(rpmPlugins plugins, rpmte te);
Probable copy-paste error, the description should be "pre", right? :)
if (xx == 0) {
- xx = execv(argv[0], argv);
+/* Run script setup hook for all plugins */
+if (rpmpluginsCallScriptSetup(plugins, argv[0]) != RPMRC_FAIL)
{
+
[Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1
This change adds a new type of the rpm plugin, called transaction plugin and a
set of initial hooks for this plugin.
The hooks are:
PLUGINHOOK_TSM_PRE
Pre-transaction hook that is called before an rpm transaction begins
PLUGINHOOK_TSM_POST
Post-transaction hook that is called after an rpm transaction ends
PLUGINHOOK_PSM_PRE
Pre-transaction-element hook that is called before an rpm transaction-element
is processed
PLUGINHOOK_PSM_POST
Post-transaction-element hook that is called after an rpm transaction-element
is processed
PLUGINHOOK_SCRIPT_SETUP
Per-script hook that is called once for each rpm mainainers script that is
present in the package
Each hook is called for every plugin that have this hook registered.
The avaliable transaction plugins can be specified in macros.in via
transaction_plugins element.
---
lib/psm.c |3 +-
lib/rpmplugins.c | 118 +
lib/rpmplugins.h | 64 -
lib/rpmscript.c | 17 +---
lib/rpmscript.h |2 +-
lib/rpmte.c | 14 ++-
lib/transaction.c | 39 ++
plugins/plugin.h | 14 +++
8 files changed, 260 insertions(+), 11 deletions(-)
diff --git a/lib/psm.c b/lib/psm.c
index 8f5376d..1a0c27e 100644
--- a/lib/psm.c
+++ b/lib/psm.c
@@ -23,6 +23,7 @@
#include "lib/rpmfi_internal.h" /* XXX replaced/states... */
#include "lib/rpmte_internal.h"/* XXX internal apis */
#include "lib/rpmdb_internal.h" /* rpmdbAdd/Remove */
+#include "lib/rpmts_internal.h" /* ts->plugins */
#include "lib/rpmscript.h"
#include "debug.h"
@@ -421,7 +422,7 @@ static rpmRC runScript(rpmpsm psm, ARGV_const_t prefixes,
rpmswEnter(rpmtsOp(psm->ts, RPMTS_OP_SCRIPTLETS), 0);
rc = rpmScriptRun(script, arg1, arg2, sfd,
- prefixes, warn_only, selinux);
+ prefixes, warn_only, selinux, psm->ts->plugins);
rpmswExit(rpmtsOp(psm->ts, RPMTS_OP_SCRIPTLETS), 0);
/* Map warn-only errors to "notfound" for script stop callback */
diff --git a/lib/rpmplugins.c b/lib/rpmplugins.c
index 9098aa5..bb44f34 100644
--- a/lib/rpmplugins.c
+++ b/lib/rpmplugins.c
@@ -110,6 +110,39 @@ rpmRC rpmpluginsAddCollectionPlugin(rpmPlugins plugins,
const char *name)
return rc;
}
+rpmRC rpmpluginsAddTransactionPlugin(rpmPlugins plugins, const char *name)
+{
+char *path;
+char *options;
+int rc = RPMRC_FAIL;
+
+path = rpmExpand("%{?__transaction_",name, "}", NULL);
+if (!path || rstreq(path, "")) {
+ rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_%s macro\n"),
name);
+ goto exit;
+}
+
+/* split the options from the path */
+#define SKIPSPACE(s){ while (*(s) && risspace(*(s))) (s)++; }
+#define SKIPNONSPACE(s) { while (*(s) && !risspace(*(s))) (s)++; }
+options = path;
+SKIPNONSPACE(options);
+if (risspace(*options)) {
+ *options = '\0';
+ options++;
+ SKIPSPACE(options);
+}
+if (*options == '\0') {
+ options = NULL;
+}
+
+rc = rpmpluginsAdd(plugins, name, path, options);
+
+ exit:
+_free(path);
+return rc;
+}
+
rpmPlugins rpmpluginsFree(rpmPlugins plugins)
{
int i;
@@ -195,3 +228,88 @@ rpmRC rpmpluginsCallCollectionPreRemove(rpmPlugins
plugins, const char *name)
RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_COLL_PRE_REMOVE);
return hookFunc();
}
+
+rpmRC rpmpluginsCallTsmPre(rpmPlugins plugins, rpmts ts)
+{
+rpmRC (*hookFunc)(rpmts);
+int i;
+rpmRC rc = RPMRC_OK;
+const char *name = NULL;
+
+for (i = 0; i < plugins->count; i++) {
+ name = plugins->names[i];
+ RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_PRE);
+ if ( hookFunc(ts) == RPMRC_FAIL )
+ rc = RPMRC_FAIL;
+}
+
+return rc;
+}
+
+rpmRC rpmpluginsCallTsmPost(rpmPlugins plugins, rpmts ts)
+{
+rpmRC (*hookFunc)(rpmts);
+int i;
+rpmRC rc = RPMRC_OK;
+const char *name = NULL;
+
+for (i = 0; i < plugins->count; i++) {
+ name = plugins->names[i];
+ RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_POST);
+ if ( hookFunc(ts) == RPMRC_FAIL )
+ rc = RPMRC_FAIL;
+}
+
+return rc;
+}
+
+rpmRC rpmpluginsCallPsmPre(rpmPlugins plugins, rpmte te)
+{
+rpmRC (*hookFunc)(rpmte);
+int i;
+rpmRC rc = RPMRC_OK;
+const char *name = NULL;
+
+for (i = 0; i < plugins->count; i++) {
+ name = plugins->names[i];
+ RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_PSM_PRE);
+ if ( hookFunc(te) == RPMRC_FAIL )
+ rc = RPMRC_FAIL;
+}
+
+return rc;
+}
+
+rpmRC rpmpluginsCallPsmPost(rpmPlugins plugins, rpmte te)
+{
+rpmRC (*hookFunc)(rpmte);
+int i;
+rpmRC rc = RPMRC_OK;
+const char *name = NULL;
+
+for (i = 0; i < plugins->count; i++) {
+ name = plugins->names[i];
+ RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_PSM_POST);
+ if ( hookFunc(te) == RPMRC_FAIL )
+ rc = RPMRC_FAIL;
+}
+
+
