Re: [PATCH v2 7/8] Use posix_spawn to run external scripts

[Ladislav Michl]

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 <address@hidden> 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.

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 :)

Also I do not remember when I used fork/exec instead of posix_spawn last
time - the later is just much neater API, not mentioning performance.
You can follow similar discussion here:
https://github.com/golang/go/issues/5838

So feel free to drop this patch, it can be digged from the mail archive
once someone runs into performace issues (most likely setups doing
device_open for every operation, such as running gnokii from scripts).

> 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 way
to go? (while doing that I would also add support for external even loops)

> Gonna give it try (more as in compiling in various setups) and if it works,
> I'll merge it to a master. Let me know if you'll find any issues with this
> version.

Seems good for me and works for me on Linux. I'll give it a try on Win32
and MacOS over the week.

Thank you,
        ladis