Re: [PATCH v2 7/8] Use posix_spawn to run external scripts
Pawel Kot <[email protected]> Mon, 20 Jan 2020 13:07:35 +0100
| Newsgroups | gmane.linux.drivers.gnokii |
|---|---|
| Message-ID | <CAHytCw6nN+W0JRFqDJ+N9-tUi6=OhYg04vui31Vu4=8uD8gQqg@mail.gmail.com> |
--===============1819219278380479707== Content-Type: multipart/alternative; boundary="000000000000bbc8d9059c91239f" --000000000000bbc8d9059c91239f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jan 20, 2020 at 9:40 AM Ladislav Michl <[email protected]> wrote= : > > On Mon, Jan 20, 2020 at 12:03:30AM +0100, Pawel Kot wrote: > > Hi, > > > > On Tue, Dec 4, 2018 at 10:32 PM Ladislav Michl <[email protected]> wrote: > > > posix_spawn specification dates back to last century and its > > > implementation is mature enough in all systems we do support. > > > Thus use it instead of current fork and exec in hope it will > > > save us some resources. > > > > So I think this one does more than described. My understanding is that now > > you can pass env variables into a script. Is that correct? And what was > > wrong with traditional fork/exec approach? I mean in our particular case. > > Did you run into oom in some setups? > > Even current implementation is able to set environment for a script and there > is nothing wrong with traditional fork/exec approach, except this is one of > the worst unix apis ever seen ;-) Also that FIXME in original code could be > fixed using O_CLOEXEC where available. .. but a classic one. :) > So, I do not insist on this patch as it is, but having device_script as > separate platform specific function is still usefull, just because later > we can implement it using CreateProcess Win32 API function (therefore > not requiring cygwin). Here it is done in single patch to indicate > with posixscript.c name that it is expected to work on every POSIX.1-2001 > conformant system. It is expected CreateProcess version to be named > as win32script.c :) Fair enough. Let me give another look. > > I've merged all other patches except this one (and one related) and devices > > build refactor (which I do not like in this form) into github. > > Any particular issue with that except that one you mentioned earlier? I mean > #else > int fbusdku2usb_open(struct gn_statemachine *state) > { > return -1; > } > ...etc, it header files? This one I did to improve modularity and it does its > job pretty well without touching too much core code. Alternatively I can > turn it into proper device plugin architecture. Is that your preferred wa= y > to go? (while doing that I would also add support for external even loops= ) Yes, if we're going to change that let's do it in a proper way. Happy to discuss on IRC sometime. > Seems good for me and works for me on Linux. I'll give it a try on Win32 > and MacOS over the week. Seems to compile on Mac. Will give a whirl on Windows during next weekend. Cheers, Pawe=C5=82 -- Pawel Kot --000000000000bbc8d9059c91239f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><br><br>On Mon, Jan 20, 2020 at 9:40 AM Ladislav Michl <= ;<a href=3D"mailto:[email protected]">[email protected]</a>> wrote= :<br>><br>> On Mon, Jan 20, 2020 at 12:03:30AM +0100, Pawel Kot wrote= :<br>> > Hi,<br>> ><br>> > On Tue, Dec 4, 2018 at 10:32 P= M Ladislav Michl <<a href=3D"mailto:[email protected]">ladis@linux-mi= ps.org</a>> wrote:<br>> > > posix_spawn specification dates bac= k to last century and its<br>> > > implementation is mature enough= in all systems we do support.<br>> > > Thus use it instead of cur= rent fork and exec in hope it will<br>> > > save us some resources= .<br>> ><br>> > So I think this one does more than described. M= y understanding is that now<br>> > you can pass env variables into a = script. Is that correct? And what was<br>> > wrong with traditional f= ork/exec approach? I mean in our particular case.<br>> > Did you run = into oom in some setups?<br>><br>> Even current implementation is abl= e to set environment for a script and there<br>> is nothing wrong with t= raditional fork/exec approach, except this is one of<br>> the worst unix= apis ever seen ;-) Also that FIXME in original code could be<br>> fixed= using O_CLOEXEC where available.<div><br></div><div>.. but a classic one. = :)</div><div><br>> So, I do not insist on this patch as it is, but havin= g device_script as<br>> separate platform specific function is still use= full, just because later<br>> we can implement it using CreateProcess Wi= n32 API function (therefore<br>> not requiring cygwin). Here it is done = in single patch to indicate<br>> with posixscript.c name that it is expe= cted to work on every POSIX.1-2001<br>> conformant system. It is expecte= d CreateProcess version to be named<br>> as win32script.c :)<br><br>Fair= enough. Let me give another look.</div><div><br>> > I've merged = all other patches except this one (and one related) and devices<br>> >= ; build refactor (which I do not like in this form) into github.<br>><br= >> Any particular issue with that except that one you mentioned earlier?= I mean<br>> #else<br>> int fbusdku2usb_open(struct gn_statemachine *= state)<br>> {<br>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;<br>> }<b= r>> ...etc, it header files? This one I did to improve modularity and it= does its<br>> job pretty well without touching too much core code. Alte= rnatively I can<br>> turn it into proper device plugin architecture. Is = that your preferred way<br>> to go? (while doing that I would also add s= upport for external even loops)<br><br>Yes, if we're going to change th= at let's do it in a proper way. Happy to discuss on IRC sometime.</div>= <div><br></div><div>> Seems good for me and works for me on Linux. I'= ;ll give it a try on Win32<br>> and MacOS over the week.</div><div><br><= /div><div>Seems to compile on Mac. Will give a whirl on Windows during next= weekend.</div><div><br>Cheers,</div><div>Pawe=C5=82<br>--<br>Pawel Kot</di= v></div> --000000000000bbc8d9059c91239f-- --===============1819219278380479707== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZ25va2lpLXVz ZXJzIG1haWxpbmcgbGlzdApnbm9raWktdXNlcnNAbm9uZ251Lm9yZwpodHRwczovL2xpc3RzLm5v bmdudS5vcmcvbWFpbG1hbi9saXN0aW5mby9nbm9raWktdXNlcnMK --===============1819219278380479707==--