Hi Felix, Thank you for the thorough review!
Following up on our discussion at #openwrt-devel, I have opened up our SVN for reading. Maybe it would indeed be better to keep it as a separate feed on our server, referenced from OpenWrt; we are planning several other packages related to each other and it would be less of a hassle for everyone if we did the maintenance ourselves. Nonetheless, your remarks are quite valid. Let me reflect to them one by one: Felix Fietkau wrote: >> Index: net/epoint/files/CONTROL/postinst >> =================================================================== >> --- net/epoint/files/CONTROL/postinst (revision 0) >> +++ net/epoint/files/CONTROL/postinst (revision 0) >> @@ -0,0 +1,57 @@ >> [...] > preinst, postinst, prerm, conffiles, etc. files should be defined inline > in the package makefile using something like this: > > define Package/<name>/postinst > [...] > endef Interesting. Could you point me to some documentation where this is explained or give a brief description why is that needed? I am not questioning it, just wondering about how it works. >> + >> +grep -q epoint $IPKG_INSTROOT/etc/services || \ >> + echo 'epoint 8080/tcp' >> $IPKG_INSTROOT/etc/services >> + >> +if [ -n "$IPKG_INSTROOT" ]; then >> + touch $IPKG_INSTROOT/etc/epoint_firstboot >> + >> + CONF="$IPKG_INSTROOT/etc/opkg.conf" >> + # XXX This is ugly hack. We should force offline installation of opkg >> + # to happen before us, or fetch and modify opkg.conf somehow. >> + if [ -f "$CONF" ]; then >> + grep -q epoint "$CONF" || \ >> + echo 'src epoint http://www.epointsystem.org/openwrt/packages' >> >> >> "$CONF" >> + else >> + cat > "$CONF" <<EOF >> +src/gz snapshots >> http://downloads.openwrt.org/kamikaze/8.09.1/brcm-2.4/packages >> +src epoint http://www.epointsystem.org/openwrt/packages >> +dest root / >> +dest ram /tmp >> +lists_dir ext /var/opkg-lists >> +option overlay_root /jffs >> +EOF >> + fi >> + >> + mkdir -p $IPKG_INSTROOT/etc/uci-defaults >> + cat > $IPKG_INSTROOT/etc/uci-defaults/epoint <<DATA >> +#!/bin/sh >> + >> +uci batch <<-EOF >> + set luci.main.mediaurlbase=/luci-static/epoint >> + commit >> +EOF >> +DATA >> + chmod 755 $IPKG_INSTROOT/etc/uci-defaults/epoint >> +fi >> + >> +if [ -z "$IPKG_INSTROOT" ]; then >> + . /etc/functions.sh >> + for m in /etc/modules.d/*ipt*; do >> + load_modules `basename $m` >> + done >> + >> + rm -f /tmp/luci-indexcache >> + rm -f /tmp/luci-modulecache/* >> + rm -rf /tmp/luci-templatecache/* >> + >> + /etc/init.d/xinetd enable >> + /etc/init.d/xinetd restart >> + /etc/init.d/matrixhttps enable >> + /etc/init.d/matrixhttps start >> + /etc/init.d/epoint enable >> + /etc/init.d/epoint start >> + >> + ACTION=ifup /etc/hotplug.d/iface/40-hotspot-dns >> +fi >> +exit 0 > Some of this is probably easier to handle by installing a script into > /etc/uci-defaults, which will make it run on first boot only (or on the > next reboot if you're installing it on the device). > You could then also trigger the uci-defaults apply in the postinst > script of the package by calling . /etc/functions.sh; uci_apply_defaults Yes, that would probably be cleaner. Could you point to some other package that changes configuration in a similar manner? >> Index: net/epoint/files/CONTROL/prerm >> =================================================================== >> --- net/epoint/files/CONTROL/prerm (revision 0) >> +++ net/epoint/files/CONTROL/prerm (revision 0) >> @@ -0,0 +1,8 @@ >> [...] > See above. > >> Index: net/epoint/files/www/epoint-static/epoint.png >> =================================================================== >> Cannot display: file marked as a binary type. >> svn:mime-type = application/octet-stream > This will have to be handled separately once we've decided whether we > will merge this package into OpenWrt, or you guys maintain a separate > feed for your packages. After some pondering, I am inclined to have us maintain a separate public feed. It seems to be easier for everyone involved. The only coordination that we need to do in this case is making sure that in OpenWrt releases, the source of our feed is not the trunk of our SVN but a tag that stays unchanged so that a well-tested release is not broken by us changing something in our trunk. Our latest public release is at https://www.epointsystem.org/svn/vending_machine/hotspot/tags/hotspot12/ >> Index: net/epoint/files/etc/ssl/privkey.pem >> Index: net/epoint/files/etc/ssl/cert.pem > You may want to generate these either on the device or at least on the > host, if you're using these in a place where security matters. I am not. These are for an SSL certificate, so nothing is encrypted using this key; it merely signs the ephemeral D-H key during the SSL handshake, when accessing LuCI via https. This signature does not secure anything, as it is not a certified site, just an encrypted http connection. On the other hand, for usability reasons it is beneficial that in every instance the certificate is the same. Otherwise, if one maintains several routers, a lot of non-trivial, time-consuming certificate manipulations are needed before the same web browser will be willing to connect to an https site with the same URL but a different certificate that is not issued by one of those trusted authorities. > You should use $(TAR) instead of tar to keep it more portable. Okay. Regards, -- Daniel
signature.asc
Description: OpenPGP digital signature
_______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
