Hide Forgot
Spec URL: http://fedorapeople.org/gitweb?p=grover/public_git/targetcli.git;a=blob;f=targetcli.spec;h=9aaec06a37246a236897d2460f144b92a3f42892;hb=140e7bbb469ff09722faa7356357f10abe2b82a8 SRPM URL: http://grover.fedorapeople.org/srpms/targetcli-1.99.2.gitb03ec79-1.el6.src.rpm Description: This is a command-line interface (written in Python) that allows the admin to configure the kernel's scsi target subsystem to export local storage resources as iscsi or fcoe network block devices. An earlier version of this packaging has been reviewed by dmalcolm. Unfortunately upstream has still not tagged any releases, so we must fall back on git snapshots.
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.