Re: [PATCH v2 7/8] Use posix_spawn to run external scripts
Pawel Kot <[email protected]> Fri, 24 Jan 2020 10:14:37 +0100
| Newsgroups | gmane.linux.drivers.gnokii |
|---|---|
| Message-ID | <CAHytCw7gP34tPNB1gpVLo7-oxq28ggoKNf0kUcZR7qWKVAr3qA@mail.gmail.com> |
--===============3806524595566462674== Content-Type: multipart/alternative; boundary="0000000000007fb132059cdf3010" --0000000000007fb132059cdf3010 Content-Type: text/plain; charset="UTF-8" Hi, On Fri, Jan 24, 2020 at 10:07 AM Ladislav Michl <[email protected]> wrote: > On Fri, Jan 24, 2020 at 09:48:25AM +0100, Pawel Kot wrote: > > On Thu, Jan 23, 2020 at 2:02 AM Ladislav Michl <[email protected]> wrote: > > > +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. Fair enough, but let me review it over the weekend more thoroughly. > > > + .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. I do not mind longer or shorter. I would only prefer have the same naming in function names and in structure fields. > > 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. Separate patch would be nice. > > > -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 :) OK. > > > 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. Sounds good, thanks. Cheers, -- Pawel Kot --0000000000007fb132059cdf3010 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Hi,<br><br>On Fri, Jan 24, 2020 at 10:07 AM Ladislav Michl= <<a href=3D"mailto:[email protected]">[email protected]</a>> w= rote:<br>> On Fri, Jan 24, 2020 at 09:48:25AM +0100, Pawel Kot wrote:<br= >> > On Thu, Jan 23, 2020 at 2:02 AM Ladislav Michl <<a href=3D"ma= ilto:[email protected]">[email protected]</a>> wrote:<br>> >= > +const static gn_device_ops _bluetooth_ops =3D {<br>> > > + = =C2=A0 =C2=A0 =C2=A0 .open =C2=A0 =3D _bluetooth_open,<br>> ><br>>= > Why just open with _ prefix? Overall question for other places as wel= l.<br>><br>> It would be more convenient to have all device functions= with the same<br>> prototype. It is not the case (yet) and I wanted to = have plugin change<br>> isolated without touching device drivers.<br>>= ;<br>> So I just introduced _ prefixed variants to keep change in one pl= ace.<br>> Later we can unify driver functions to use the same prototype = and<br>> this will go away. So it can have any name, feel free to sugges= t<br>> something better.<br><br>Fair enough, but let me review it over t= he weekend more thoroughly.<div><br>> > > + =C2=A0 =C2=A0 =C2=A0 .= close =C2=A0=3D bluetooth_close,<br>> > > + =C2=A0 =C2=A0 =C2=A0 .= select =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_write,<br>> > > +};<br>> ><br>> >= > =C2=A0gn_error device_nreceived(int *n, struct gn_statemachine *state= )<br>> > [...]<br>> > > + =C2=A0 =C2=A0 =C2=A0 return state-= >device.ops->nrcvd(state->device.fd, n, state);<br>> ><br>&g= t; > I would leave naming consistent like nreceived everywhere. It appli= es not<br>> > just here.<br>><br>> It was there, but it is too = long to have device_ops above nicely aligned.<br>> This way all ops name= s are about the same lenght. If you prefer longer<br>> variant, let'= s change that.<br><br>I do not mind longer or shorter. I would only prefer = have the same naming in function names and in structure fields.</div><div><= br>> > OK, so this part is just being moved.<br>><br>> Snipets = are shuffled around to have all device checks in one place<br>> and the = rest at another. No problem dropping that change or having<br>> separate= patch for it.<br><br>Separate patch would be nice.</div><div><br>> >= > -void device_reset(struct gn_statemachine *state);<br>> ><br>&g= t; > Why?<br>><br>> This function is not used anywhere. Perhaps ba= se device change on some<br>> preparation cleanup patches would make you= more happy :)<br><br>OK.</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;<br>> ><br>> > W= on't we need it anymore?<br>><br>> No, we have device_ops now. Al= so it would allow us to remove all stuff like<br>> (fbus_serial_open):<b= r>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (state->config.connection_type =3D= =3D GN_CT_TCP)<br>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 type =3D GN_CT_TCP;<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 =C2=A0 type =3D GN_CT_Seri= al;<br>> as we handle all devices consistently now.<br><br>Sounds good, = thanks.<br><br>Cheers,</div><div>--<br>Pawel Kot</div></div> --0000000000007fb132059cdf3010-- --===============3806524595566462674== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZ25va2lpLXVz ZXJzIG1haWxpbmcgbGlzdApnbm9raWktdXNlcnNAbm9uZ251Lm9yZwpodHRwczovL2xpc3RzLm5v bmdudS5vcmcvbWFpbG1hbi9saXN0aW5mby9nbm9raWktdXNlcnMK --===============3806524595566462674==--