Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
Missed the part about moving to rpmio/rpmlua.c... anyway, been long enough - I adjusted that part myself and pushed as commit 18e792340e33eade01967d01d5801f1ae9e0ad33 (and cb2ae4bdf2f60876fdc68e3f84938e9c37182fab). Thanks for the patches! -- 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/390#issuecomment-394317385___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
Closed #390. -- 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/390#event-1661014629___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
I've rebased this and added use of the function from #444. Please review. -- 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/390#issuecomment-393193758___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
Once #444 lands, this should be updated to use it and move the code to rpmio/rpmlua.c and then it'd probably be good to go. -- 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/390#issuecomment-392508731___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
@ignatenkobrain , that's missing the point entirely. It's all the already open descriptors that we need to take care of, including whatever that might be opened by dnf and all the other API consumers. -- 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/390#issuecomment-367632254___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
@pmatilai I don't think that I open file descriptors by hand in this function, so there is nothing to close.. -- 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/390#issuecomment-367604224___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
The real flaw with looping over close(fdno) before exec in rpm is that it masks the root cause of the problem: applications that are not setting FD_CLOEXEC. The loop also prevents an application from intentionally passing an open fdno to a child of rpm (if that is/was intended). I'd suggest removing the loop entirely (or logging leaky fdnos) so that applications can be fixed directly: it's not a difficult patch in most cases. -- 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/390#issuecomment-367384963___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
BTW, wrt closing file descriptors, we can't go adding copy-paste versions of that all over the place, two places is one too many already. One possibility might be having a central posix_atfork() handler to take care of it. Central handling is also necessary to do something about https://bugzilla.redhat.com/show_bug.cgi?id=1537564 - on Linux we could look into /proc/self/fd instead of brute-forcing everything but it doesn't exactly simplify the code so copy-paste is out of the question. -- 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/390#issuecomment-367362028___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
Don't consider this an actual review, but two things that occurred to me: - this needs to close open file descriptors (see eg commits 7a7c31f551ff167f8718aea6d5048f6288d60205 and aa9a791d808f504781d0b75255df3387383a1809) - technically this belongs to rpmio/ side because it does not depend on anything in librpm -- 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/390#issuecomment-367358767___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
@pmatilai in any case, any rebase / changes in posix module in rpm/lua doesn't help here. I just want function which executes process without using shell which would be very simple.. So this is what this PR is about. Could you review it please? ;) -- 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/390#issuecomment-364074828___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
Yes there are a couple of hacks to lposix which means we can't just switch to
upstream, rebasing to newer one is an entirely another thing, and was under
discussion last year already (there's a ticket around here somewhere).
The other thing is that there are other ways around eg the exec() thing, the
current implementation was just a quick hack to get something in there - people
were discovering -p %{lua} and the posix extension and getting themselves *and*
rpm in trouble with it.
One kinda indirect solution would always executing -p %{lua} scriptlets in a
forked process, at which point there's no need for the check in the first
place. This would also allow feeding paths into file triggers the same way
regardless of interpreter, and would protect rpm from scriptlets doing stupid
things with macros *and* packages from one another. And no doubt it would break
somebody's carefully crafted scripts which rely on the fact that they execute
in-process, but I don't have too much sympathy over that.
--
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/390#issuecomment-364049793___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
I wonder if @n3npq's idea of rebasing the luaposix we have included in RPM is a better idea... Both the `posix` and `rexlib` modules were copied into the source tree a long time ago, maybe rebasing is a better solution? @pmatilai what do you think? -- 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/390#issuecomment-363982013___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
@Conan-Kudo no, they don't use malloc, they use `lua_newuserdata` which is managed by Lua itself IIUC. -- 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/390#issuecomment-363467906___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
@ignatenkobrain Is the memory leak fix needed in upstream [luaposix](https://github.com/luaposix/luaposix)? -- 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/390#issuecomment-363464650___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] lua: add rpm.execute() (#390)
Alright, moved under RPM namespace and renamed to `execute()` which better aligns with what function really does. -- 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/390#issuecomment-363463542___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
