[Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

[Peter Kjellerstedt]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20180517/884b2634/attachment.html>