Bug 744340

One more thing. Targetcli is a successor to lio-utils, but still relies on lio-utils code for saving the configuration (so it can be restore on reboot.) This is an unspeakable hack, and I'm working with upstream to save config properly, but until then, lio-utils code is added to targetcli via a patch.

rpmlint output:
on SRPM:
targetcli.src: W: strange-permission targetcli.init 0775L
targetcli.src:11: W: macro-in-comment %{version}
targetcli.src:11: W: macro-in-comment %{version}
targetcli.src: W: invalid-url Source0: targetcli-1.99.2.gitb03ec79.tar.gz
1 packages and 0 specfiles checked; 0 errors, 8 warnings.


on RPM:
targetcli.noarch: E: explicit-lib-dependency python-rtslib
targetcli.noarch: W: spurious-executable-perm /usr/share/man/man8/targetcli.8.gz
targetcli.noarch: W: service-default-enabled /etc/rc.d/init.d/targetcli
targetcli.noarch: W: service-default-enabled /etc/rc.d/init.d/targetcli
targetcli.noarch: W: no-reload-entry /etc/rc.d/init.d/targetcli
targetcli.noarch: E: subsys-not-used /etc/rc.d/init.d/targetcli
1 packages and 0 specfiles checked; 2 errors, 8 warnings.

(spelling fixes omitted)

The package is fine with all MUSTs in packagereview process.

But initscript is a no-no.

First, new packages for F16 and following version should provide systemd unit. Optional SysV initscript need to go into subpackage.

Second, this scripts does many, many things which should be split:
- module load should be handled by snippets dropped in /etc/module-load.d/
- configfs mount should be handled by systemd .mount unit or left for a distro to set up
- config restoration is handled by running few scripts. I see that you are working with upstream to save config otherwise, but for know I recommend:
  - create systemd .service unit for each target type; Type=oneshot, ConditionFile=/etc/target/lio_start.sh (and others), ExecStart the same .sh
  - create blocktarget.target, which requires:
    - sys-kernel-config.mount
    - all .service units created

This should make it fine.

Created attachment 531370 [details]
liotarget.target

I prepared unit files fora target configuration.

Created attachment 531371 [details]
lio-restore-config-fc.service

Created attachment 531372 [details]
lio-restore-config-lio.service

Created attachment 531373 [details]
lio-restore-config-loopback.service

Created attachment 531374 [details]
lio-restore-config-tcm.service

Updated .spec and src rpm links:

http://fedorapeople.org/gitweb?p=grover/public_git/targetcli.git;a=blob;f=targetcli.spec;h=81e7b9a12970af66a9e584c78b3ca325e065597c;hb=2f7520113f42653254d4b157932b9aefc97a2f58

http://grover.fedorapeople.org/srpms/targetcli-1.99.2.gitb03ec79-3.fc16.src.rpm

I patched targetcli to create a single script called all_start.sh. This should simplify systemd config, and will be more in-line with the new implementation, which will also have a single config file. Made other modifications to spec for systemd, please take a look, thanks!

Created attachment 531753 [details]
/etc/modules-load.d/targetcli.conf

Two little nitpicks:
1) you don't need mkdir -p %{buildroot}%{_sysconfdir}/rc.d/init.d in spec anymore
2) module loading from scripts should be avoided, and done by dropping list of modules in /etc/modules-load.d/ dir.

But those are really details. Apart from this, thanks for packaging this!

Thanks for the review, I will incorporate your remaining fixes, and go ahead with new pkg scm request as you have set fedora-review+.

If you can, would you also consider reviewing the library dependencies for this package, bug 744342, bug 744347, and bug 744349?

Thanks again.

New Package SCM Request
=======================
Package Name: targetcli
Short Description: An administration shell for storage targets
Owners: grover
Branches: f16
InitialCC: zdzichu

Git done (by process-git-requests).

Removed InitialCC, not a valid FAS account.