On 12.10.2016 19:49, Christopher Larson wrote: > > On Wed, Oct 12, 2016 at 12:30 AM, Andreas Oberritter > <o...@opendreambox.org <mailto:o...@opendreambox.org>> wrote: > > On 11.10.2016 21:34, Christopher Larson wrote: > > > > On Tue, Oct 11, 2016 at 12:03 PM, Jagadeesh Krishnanjanappa > > <jkrishnanjana...@mvista.com <mailto:jkrishnanjana...@mvista.com> > <mailto:jkrishnanjana...@mvista.com > <mailto:jkrishnanjana...@mvista.com>>> wrote: > > > > On Tue, Oct 11, 2016 at 9:06 PM, Christopher Larson > > <clar...@kergoth.com <mailto:clar...@kergoth.com> > <mailto:clar...@kergoth.com <mailto:clar...@kergoth.com>>> wrote: > > > > > > On Tue, Oct 11, 2016 at 5:11 AM, Jagadeesh Krishnanjanappa > > <jkrishnanjana...@mvista.com > <mailto:jkrishnanjana...@mvista.com> > > <mailto:jkrishnanjana...@mvista.com > <mailto:jkrishnanjana...@mvista.com>>> > wrote: > > > > Thanks for reviewing the patch. > > > > > > I think this is too fragile to land in OE-Core. What > > happens if a > > network driver prints "device=eth0"? Or if other kernel > > messages make > > the string disappear from dmesg and connman gets > > restarted later? > > > > > > True. If device=eth0 gets disappeared/corrupted, then we may > > have problem. > > > > > > FWIW, I'm attaching my current wrapper script for > > connmand. I don't > > think it's perfect, but at least it doesn't depend on > > any init manager > > and works across restarts. > > > > The wrapper script attached by you, takes care of the > > missing scenarios. Seems to be more complete. > > > > > > > > Add these lines to connman's do_install, in case you'd > > like to test > > and/or submit it: > > > > mv ${D}${sbindir}/connmand ${D}${sbindir}/connmand.real > > install -m 755 ${WORKDIR}/connmand-nfsroot.in > <http://connmand-nfsroot.in> > > <http://connmand-nfsroot.in> ${D}${sbindir}/connmand > > sed -e 's,@sbindir@,${sbindir},g' -i > ${D}${sbindir}/connmand > > > > I think it would be good idea to integrate your changes into > > the already existing OE-core's connman script, instead of a > > calling original connman script from the wrapper script. > > > > > > I threw > > together > https://github.com/openembedded/openembedded-core/compare/master...kergoth:connman-systemd-nfs > > <https://github.com/openembedded/openembedded-core/compare/master...kergoth:connman-systemd-nfs> > > > <https://github.com/openembedded/openembedded-core/compare/master...kergoth:connman-systemd-nfs > > <https://github.com/openembedded/openembedded-core/compare/master...kergoth:connman-systemd-nfs>> > last > > night, only limited testing. I think using a script in > > libexecdir is rather cleaner than wrapping connmand in place, > > for something like this, personally. > > > > > > In the above commit, the last line of connman/connman/start-connman > > file, should be exec @SBINDIR@/connmand "$@" instead of exec > > @SBINDIR@/connman "$@" ; as the actual binary file name is connmand. > > > > > > Thanks, didn’t notice that. Obviously it needs polish and testing before > > submission, I mainly linked it to solicit thoughts on the approach :) > > your proposal adds some complexity to the recipe, which I tried to > avoid, because this wrapper script is something we'll probably have to > maintain forever. > > For example, your recipe might break without notice if upstream is going > to ship another binary as ${sbindir}/connmand-something in the future; > or if upstream decides to change the systemd unit to use a variable > instead of a hardcoded path to connmand. > > Besides that, I don't have a strong opinion on the location of the > wrapper script. > > > Good points. I think the future-proofing could be addressed with > something like sed -i -e "s#^ExecStart=.*#ExecStart=$wrapperpath#" > ${D}${systemd_unitdir}/system/connman.service, and then switch away from > that weak start-connman script in favor of yours, but keep it in libexec > for cleanliness.
Agreed. Regards, Andreas -- _______________________________________________ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
