| Message ID | 1410953999-16520-7-git-send-email-cvubrugier@fastmail.fm |
|---|---|
| State | Superseded |
| Headers | show |
Dear Christophe Vu-Brugier, On Wed, 17 Sep 2014 13:39:59 +0200, Christophe Vu-Brugier wrote: > Signed-off-by: Christophe Vu-Brugier <cvubrugier@fastmail.fm> > --- > package/targetcli-fb/S50target | 46 ++++++++++++++++++++++++++++++++++++ > package/targetcli-fb/targetcli-fb.mk | 11 +++++++++ > 2 files changed, 57 insertions(+) > create mode 100755 package/targetcli-fb/S50target I think this should be part of the previous patch. > diff --git a/package/targetcli-fb/targetcli-fb.mk b/package/targetcli-fb/targetcli-fb.mk > index 2384569..b83a6b6 100644 > --- a/package/targetcli-fb/targetcli-fb.mk > +++ b/package/targetcli-fb/targetcli-fb.mk > @@ -11,4 +11,15 @@ TARGETCLI_FB_LICENSE_FILES = COPYING > TARGETCLI_FB_SETUP_TYPE = setuptools > TARGETCLI_FB_DEPENDENCIES = python-configshell-fb python-rtslib-fb > > +define TARGETCLI_FB_INSTALL_INITSCRIPT > + @if [ ! -f $(TARGET_DIR)/etc/init.d/S50target ]; then \ > + $(INSTALL) -m 0755 -D package/targetcli-fb/S50target $(TARGET_DIR)/etc/init.d/S50target; \ > + fi This should not be handled using a post install target hook, but rather using the TARGETCLI_FB_INIT_SYSV mechanism. See the Buildroot manual or other packages for example (dnsmasq for example, or ntp if you want to see both the SysV and the systemd cases). Also, I don't think adding a @ at the beginning is really needed, as we usually don't do that I believe. The test could also be summarized as: [ -f $(TARGET_DIR)/etc/init.d/S50target ] || \ $(INSTALL) ... > + @if [ ! -d $(TARGET_DIR)/etc/target ]; then \ > + $(INSTALL) -m 0755 -d $(TARGET_DIR)/etc/target; \ > + fi This should continue to belong to post install hook because it isn't related to which init system is used. Is the test really necessary in this case? Maybe you can just to the $(INSTALL), which doesn't fail if the directory already exists I believe. If not, just use mkdir -p and that's it. If you really want to be nice, you could also add a comment above the hook explaining why this directory is needed. Thanks! Thomas
Hi Thomas, On Wed, 17 Sep 2014 14:12:04 +0200, Thomas Petazzoni wrote : > On Wed, 17 Sep 2014 13:39:59 +0200, Christophe Vu-Brugier wrote: > > Signed-off-by: Christophe Vu-Brugier <cvubrugier@fastmail.fm> > > --- > > package/targetcli-fb/S50target | 46 ++++++++++++++++++++++++++++++++++++ > > package/targetcli-fb/targetcli-fb.mk | 11 +++++++++ > > 2 files changed, 57 insertions(+) > > create mode 100755 package/targetcli-fb/S50target > > I think this should be part of the previous patch. OK. > > diff --git a/package/targetcli-fb/targetcli-fb.mk b/package/targetcli-fb/targetcli-fb.mk > > index 2384569..b83a6b6 100644 > > --- a/package/targetcli-fb/targetcli-fb.mk > > +++ b/package/targetcli-fb/targetcli-fb.mk > > @@ -11,4 +11,15 @@ TARGETCLI_FB_LICENSE_FILES = COPYING > > TARGETCLI_FB_SETUP_TYPE = setuptools > > TARGETCLI_FB_DEPENDENCIES = python-configshell-fb python-rtslib-fb > > > > +define TARGETCLI_FB_INSTALL_INITSCRIPT > > + @if [ ! -f $(TARGET_DIR)/etc/init.d/S50target ]; then \ > > + $(INSTALL) -m 0755 -D package/targetcli-fb/S50target $(TARGET_DIR)/etc/init.d/S50target; \ > > + fi > > This should not be handled using a post install target hook, but rather > using the TARGETCLI_FB_INIT_SYSV mechanism. See the Buildroot manual or > other packages for example (dnsmasq for example, or ntp if you want to > see both the SysV and the systemd cases). OK. By the way, I also intend to add a systemd unit file later. > Also, I don't think adding a @ at the beginning is really needed, as we > usually don't do that I believe. The test could also be summarized as: > > [ -f $(TARGET_DIR)/etc/init.d/S50target ] || \ > $(INSTALL) ... > > > + @if [ ! -d $(TARGET_DIR)/etc/target ]; then \ > > + $(INSTALL) -m 0755 -d $(TARGET_DIR)/etc/target; \ > > + fi > > This should continue to belong to post install hook because it isn't > related to which init system is used. Is the test really necessary in > this case? Maybe you can just to the $(INSTALL), which doesn't fail if > the directory already exists I believe. If not, just use mkdir -p and > that's it. If you really want to be nice, you could also add a comment > above the hook explaining why this directory is needed. The directory "/etc/target" is needed because it is expected by targetcli. The configuration of the "SCSI target" driver is stored in a JSON file named "/etc/target/saveconfig.json". This configuration is used to create the configfs hierarchy that configures the target driver. Thank you!
Dear Christophe Vu-Brugier, On Wed, 17 Sep 2014 15:19:25 +0200, Christophe Vu-Brugier wrote: > > This should not be handled using a post install target hook, but rather > > using the TARGETCLI_FB_INIT_SYSV mechanism. See the Buildroot manual or > > other packages for example (dnsmasq for example, or ntp if you want to > > see both the SysV and the systemd cases). > > OK. By the way, I also intend to add a systemd unit file later. Great! > > This should continue to belong to post install hook because it isn't > > related to which init system is used. Is the test really necessary in > > this case? Maybe you can just to the $(INSTALL), which doesn't fail if > > the directory already exists I believe. If not, just use mkdir -p and > > that's it. If you really want to be nice, you could also add a comment > > above the hook explaining why this directory is needed. > > The directory "/etc/target" is needed because it is expected by > targetcli. The configuration of the "SCSI target" driver is stored in a > JSON file named "/etc/target/saveconfig.json". This configuration is > used to create the configfs hierarchy that configures the target driver. Ok, makes sense. Maybe just add exactly this explanation as a comment above the creation of /etc/target ? Thanks! Thomas
diff --git a/package/targetcli-fb/S50target b/package/targetcli-fb/S50target new file mode 100755 index 0000000..88290e4 --- /dev/null +++ b/package/targetcli-fb/S50target @@ -0,0 +1,46 @@ +#!/bin/sh +# +# Restore / clear the Linux "SCSI target" driver configuration with `targetctl` +# + +start() { + local ret + + echo -n "Restoring target configuration: " + /usr/bin/targetctl restore >/dev/null 2>&1 + ret=$? + echo "done" + + return $ret +} + +stop() { + local ret + + echo -n "Clearing target configuration: " + /usr/bin/targetctl clear >/dev/null 2>&1 + ret=$? + echo "done" + + return $ret +} + +restart() { + stop + start +} + +case "$1" in + start) + start + ;; + stop) + stop + ;; + restart) + restart + ;; + *) + echo "Usage: $0 {start|stop|restart}" + exit 1 +esac diff --git a/package/targetcli-fb/targetcli-fb.mk b/package/targetcli-fb/targetcli-fb.mk index 2384569..b83a6b6 100644 --- a/package/targetcli-fb/targetcli-fb.mk +++ b/package/targetcli-fb/targetcli-fb.mk @@ -11,4 +11,15 @@ TARGETCLI_FB_LICENSE_FILES = COPYING TARGETCLI_FB_SETUP_TYPE = setuptools TARGETCLI_FB_DEPENDENCIES = python-configshell-fb python-rtslib-fb +define TARGETCLI_FB_INSTALL_INITSCRIPT + @if [ ! -f $(TARGET_DIR)/etc/init.d/S50target ]; then \ + $(INSTALL) -m 0755 -D package/targetcli-fb/S50target $(TARGET_DIR)/etc/init.d/S50target; \ + fi + @if [ ! -d $(TARGET_DIR)/etc/target ]; then \ + $(INSTALL) -m 0755 -d $(TARGET_DIR)/etc/target; \ + fi +endef + +TARGETCLI_FB_POST_INSTALL_TARGET_HOOKS += TARGETCLI_FB_INSTALL_INITSCRIPT + $(eval $(python-package))
Signed-off-by: Christophe Vu-Brugier <cvubrugier@fastmail.fm> --- package/targetcli-fb/S50target | 46 ++++++++++++++++++++++++++++++++++++ package/targetcli-fb/targetcli-fb.mk | 11 +++++++++ 2 files changed, 57 insertions(+) create mode 100755 package/targetcli-fb/S50target