| Message ID | CAPOFwxopE6DvRWF0uDimpS0HPmJ_jxt-0EKonkvC5b6xVnzpyQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 06/03/12 21:52, David Purdy wrote: > p910nd is a lightweight, nonspooling printserver that accepts jobs > on ports 9100-9102 via network, and sends them directly to a USB > printer. It is ideally suited for embedded devices and diskless > workstations. Please include a Signed-off-by line for yourself. This is a short way for you to assert that you are entitled to contribute the patch under buildroot's GPL license. See http://kerneltrap.org/files/Jeremy/DCO.txt for more details. The rest of my comments are a bit of nitpicking. [snip] > diff --git a/package/p910nd/Config.in b/package/p910nd/Config.in > new file mode 100644 > index 0000000..490dace > --- /dev/null > +++ b/package/p910nd/Config.in > @@ -0,0 +1,8 @@ > +config BR2_PACKAGE_P910ND > + bool "p910nd" > + help > + p910nd is a small printer daemon intended for diskless > + workstations. Using ports 9100-9102, it accepts > + print jobs and passes them directly to a USB printer. > + > + http://p910nd.sourceforge.net/ Doesn't this package rely on any external libary at all? Not even libusb? > diff --git a/package/p910nd/S55p910nd b/package/p910nd/S55p910nd > new file mode 100644 > index 0000000..8778a62 > --- /dev/null > +++ b/package/p910nd/S55p910nd > @@ -0,0 +1,48 @@ > +#!/bin/sh > + > +DEFAULT=/etc/default/p910nd We don't normally have a /etc/default directory. Even so, it normally contains shell fragments rather than configuration files (at least it does on Debian+derivatives). Instead I would name the config file /etc/p910nd.conf > +RUN_D=/var/run > + > +_start() { > + mkdir -p $RUN_D We usually use tabs to indent shell scripts. > + [ -f $DEFAULT ]&& ( Space between ] and && The sub-shell isn't needed, { ... } would be sufficient. Or it could even be left out since there's only one command (while) inside. > + while read port options; do > + case "$port" in > + ""|\#*) > + continue; > + esac > + p910nd $options $port We usually use start-stop-daemon to control daemons. > + if [ $? -ne 0 ]; then > + exit 1 > + fi > + done > + )< $DEFAULT > + exit 0 > +} > + > +_stop() { > + [ -f $DEFAULT ]&& ( > + while read port options; do > + case "$port" in > + ""|\#*) > + continue; > + esac > + PID_F=$RUN_D/p910${port}d.pid > + [ -f $PID_F ]&& kill $(cat $PID_F) This has the annoying effect of not killing things properly if the configuration file has changed. What about: for PID_F in $RUN_D/p910?d.pid; do start-stop-daemon -K -p $PID_F -x /usr/sbin/p910nd done > + rm $PID_F > + done > + )< $DEFAULT > +} > + > +case $1 in > + start) > + _start > + ;; > + stop) > + _stop > + ;; > + *) > + echo "usage: $0 (start|stop)" Please add a restart as well. > + exit 1 > +esac > +exit $? > diff --git a/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch > b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch > new file mode 100644 > index 0000000..867b8cf > --- /dev/null > +++ b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch > @@ -0,0 +1,13 @@ > +Index: p910nd/p910nd.c Patches should start with a comment explaining what the patch does and why it is needed, just like a commit message. It should also contain a Signed-off-by line. > +=================================================================== > +--- p910nd.orig/p910nd.c 2011-11-14 22:47:41.986401420 +0100 > ++++ p910nd/p910nd.c 2011-11-14 22:49:27.274923524 +0100 > +@@ -122,7 +122,7 @@ > + #ifdef LOCKFILE_DIR > + #define LOCKFILE LOCKFILE_DIR "/p910%cd" Wouldn't it be possible to simply add -DLOCKFILE_DIR='"/var/lock"' to CFLAGS (if the Makefile allows extending CFLAGS with something extra). > + #else > +-#define LOCKFILE "/var/lock/subsys/p910%cd" > ++#define LOCKFILE "/var/lock/p910%cd" > + #endif > + #ifndef PRINTERFILE > + #define PRINTERFILE "/dev/lp%c" > diff --git a/package/p910nd/p910nd.default b/package/p910nd/p910nd.default > new file mode 100644 > index 0000000..77317cf > --- /dev/null > +++ b/package/p910nd/p910nd.default > @@ -0,0 +1,9 @@ > +# printing port list, in the form "number [options]" > +# where: > +# - number is the port number in the range [0-9] > +# the p910nd daemon will listen on tcp port 9100+number > +# - options can be : > +# -b to turn on bidirectional copying. > +# -f to specify a different printer device. > +# > +0 -b -f /dev/usb/lp0 > diff --git a/package/p910nd/p910nd.mk b/package/p910nd/p910nd.mk > new file mode 100644 > index 0000000..c229b32 > --- /dev/null > +++ b/package/p910nd/p910nd.mk > @@ -0,0 +1,23 @@ > +############################################################# > +# > +# p910nd > +# > +############################################################# > + > +P910ND_VERSION = 0.95 > +P910ND_SITE = > http://voxel.dl.sourceforge.net/project/p910nd/p910nd/$(P910ND_VERSION) Use $(BR2_SOURCEFORGE_MIRROR) instead of a fixed mirror (voxel). Also, it looks like your patch is line-wrapped, so it won't apply cleanly. The easiest way to avoid this is to use git send-email (see the man page on how to use it with gmail). > +P910ND_SOURCE = p910nd-$(P910ND_VERSION).tar.bz2 > + > +define P910ND_BUILD_CMDS > + $(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D) Nowadays, we usually use $(MAKE) $(TARGET_CONFIGURE_OPTS) $(P910ND_MAKE_OPTS) -C $(@D) where P910ND_MAKE_OPTS are the extra make flags, if any (in this case none). > +endef > + > +define P910ND_INSTALL_TARGET_CMDS > + $(INSTALL) -D -m 0755 $(@D)/p910nd $(TARGET_DIR)/usr/sbin/p910nd > + $(INSTALL) -d -m 0755 $(TARGET_DIR)/etc/default Creating the directory is done automatically by the -D option used below. > + $(INSTALL) -m 0755 -D package/p910nd/p910nd.default > $(TARGET_DIR)/etc/default/p910nd This one shouldn't be executable (mode 0644 instead of 0755). > + $(INSTALL) -m 0755 -D package/p910nd/S55p910nd > $(TARGET_DIR)/etc/init.d/S55p910nd > + > +endef > + > +$(eval $(call GENTARGETS,package,p910nd)) The extra arguments for GENTARGETS are unnecessary. Regards, Arnout
Arnout, bedankt ... Your comments were very insightful and helpful. I addressed each item you mentioned... [do I need to put a version history (listing changes) with my V2 patch?] On Wed, Jun 6, 2012 at 2:43 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 06/03/12 21:52, David Purdy wrote: >> > Please include a Signed-off-by line for yourself. Yes, I normally do... it was actually a "resend" of the original git send-email-generated patch, I guess no one saw the original one... ? > > > The rest of my comments are a bit of nitpicking. **Nitpicking?** Certainly just good suggestions - I highly appreciate them. Hartelijk bedankt! > [snip] > >> diff --git a/package/p910nd/Config.in b/package/p910nd/Config.in >> new file mode 100644 >> index 0000000..490dace >> --- /dev/null >> +++ b/package/p910nd/Config.in >> @@ -0,0 +1,8 @@ >> +config BR2_PACKAGE_P910ND >> + bool "p910nd" >> + help >> + p910nd is a small printer daemon intended for diskless >> + workstations. Using ports 9100-9102, it accepts >> + print jobs and passes them directly to a USB printer. >> + >> + http://p910nd.sourceforge.net/ > > Doesn't this package rely on any external libary at all? > Not even libusb? Nope, none at all. >> diff --git a/package/p910nd/S55p910nd b/package/p910nd/S55p910nd >> new file mode 100644 >> index 0000000..8778a62 >> --- /dev/null >> +++ b/package/p910nd/S55p910nd >> @@ -0,0 +1,48 @@ >> +#!/bin/sh >> + >> +DEFAULT=/etc/default/p910nd > > > We don't normally have a /etc/default directory. Even so, it > normally contains shell fragments rather than configuration files > (at least it does on Debian+derivatives). > > Instead I would name the config file /etc/p910nd.conf Done, you are right, it makes better sense. > >> +RUN_D=/var/run >> + >> +_start() { >> + mkdir -p $RUN_D > > > We usually use tabs to indent shell scripts. Fixed. > >> + [ -f $DEFAULT ]&& ( > > > Space between ] and && > > The sub-shell isn't needed, { ... } would be sufficient. > Or it could even be left out since there's only one command > (while) inside. > > >> + while read port options; do >> + case "$port" in >> + ""|\#*) >> + continue; >> + esac >> + p910nd $options $port > > > We usually use start-stop-daemon to control daemons. Will also change & clean up the entire initscript... > >> + if [ $? -ne 0 ]; then >> + exit 1 >> + fi >> + done >> + )< $DEFAULT >> + exit 0 >> +} >> + >> +_stop() { >> + [ -f $DEFAULT ]&& ( >> + while read port options; do >> + case "$port" in >> + ""|\#*) >> + continue; >> + esac >> + PID_F=$RUN_D/p910${port}d.pid >> + [ -f $PID_F ]&& kill $(cat $PID_F) > > > This has the annoying effect of not killing things properly > if the configuration file has changed. What about: > > for PID_F in $RUN_D/p910?d.pid; do > start-stop-daemon -K -p $PID_F -x /usr/sbin/p910nd > done Thanks for this - I'll certainly try to incorporate it. >> + rm $PID_F >> + done >> + )< $DEFAULT >> +} >> + >> +case $1 in >> + start) >> + _start >> + ;; >> + stop) >> + _stop >> + ;; >> + *) >> + echo "usage: $0 (start|stop)" > > > Please add a restart as well. Yup, that makes sense as well. >> + exit 1 >> +esac >> +exit $? >> diff --git >> a/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch >> >> b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch >> new file mode 100644 >> index 0000000..867b8cf >> --- /dev/null >> +++ >> b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch >> @@ -0,0 +1,13 @@ >> +Index: p910nd/p910nd.c > > > Patches should start with a comment explaining what the patch does and why > it is needed, just like a commit message. It should also contain a > Signed-off-by line. With your suggestions about P910ND_MAKE_OPTS, the patch is unnecessary (yes, simpler this way) - thanks for the idea. >> +=================================================================== >> +--- p910nd.orig/p910nd.c 2011-11-14 22:47:41.986401420 +0100 >> ++++ p910nd/p910nd.c 2011-11-14 22:49:27.274923524 +0100 >> +@@ -122,7 +122,7 @@ >> + #ifdef LOCKFILE_DIR >> + #define LOCKFILE LOCKFILE_DIR "/p910%cd" > > > Wouldn't it be possible to simply add -DLOCKFILE_DIR='"/var/lock"' to > CFLAGS > (if the Makefile allows extending CFLAGS with something extra). > > >> + #else >> +-#define LOCKFILE "/var/lock/subsys/p910%cd" >> ++#define LOCKFILE "/var/lock/p910%cd" >> + #endif >> + #ifndef PRINTERFILE >> + #define PRINTERFILE "/dev/lp%c" >> diff --git a/package/p910nd/p910nd.default b/package/p910nd/p910nd.default >> new file mode 100644 >> index 0000000..77317cf >> --- /dev/null >> +++ b/package/p910nd/p910nd.default >> @@ -0,0 +1,9 @@ >> +# printing port list, in the form "number [options]" >> +# where: >> +# - number is the port number in the range [0-9] >> +# the p910nd daemon will listen on tcp port 9100+number >> +# - options can be : >> +# -b to turn on bidirectional copying. >> +# -f to specify a different printer device. >> +# >> +0 -b -f /dev/usb/lp0 >> diff --git a/package/p910nd/p910nd.mk b/package/p910nd/p910nd.mk >> new file mode 100644 >> index 0000000..c229b32 >> --- /dev/null >> +++ b/package/p910nd/p910nd.mk >> @@ -0,0 +1,23 @@ >> +############################################################# >> +# >> +# p910nd >> +# >> +############################################################# >> + >> +P910ND_VERSION = 0.95 >> +P910ND_SITE = >> http://voxel.dl.sourceforge.net/project/p910nd/p910nd/$(P910ND_VERSION) > > > Use $(BR2_SOURCEFORGE_MIRROR) instead of a fixed mirror (voxel). Also fixed to be standardized as all the other packages/*.mk files that utilize SourceForge... > Also, it looks like your patch is line-wrapped, so it won't apply cleanly. > The easiest way to avoid this is to use git send-email (see the man page > on how to use it with gmail). Addressed above... >> +P910ND_SOURCE = p910nd-$(P910ND_VERSION).tar.bz2 >> + >> +define P910ND_BUILD_CMDS >> + $(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D) > > > Nowadays, we usually use > > $(MAKE) $(TARGET_CONFIGURE_OPTS) $(P910ND_MAKE_OPTS) -C $(@D) > > where P910ND_MAKE_OPTS are the extra make flags, if any (in this > case none). Yes, addressed as well. Thanks. >> +endef >> + >> +define P910ND_INSTALL_TARGET_CMDS >> + $(INSTALL) -D -m 0755 $(@D)/p910nd $(TARGET_DIR)/usr/sbin/p910nd >> + $(INSTALL) -d -m 0755 $(TARGET_DIR)/etc/default > > > Creating the directory is done automatically by the -D option used below. Unneeded, since p910nd.conf will live in /etc >> + $(INSTALL) -m 0755 -D package/p910nd/p910nd.default >> $(TARGET_DIR)/etc/default/p910nd > > > This one shouldn't be executable (mode 0644 instead of 0755). Fixed. >> + $(INSTALL) -m 0755 -D package/p910nd/S55p910nd >> $(TARGET_DIR)/etc/init.d/S55p910nd >> + >> +endef >> + >> +$(eval $(call GENTARGETS,package,p910nd)) > > > The extra arguments for GENTARGETS are unnecessary. Yes, you're right. > Regards, > Arnout > > -- > Arnout Vandecappelle arnout at mind be > Senior Embedded Software Architect +32-16-286540 > Essensium/Mind http://www.mind.be > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven Overall, this actually shortened and cleaned it up a lot, both for the package files, and when installed, as well. Will send a [PATCH V2] within a few days. regards, thanks, Dave Purdy
diff --git a/package/Config.in b/package/Config.in index fb1b08f..c600630 100644 --- a/package/Config.in +++ b/package/Config.in @@ -549,6 +549,7 @@ source "package/openntpd/Config.in" source "package/openssh/Config.in" source "package/openswan/Config.in" source "package/openvpn/Config.in" +source "package/p910nd/Config.in" source "package/portmap/Config.in" source "package/pppd/Config.in" source "package/pptp-linux/Config.in" diff --git a/package/p910nd/Config.in b/package/p910nd/Config.in new file mode 100644 index 0000000..490dace --- /dev/null +++ b/package/p910nd/Config.in @@ -0,0 +1,8 @@ +config BR2_PACKAGE_P910ND + bool "p910nd" + help + p910nd is a small printer daemon intended for diskless + workstations. Using ports 9100-9102, it accepts + print jobs and passes them directly to a USB printer. + + http://p910nd.sourceforge.net/ diff --git a/package/p910nd/S55p910nd b/package/p910nd/S55p910nd new file mode 100644 index 0000000..8778a62 --- /dev/null +++ b/package/p910nd/S55p910nd @@ -0,0 +1,48 @@ +#!/bin/sh + +DEFAULT=/etc/default/p910nd +RUN_D=/var/run + +_start() { + mkdir -p $RUN_D + [ -f $DEFAULT ] && ( + while read port options; do + case "$port" in + ""|\#*) + continue; + esac + p910nd $options $port + if [ $? -ne 0 ]; then + exit 1 + fi + done + ) < $DEFAULT + exit 0 +} + +_stop() { + [ -f $DEFAULT ] && ( + while read port options; do + case "$port" in + ""|\#*) + continue; + esac + PID_F=$RUN_D/p910${port}d.pid + [ -f $PID_F ] && kill $(cat $PID_F) + rm $PID_F + done + ) < $DEFAULT +} + +case $1 in + start) + _start + ;; + stop) + _stop + ;; + *) + echo "usage: $0 (start|stop)" + exit 1 +esac +exit $? diff --git a/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch new file mode 100644 index 0000000..867b8cf --- /dev/null +++ b/package/p910nd/p910nd-0.95-use_var-lock-instead-of-var_log_subsys_p910nd.patch @@ -0,0 +1,13 @@ +Index: p910nd/p910nd.c +=================================================================== +--- p910nd.orig/p910nd.c 2011-11-14 22:47:41.986401420 +0100 ++++ p910nd/p910nd.c 2011-11-14 22:49:27.274923524 +0100 +@@ -122,7 +122,7 @@ + #ifdef LOCKFILE_DIR + #define LOCKFILE LOCKFILE_DIR "/p910%cd" + #else +-#define LOCKFILE "/var/lock/subsys/p910%cd" ++#define LOCKFILE "/var/lock/p910%cd" + #endif + #ifndef PRINTERFILE + #define PRINTERFILE "/dev/lp%c" diff --git a/package/p910nd/p910nd.default b/package/p910nd/p910nd.default new file mode 100644 index 0000000..77317cf --- /dev/null +++ b/package/p910nd/p910nd.default @@ -0,0 +1,9 @@ +# printing port list, in the form "number [options]" +# where: +# - number is the port number in the range [0-9] +# the p910nd daemon will listen on tcp port 9100+number +# - options can be : +# -b to turn on bidirectional copying. +# -f to specify a different printer device. +# +0 -b -f /dev/usb/lp0 diff --git a/package/p910nd/p910nd.mk b/package/p910nd/p910nd.mk new file mode 100644 index 0000000..c229b32 --- /dev/null +++ b/package/p910nd/p910nd.mk @@ -0,0 +1,23 @@ +############################################################# +# +# p910nd +# +############################################################# + +P910ND_VERSION = 0.95 +P910ND_SITE = http://voxel.dl.sourceforge.net/project/p910nd/p910nd/$(P910ND_VERSION) +P910ND_SOURCE = p910nd-$(P910ND_VERSION).tar.bz2 + +define P910ND_BUILD_CMDS + $(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D) +endef + +define P910ND_INSTALL_TARGET_CMDS + $(INSTALL) -D -m 0755 $(@D)/p910nd $(TARGET_DIR)/usr/sbin/p910nd + $(INSTALL) -d -m 0755 $(TARGET_DIR)/etc/default + $(INSTALL) -m 0755 -D package/p910nd/p910nd.default $(TARGET_DIR)/etc/default/p910nd + $(INSTALL) -m 0755 -D package/p910nd/S55p910nd $(TARGET_DIR)/etc/init.d/S55p910nd +