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)
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] luaext/Pexec: optimize setting CLOEXEC (#444)
Saur2000 requested changes on this pull request.
> + struct dirent *entry;
+ while ((entry = readdir(dir)) != NULL) {
+ fd = atoi(entry->d_name);
+ if (fd >= min_fd)
+ set_cloexec(fd);
+ }
+ closedir(dir);
+
+ return;
+
+fallback:
+ // iterate over all possible fds
+ for (fd = min_fd; fd < open_max; fd++)
+ set_cloexec(fd);
+}
+
static int Pexec(lua_State *L) /** exec(path,[args]) */
You need to do the same change for doScriptExec() in lib/rpmscript.c as well.
At least in our case, that is where all the time is spent.
> @@ -330,26 +330,64 @@ static int Pmkfifo(lua_State *L)/**
> mkfifo(path) */
}
+static void set_cloexec(int fd)
+{
+ int flag = fcntl(fd, F_GETFD);
Change "flag" to "flags". Even if FD_CLOEXEC is currently the only defined
flag, we may as well make the code forward compatible,
> @@ -330,26 +330,64 @@ static int Pmkfifo(lua_State *L)/**
> mkfifo(path) */
}
+static void set_cloexec(int fd)
+{
+ int flag = fcntl(fd, F_GETFD);
+
+ if (flag == -1 || (flag & FD_CLOEXEC))
+ return;
+
+ fcntl(fd, F_SETFD, FD_CLOEXEC);
Change "FD_CLOEXEC" to "flags | FD_CLOEXEC".
> + if (flag == -1 || (flag & FD_CLOEXEC))
+ return;
+
+ fcntl(fd, F_SETFD, FD_CLOEXEC);
+}
+
+static void set_cloexec_all(void)
+{
+ const int min_fd = 3; // don't touch stdin/out/err
+ const int default_open_max = 1024;
+ int fd, open_max;
+
+ open_max = sysconf(_SC_OPEN_MAX);
+ if (open_max == -1) {
+ open_max = default_open_max;
+ goto fallback;
I see no reason to goto fallback here.
> + fcntl(fd, F_SETFD, FD_CLOEXEC);
+}
+
+static void set_cloexec_all(void)
+{
+ const int min_fd = 3; // don't touch stdin/out/err
+ const int default_open_max = 1024;
+ int fd, open_max;
+
+ open_max = sysconf(_SC_OPEN_MAX);
+ if (open_max == -1) {
+ open_max = default_open_max;
+ goto fallback;
+ }
+
+ if (open_max <= default_open_max) {
I doubt you need to be this conservative. I don't have any measurements to
support it, but I'd be very surprised if it is not a win to remove this if
statement and always use the code that only traverses the open file descriptors.
--
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-121202950___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add a new option --alldeps to rpmdeps (#220)
I have updated the change now to hopefully be more in line with what you had in mind. -- 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/220#issuecomment-304922491___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add a new option --all-per-file to rpmdeps (#220)
\*ping* -- 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/220#issuecomment-303698411___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add a new option --all-per-file to rpmdeps (#220)
Yes, there is code in OE-core that needs to run `rpmdeps` directly. What it needs is a way to get the dependencies per input file. I.e., when it runs a command such as `rpmdeps --all-per-file lib/libuuid.so.1.3.0 usr/bin/uuidgen`, it needs the filename, the dependency types and the names of the dependencies: 0 lib/libuuid.so.1.3.0 0x4001 [elf] P libuuid.so.1(UUID_1.0) P libuuid.so.1(UUID_2.20) P libuuid.so.1(UUID_2.20) P libuuid.so.1(UUIDD_PRIVATE) P libuuid.so.1 R libc.so.6 R ld.so.1 R ld.so.1(GLIBC_2.3) R ld.so.1(GLIBC_2.4) R libc.so.6(GLIBC_2.3) R libc.so.6(GLIBC_2.3.4) R libc.so.6(GLIBC_2.4) R libc.so.6(GLIBC_2.0) R libc.so.6(GLIBC_2.2) 1 usr/bin/uuidgen 0x4001 [elf] R libuuid.so.1 R libc.so.6 R ld.so.1 R ld.so.1(GLIBC_2.4) R libc.so.6(GLIBC_2.3.4) R libc.so.6(GLIBC_2.4) R libc.so.6(GLIBC_2.2) R libc.so.6(GLIBC_2.0) R libuuid.so.1(UUID_1.0) If I understand you correctly, you suggest that `--alldeps` would output the above but without the `0x4001 [elf]` part (unless `--rpmfcdebug` is also specified)? And that it should be mutually exclusive with `--requires`& co? And if `--rpmfcdebug` is used together with, e.g., `--requires` it would work exactly as it does today? -- 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/220#issuecomment-302673115___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Add a new option --all-per-file to rpmdeps (#220)
This is an alternative solution to the part in https://github.com/rpm-software-management/rpm/pull/216 that was rejected. Rather than changing the behavior of the existing `--rpmfcdebug` option, this adds a new option `--all-per-file`, which outputs the same information as `--rpmfcdebug` does, but to stdout. The reason why this is needed and why not to just use `rpmdeps --rpmfcdebug ... 2>&1` is that if there is an error, the error message would then end up on stdout as well and never be shown to the user who then has no idea why the command failed (assuming that stdout is caught by some other process that uses the output from `rpmdeps` as its input). I am not very happy about the option name, `--all-per-file`, but it was the best I could come up with to indicate that it outputs all dependencies per input file. Feel free to suggest alternatives. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/220 -- Commit Summary -- * Add a new option --all-per-file to rpmdeps -- File Changes -- M tools/rpmdeps.c (7) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/220.patch https://github.com/rpm-software-management/rpm/pull/220.diff -- 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/220 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Three fixes for rpmdeps (#216)
Would it be an alternative to add a new option to `rpmdeps` that outputs the result from `rpmfcPrint()` to stdout? Since in my case, that output is all that I need. I am working on fixing a regression in OpenEmbedded that was introduced with the switch to dnf and rpm4 in the latest release (Pyro). And to fix that I need a command that can output the dependencies per file specified on the command line, which is exactly what `rpmdeps --rpmfcdebug` does. I can of course add a patch to OE-core to change the output from stderr to stdout (which I will have to do anyway until rpm is updated), but it would be nice to be able to get rid of it with a later update to rpm. -- 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/216#issuecomment-301871385___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Two fixes for rpmdeps (#216)
The first commit fixes the use of an uninitialized buffer in rpmdeps and should be trivial to merge. The second commit changes `rpmdeps --rpmfcdebug` to output to stdout. This makes more sense when using it as the sole output from rpmdeps, as the output will no longer mix with any warnings sent to stderr. I guess it may make less sense if --rpmfcdebug is used together with any of the other output options, e.g., --provides or --requires. It would be possible to send the output from --rpmfcdebug to either stderr or stdout depending on if any other output options are used or not, but that may be more confusing than beneficial. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/216 -- Commit Summary -- * Do not call rpmfcPrint() with an uninitialized buffer in rpmdeps * Send the output from rpmfcPrint() to stdout in rpmdeps -- File Changes -- M tools/rpmdeps.c (2) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/216.patch https://github.com/rpm-software-management/rpm/pull/216.diff -- 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/216 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
