[Buildroot] [PATCH 6/6] targetcli-fb: add sysv initscript
Christophe Vu-Brugier
cvubrugier at fastmail.fm
Wed Sep 17 13:19:25 UTC 2014
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 at 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!
More information about the buildroot
mailing list