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

[Ladislav Michl]

On Fri, Jan 24, 2020 at 09:48:25AM +0100, Pawel Kot wrote:
> Hi,
> 
> On Thu, Jan 23, 2020 at 2:02 AM Ladislav Michl <address@hidden> 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 <address@hidden>
> 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.

It would be more convenient to have all device functions with the same
prototype. It is not the case (yet) and I wanted to have plugin change
isolated without touching device drivers.

So I just introduced _ prefixed variants to keep change in one place.
Later we can unify driver functions to use the same prototype and
this will go away. So it can have any name, feel free to suggest
something better.

> > +       .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.

It was there, but it is too long to have device_ops above nicely aligned.
This way all ops names are about the same lenght. If you prefer longer
variant, let's change that.

> > +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?

See bellow for all Hm and Why part.

> > -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.

Snipets are shuffled around to have all device checks in one place
and the rest at another. No problem dropping that change or having
separate patch for it.

> > -void device_reset(struct gn_statemachine *state);
> 
> Why?

This function is not used anywhere. Perhaps base device change on some
preparation cleanup patches would make you more happy :)

> >  typedef struct {
> >         int fd;
> > -       gn_connection_type type;
> 
> Won't we need it anymore?

No, we have device_ops now. Also it would allow us to remove all stuff like
(fbus_serial_open):
        if (state->config.connection_type == GN_CT_TCP)
                type = GN_CT_TCP;
        else
                type = GN_CT_Serial;
as we handle all devices consistently now.

> > +       const gn_device_ops *ops;
> >         void *device_instance;
> >  } gn_device;
> 
> Cheers,
> --
> Pawel Kot

> _______________________________________________
> gnokii-users mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/gnokii-users