| Summary: | sandbox logging of preinst and postinst file events | ||
|---|---|---|---|
| Product: | Portage Development | Reporter: | Zac Medico <zmedico> |
| Component: | Core | Assignee: | Portage team <dev-portage> |
| Status: | RESOLVED REMIND | ||
| Severity: | enhancement | CC: | dberkholz, joel, philantrop, rhill, tom.gl |
| Priority: | High | ||
| Version: | 2.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 205312 | ||
| Attachments: |
libsandsandbox file event logging support
Portage support for preinst and postinst file modification event logging libsandbox file event logging support sandbox-1.2.1.sbnotify.patch portage-2.0.51.20.installwatch.patch portage-2.0.51.20.installwatch.patch orphan counts for 56 packages portage-2.0.51.21.installwatch.patch sandbox-1.2.11 redirect EINFO and EWARN to stderr |
||
|
Description
Zac Medico
2005-04-25 02:27:27 UTC
Created attachment 57154 [details, diff]
libsandsandbox file event logging support
Created attachment 57155 [details, diff]
Portage support for preinst and postinst file modification event logging
Comment on attachment 57155 [details, diff]
Portage support for preinst and postinst file modification event logging
These patches are against sandbox-1.2.1-r3 adn portage-2.0.51.20-r4.
The patch looks quite good. A couple of small concerns though...
I haven't looked at the surrounding code yet, but is mode[1]=='+' okay? Is it guaranteed to fail before then if strlen(mode)==0?
Also, there's a couple of seemingly unrelated sections in the patches which I've copied below. What are the purpose of these?
--- sandbox-1.2.1.orig/libctest.c 2005-04-23 14:01:52.000000000 -0700
+++ sandbox-1.2.1.sbnotify/libctest.c 1969-12-31 16:00:00.000000000 -0800
@@ -1 +0,0 @@
-int main(void) { return 0; }
--- portage-2.0.51.20.orig/pym/portage.py 2005-04-23 00:39:43.000000000 -0700
+++ portage-2.0.51.20.sbnotify/pym/portage.py 2005-04-24 23:05:31.000000000 -0700
@@ -6514,13 +6520,6 @@
self.dbdir = self.dbtmpdir
print ">>> original instance of package unmerged safely."
- # We hold both directory locks.
- self.dbdir = self.dbpkgdir
- self.delete()
- movefile(self.dbtmpdir, self.dbpkgdir, mysettings=self.settings)
-
- self.unlockdb()
-
#write out our collection of md5sums
if cfgfiledict.has_key("IGNORE"):
del cfgfiledict["IGNORE"]
Created attachment 57201 [details, diff]
libsandbox file event logging support
Okay, I think this is better:
int len = strlen(mode);
if ((len>0 && (mode[0]=='w' || mode[0]=='a')) || (len>1 && mode[1]=='+'))
libctest.c was just something unnecessary...
In portage.py the POSTINST log is written in self.dbtmpdir so this line should
go *after* postinst:
movefile(self.dbtmpdir, self.dbpkgdir, mysettings=self.settings)
I moved the whole block since it seemed to go together. Is that okay? It
seems to work.
Created attachment 57355 [details, diff] sandbox-1.2.1.sbnotify.patch I've updated the libsandbox patch to parse the fopen mode with the same logic as the preexising before_syscall_open_char() function. This functonality would be quite useful if it is included in the main branch of portage. It would be of particular interest to those concerned about "cruft" on their gentoo systems: http://gentoo-wiki.com/TIP_Clean_Up_Cruft Or... we shoot the ebuild devs orphaning files to the fs? Offhand, I'm not much for modifying the sandbox code to add this feature, simply because flipping on sandbox debug and greping the logs handles it already. This is how confcache is implemented, for example. Not much for feature anyways since it implies ebuild devs are doing things they (mostly) shouldn't :) Created attachment 57443 [details, diff]
portage-2.0.51.20.installwatch.patch
Thanks for the insight ferringb! I've updated the portage patch to work with
sandbox debug (no sandbox patch necessary). The feature is simply called
"installwatch" now.
True, ebuild devs (mostly) shouldn't orphan files in the fs. But it happens
and maybe there are some circumstances where it's actually necessary.
Particularly in postinstall where maybe the package needs to be installed
first?
This feature gives convenient feedback about ebuilds that create orphans. When
possible the ebuild will be corrected. In either case we will have a record of
the changes that were made.
Like I said... what ebuilds are orpahning files? Ebuilds that do so _should_ have a damn good reason, and be the exception (well aware the "right" view there may not be reality) Portage itself was up until 2.0.51.20. Baselayout does. Shadow does. Many many do things that they shouldn't be in pkg_preinst and pkg_postinst. Created attachment 57542 [details, diff]
portage-2.0.51.20.installwatch.patch
I added some python code to clean up the sandbox debug log and format it
indentically to the CONTENTS files.
Right now I'm merging all my binary packages into a new root. After that's
finished I'll do some cross referencing to weed out the files that are already
owned by other packages. Then I'll make available some detailed statistics for
all the packages that I have installed...
Created attachment 57669 [details] orphan counts for 56 packages In my data analysis I exclude all files that were touched by more than one installation. A tarball is available containing a complete list of all the files used to compile the report: http://home.socal.rr.com/zmedico/orphans.tar.bz2 The question is still if we do care. I am not too familiar with some of the other packages listed here, but most of the orphans by baselayout is because we _do_not_want_ those in CONTENTS (/etc/shadow, /etc/passwd, /etc/group, /etc/fstab, etc), as it creates problems (think many problems of novices overwriting /etc/passwd because there was an 'update' to it), or some needed empty directories being unmerged. Created attachment 58219 [details, diff]
portage-2.0.51.21.installwatch.patch
True, many of these files do not belong in CONTENTS. That is why the patch
lists the files separately in PREINST and POSTINST.
The files detected by this feature fall into at least 3 categories:
1) Belongs to multiple packages or the whole system (/etc/passwd).
2) Belongs to a particular package and can be installed in the normal
src_install() (ebuild should be fixed).
3) Belongs to a specific package but needs to be installed during pkg_preinst()
or pkg_postinst().
Created attachment 64027 [details, diff]
sandbox-1.2.11 redirect EINFO and EWARN to stderr
I'm attaching this here in case it's useful to anyone else who is using sandbox
logging. It prevents sandbox from interferring with bash command substitution.
This can be a major problem, for example, if it injects a file path into the
arguments of rm -fr. I actually lost /bin/mktemp and /usr/bin/find this way
(maybe more)!
It actually wont be a problem in normal environment without SANDBOX_DEBUG=1, but yeah, I can understand the need to not have it to stdout. Applied in svn. This seems really interesting. If this is in released sandbox now, perhaps the portage patch could be applied and this whole thing fixed? Personally I think its ugly (implementation wise): - Not sure EINFO/EWARN should go to stderr by default - its not errors as such (nitpicking I know). Besides, just set SANDBOX_VERBOSE=0 with newer sandbox. - Really the big issue is loading libsandbox.so manually. This makes the ties too close, and any radical internal changes in the future might break things. I'd rather that it used the sandbox shell in some way, or fix that if it does not work. NB: Please see the comment for SANDBOX_ACTIVE in the sources :/ Other possible issue that I am not sure about, is if it runs on all arch's now (haven't checked if its enabled by default, blah, blah ...). Hmm, I missed that I did apply the sandbox patch, but SANDBOX_VERBOSE will make things nice and clean. I really just do not like manual preloading of libsandbox.so - that really needs to be looked at before adding it to portage. (In reply to comment #19) > Hmm, I missed that I did apply the sandbox patch As I mentioned in Comment #15, the stderr redirection is really quite essential. Otherwise, shell substitutions can behave unexpectedly and cause bad thinks to happen. (In reply to comment #19) > I really just do not like manual preloading of > libsandbox.so - that really needs to be looked at before adding it to portage. A lot has changed since I wrote that patch (I wasn't a portage dev at the time). (In reply to comment #20) > (In reply to comment #19) > > Hmm, I missed that I did apply the sandbox patch > > As I mentioned in Comment #15, the stderr redirection is really quite > essential. Otherwise, shell substitutions can behave unexpectedly and cause > bad thinks to happen. > Like I said - it is in latest version. But also SANDBOX_VERBOSE, so just set that to 0, and there will be no output from sandbox (default is SANDBOX_VERBOSE=1). so, is there any actual changes required here in sandbox ? looks like sandbox-1.3 has all the verbose output you need ? (In reply to comment #22) > so, is there any actual changes required here in sandbox ? looks like > sandbox-1.3 has all the verbose output you need ? I haven't had a chance to look at it, and I've kind of lost interest in this feature. Thanks for taking care of the sandbox side. I guess I'll reassign it to dev-portage now since that's where remaining work would be. |