Re: [PATCH v2 7/8] Use posix_spawn to run external scripts
Pawel Kot <[email protected]> Fri, 24 Jan 2020 09:48:25 +0100
| Newsgroups | gmane.linux.drivers.gnokii |
|---|---|
| Message-ID | <CAHytCw5veT90TYfghjRF-xTBtuRW6ia10Y4_WHiKp86stTbwGg@mail.gmail.com> |
--===============7885160134340257968== Content-Type: multipart/alternative; boundary="000000000000bf23b0059cded283" --000000000000bf23b0059cded283 Content-Type: text/plain; charset="UTF-8" Hi, On Thu, Jan 23, 2020 at 2:02 AM Ladislav Michl <[email protected]> wrote: > > On Mon, Jan 20, 2020 at 01:07:35PM +0100, Pawel Kot wrote: > > On Mon, Jan 20, 2020 at 9:40 AM Ladislav Michl <[email protected]> wrote: > > > ... 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) > > > > Yes, if we're going to change that let's do it in a proper way. Happy to > > discuss on IRC sometime. > > Your IRC connection is unstable beyond being usefull for any discussion ;-) I'm travelling a lot during a week. Weekends are more peaceful :) > So here is a quick draft, patch is based on another unreleased one moving > device_script into separate file. Please note it is only a draft, compile > tested only (assumes C99 compiler). I tried to avoid device driver changes, > but patch is still a bit hard to read. Apologies for that. However, it > should suffice as a beginning of a discussion. I'll polish it a bit more > based on feedback received and send as separate patch serie. Looks good. Some minor stuff below. > +const static gn_device_ops _bluetooth_ops = { > + .open = _bluetooth_open, Why just open with _ prefix? Overall question for other places as well. > + .close = bluetooth_close, > + .select = bluetooth_select, > + .read = bluetooth_read, > + .write = bluetooth_write, > +}; > gn_error device_nreceived(int *n, struct gn_statemachine *state) [...] > + return state->device.ops->nrcvd(state->device.fd, n, state); I would leave naming consistent like nreceived everywhere. It applies not just here. > +dnl ======================== Defines location for gettext > +AC_ARG_WITH(gettext, > + [ --with-gettext=DIR specifies the base gettext], > + [ if test x$withval = xyes; then > + AC_MSG_WARN(Usage is: --with-gettext=DIR) > + else > + CFLAGS="$CFLAGS -I$withval" > + fi > + ] > +) > + Hm? > +dnl ======================== Checks for gethostbyname support > +AC_CHECK_FUNC(gethostbyname, , > + AC_CHECK_LIB(nsl, gethostbyname, TCP_LIBS="-lnsl" > + AC_SUBST(TCP_LIBS))) > +dnl Haiku requires -lnetwork for socket functions > +AC_CHECK_FUNC(gethostbyname, , > + AC_CHECK_LIB(network, gethostbyname, TCP_LIBS="-lnetwork" > + AC_SUBST(TCP_LIBS))) > + Hm? > -if test "$enable_phonet" = "yes"; then > - AC_CHECK_HEADER(linux/phonet.h, > - [AC_DEFINE(HAVE_SOCKETPHONET, 1, [Whether Phonet is available]) > - USE_SOCKETPHONET="yes"],, > - [#include <sys/socket.h> > - #include <linux/phonet.h>]) > -fi Why? > -dnl ======================== Checks for gethostbyname support > -AC_CHECK_FUNC(gethostbyname, , > - AC_CHECK_LIB(nsl, gethostbyname, TCP_LIBS="-lnsl" > - AC_SUBST(TCP_LIBS))) > -dnl Haiku requires -lnetwork for socket functions > -AC_CHECK_FUNC(gethostbyname, , > - AC_CHECK_LIB(network, gethostbyname, TCP_LIBS="-lnetwork" > - AC_SUBST(TCP_LIBS))) > - OK, so this part is just being moved... > -if test "$enable_irda" = "yes"; then > - AC_CHECK_HEADER(linux/irda.h, > - [AC_DEFINE(HAVE_IRDA, 1, [Whether IrDA is available]) > - USE_IRDA="yes"],, > - [#include <sys/socket.h> > - #include <sys/ioctl.h> > - #include <linux/types.h>]) > -fi Why? > -dnl ======================== Defines location for gettext > -AC_ARG_WITH(gettext, > - [ --with-gettext=DIR specifies the base gettext], > - [ if test x$withval = xyes; then > - AC_MSG_WARN(Usage is: --with-gettext=DIR) > - else > - CFLAGS="$CFLAGS -I$withval" > - fi > - ] > -) OK, so this part is just being moved. > -void device_reset(struct gn_statemachine *state); Why? > typedef struct { > int fd; > - gn_connection_type type; Won't we need it anymore? > + const gn_device_ops *ops; > void *device_instance; > } gn_device; Cheers, -- Pawel Kot --000000000000bf23b0059cded283 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Hi,<br><br>On Thu, Jan 23, 2020 at 2:02 AM Ladislav Michl = <<a href=3D"mailto:[email protected]">[email protected]</a>> wr= ote:<br>><br>> On Mon, Jan 20, 2020 at 01:07:35PM +0100, Pawel Kot wr= ote:<br>> > On Mon, Jan 20, 2020 at 9:40 AM Ladislav Michl <<a hre= f=3D"mailto:[email protected]">[email protected]</a>> wrote:<br>&g= t; > > ... Alternatively I can turn it into proper device plugin arch= itecture.<br>> > > Is that your preferred way to go? (while doing = that I would also add<br>> > > support for external even loops)<br= >> ><br>> > Yes, if we're going to change that let's do= it in a proper way. Happy to<br>> > discuss on IRC sometime.<br>>= <br>> Your IRC connection is unstable beyond being usefull for any discu= ssion ;-)<div><br></div><div>I'm travelling a lot during a week. Weeken= ds are more peaceful :)</div><div><br>> So here is a quick draft, patch = is based on another unreleased one moving<br>> device_script into separa= te file. Please note it is only a draft, compile<br>> tested only (assum= es C99 compiler). I tried to avoid device driver changes,<br>> but patch= is still a bit hard to read. Apologies for that. However, it<br>> shoul= d suffice as a beginning of a discussion. I'll polish it a bit more<br>= > based on feedback received and send as separate patch serie.<br><br>Lo= oks good. Some minor stuff below.</div><div><br>> +const static gn_devic= e_ops _bluetooth_ops =3D {<br>> + =C2=A0 =C2=A0 =C2=A0 .open =C2=A0 =3D = _bluetooth_open,</div><div><br></div><div>Why just open with _ prefix? Over= all question for other places as well.</div><div><br>> + =C2=A0 =C2=A0 = =C2=A0 .close =C2=A0=3D bluetooth_close,<br>> + =C2=A0 =C2=A0 =C2=A0 .se= lect =3D bluetooth_select,<br>> + =C2=A0 =C2=A0 =C2=A0 .read =C2=A0 =3D = bluetooth_read,<br>> + =C2=A0 =C2=A0 =C2=A0 .write =C2=A0=3D bluetooth_w= rite,<br>> +};</div><div><br>> =C2=A0gn_error device_nreceived(int *n= , struct gn_statemachine *state)</div><div>[...]<br>> + =C2=A0 =C2=A0 = =C2=A0 return state->device.ops->nrcvd(state->device.fd, n, state)= ;</div><div><br></div><div>I would leave naming consistent like nreceived e= verywhere. It applies not just here.</div><div><br>> +dnl =3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Defines locati= on for gettext<br>> +AC_ARG_WITH(gettext,<br>> + =C2=A0 =C2=A0 =C2=A0= [ =C2=A0--with-gettext=3DDIR =C2=A0 =C2=A0 =C2=A0specifies the base gettex= t],<br>> + =C2=A0 =C2=A0 =C2=A0 [ if test x$withval =3D xyes; then<br>&g= t; + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 AC_MSG_WARN(Usage is:= --with-gettext=3DDIR)<br>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 else<br>> += =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CFLAGS=3D"$CFLAGS -I= $withval"<br>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 fi<br>> + =C2=A0 = =C2=A0 =C2=A0 ]<br>> +)<br>> +</div><div><br></div><div>Hm?</div><div= ><br>> +dnl =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D Checks for gethostbyname support<br>> +AC_CHECK_FUNC(gethos= tbyname, ,<br>> + =C2=A0 =C2=A0 =C2=A0 AC_CHECK_LIB(nsl, gethostbyname, = TCP_LIBS=3D"-lnsl"<br>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0AC_SUBST(TCP_LIBS)))<br>> +dnl Haiku r= equires -lnetwork for socket functions<br>> +AC_CHECK_FUNC(gethostbyname= , ,<br>> + =C2=A0 =C2=A0 =C2=A0 AC_CHECK_LIB(network, gethostbyname, TCP= _LIBS=3D"-lnetwork"<br>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0AC_SUBST(TCP_LIBS)))<br>> +</div><div>= <br></div><div>Hm?</div><div><br>> -if test "$enable_phonet" = =3D "yes"; then<br>> - =C2=A0 =C2=A0 =C2=A0 AC_CHECK_HEADER(li= nux/phonet.h,<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 [A= C_DEFINE(HAVE_SOCKETPHONET, 1, [Whether Phonet is available])<br>> - =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0USE_SOCKETPHONET=3D&quo= t;yes"],,<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 [= #include <sys/socket.h><br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0#include <linux/phonet.h>])<br>> -fi<br><br>Wh= y?</div><div><br>> -dnl =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D Checks for gethostbyname support<br>> -AC_CHECK= _FUNC(gethostbyname, ,<br>> - =C2=A0 =C2=A0 =C2=A0 AC_CHECK_LIB(nsl, get= hostbyname, TCP_LIBS=3D"-lnsl"<br>> - =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0AC_SUBST(TCP_LIBS)))<br>> -= dnl Haiku requires -lnetwork for socket functions<br>> -AC_CHECK_FUNC(ge= thostbyname, ,<br>> - =C2=A0 =C2=A0 =C2=A0 AC_CHECK_LIB(network, gethost= byname, TCP_LIBS=3D"-lnetwork"<br>> - =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0AC_SUBST(TCP_LIBS)))<br>> -= </div><div><br></div><div>OK, so this part is just being moved...</div><div= ><br>> -if test "$enable_irda" =3D "yes"; then<br>&g= t; - =C2=A0 =C2=A0 =C2=A0 AC_CHECK_HEADER(linux/irda.h,<br>> - =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 [AC_DEFINE(HAVE_IRDA, 1, [Whether= IrDA is available])<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0USE_IRDA=3D"yes"],,<br>> - =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 [#include <sys/socket.h><br>> - =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0#include <sys/ioctl.h&g= t;<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0#includ= e <linux/types.h>])<br>> -fi<br><br>Why?</div><div><br>> -dnl = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D De= fines location for gettext<br>> -AC_ARG_WITH(gettext,<br>> - =C2=A0 = =C2=A0 =C2=A0 [ =C2=A0--with-gettext=3DDIR =C2=A0 =C2=A0 =C2=A0specifies th= e base gettext],<br>> - =C2=A0 =C2=A0 =C2=A0 [ if test x$withval =3D xye= s; then<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 AC_MSG_W= ARN(Usage is: --with-gettext=3DDIR)<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 e= lse<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CFLAGS=3D&qu= ot;$CFLAGS -I$withval"<br>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 fi<br>>= ; - =C2=A0 =C2=A0 =C2=A0 ]<br>> -)<br><br>OK, so this part is just being= moved.</div><div><br>> -void device_reset(struct gn_statemachine *state= );</div><div><br></div><div>Why?</div><div><br>> =C2=A0typedef struct {<= br>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 int fd;<br>> - =C2=A0 =C2=A0 =C2=A0 = gn_connection_type type;</div><div><br></div><div>Won't we need it anym= ore?</div><div><br>> + =C2=A0 =C2=A0 =C2=A0 const gn_device_ops *ops;<br= >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 void *device_instance;<br>> =C2=A0} gn= _device;<br><br>Cheers,</div><div>--<br>Pawel Kot</div></div> --000000000000bf23b0059cded283-- --===============7885160134340257968== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZ25va2lpLXVz ZXJzIG1haWxpbmcgbGlzdApnbm9raWktdXNlcnNAbm9uZ251Lm9yZwpodHRwczovL2xpc3RzLm5v bmdudS5vcmcvbWFpbG1hbi9saXN0aW5mby9nbm9raWktdXNlcnMK --===============7885160134340257968==--