Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
Ok, pushed with some minor tweaks like unchanging configure.ac and adding a missing include line. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-393173906___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
Closed #444 via 5e6f05cd8dad6c1ee6bd1e6e43f176976c9c3416. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#event-1653632285___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
Saur2000 requested changes on this pull request. I cannot find a way to leave review comments for the commit messages using GitHub, but you should change "say rpm or dnf" to "rpm or dnf" (or "e.g. rpm or dnf") in the "Optimize rpmSetCloseOnExec" commit. You should also change "when running with e.g. under Docker" to "when e.g. running under Docker" in the same commit. > @@ -959,7 +959,7 @@ AC_ARG_WITH([lua], [AS_HELP_STRING([--with-lua], [build > with lua support])], AS_IF([test "$with_lua" != no],[ PKG_CHECK_MODULES([LUA], -[lua >= 5.1], +[lua53 >= 5.1], Was this intentional? It seems unrelated. > } + + closedir(dir); + + return; Remove. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#pullrequestreview-124279051___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
@ffesti @pmatilai please see the updated (and rebased) patch set. > rpm comment style is "/* foo */", "// foo" should not be used I actually thought about it so I did `git grep '// '` which revealed quite a few uses and so I went ahead. Anyway, done. > and put the prototype to rpmio_internal.h I'm assuming you are OK with `luaext/lposix.c` to have `#include "rpmio/rpmio_internal.h"` added. > The patch set would be even nicer if moving the code into its own function > would be separated from any changes to the code. done -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-392998437___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
The patch set would be even nicer if moving the code into its own function would be separated from any changes to the code. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-392495319___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
Back from vacation... This (using /proc/self/fd when available) was what I had in mind, so we're on the right track here, thanks for the work so far. Some quick remarks == requests: - rpm comment style is "/* foo */", "// foo" should not be used - just slap the new cloexec function to end of rpmio/rpmio.c (it's quite well at home there, rpmio.c has all/most headers already and this really doesn't need a separate file) and put the prototype to rpmio_internal.h, this is not an interface we want to export - the jump to fallback label seems klunky and unnecessary, just handle the respective cases in the if-else directly -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-392494507___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
@pmatilai @ffesti PTAL -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-390716012___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
Saur2000 approved this pull request. Looks good to me. Thanks for the work. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#pullrequestreview-121521555___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
> Filed https://bugzilla.gnome.org/show_bug.cgi?id=796227 This one is bad, as ulimit might have been changed during the runtime of a process, and so using rlim_max is more correct and safe. For the same reason I have added the second commit https://github.com/rpm-software-management/rpm/pull/444/commits/aa6cc049cc43cc0ae7d7c2d40fe0c9f7d8dcb6f8 to this PR. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-390060247___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
> One other thing is, they call getrlimit(RLIMIT_NOFILE, ) and use > rl.rlim_max which seems to be a mistake -- rlim_cur should be used. Filed https://bugzilla.gnome.org/show_bug.cgi?id=796227 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-390055067___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
@cgwalters I took a look at glib implementation, they make use of `fdwalk()` if available (it's a function unique to SunOS/Solaris AFAICS), and reimplement it using /proc/self/fd if not (which is the case for Linux). This seems to be an unnecessary complication (unless we care much about Solaris). One other thing is, they call `getrlimit(RLIMIT_NOFILE, )` and use `rl.rlim_max` which seems to be a mistake -- `rlim_cur` should be used. But since this code is only used when /proc is not available it's probably nothing. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-390049589___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)
OK, patch updated to take all the review comments into account (thanks @Saur2000 for your suggestions) so it's now also fixing the `doScriptExec()` from `lib/rpmscript.c`. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-390041464___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
