Bug 225915 - Merge Review: iscsi-initiator-utils
Summary: Merge Review: iscsi-initiator-utils
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Package Reviews List
Depends On:
TreeView+ depends on / blocked
Reported: 2007-01-31 19:07 UTC by Nobody's working on this, feel free to take it
Modified: 2008-12-12 22:17 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-12-12 22:17:48 UTC
Type: ---
adrian: fedora-review+

Attachments (Terms of Use)
Patch against devel branch (4.52 KB, patch)
2007-06-22 12:37 UTC, Adrian Reber
no flags Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 19:07:32 UTC
Fedora Merge Review: iscsi-initiator-utils

Initial Owner: mchristi@redhat.com

Comment 1 Adrian Reber 2007-06-22 12:36:12 UTC
The soon attached patches will fix following problems with this package:

* empty debuginfo package
* E: iscsi-initiator-utils executable-marked-as-config-file /etc/rc.d/init.d/iscsi
* E: iscsi-initiator-utils executable-marked-as-config-file /etc/rc.d/init.d/iscsid
* W: iscsi-initiator-utils service-default-enabled /etc/rc.d/init.d/iscsi
* W: iscsi-initiator-utils service-default-enabled /etc/rc.d/init.d/iscsid
* using Requires(...) instead of Prereq
* changed buildroot
* using --preserve-timestamps for install
* replaced hardcoded paths with rpm macros
* conditional execution of chkconfig in scriptlets

The remaining warnings from rpmlint:
E: iscsi-initiator-utils non-readable /etc/iscsi/iscsid.conf 0600
W: iscsi-initiator-utils summary-not-capitalized iSCSI daemon and utility programs
E: iscsi-initiator-utils statically-linked-binary /sbin/iscsistart
W: iscsi-initiator-utils no-reload-entry /etc/rc.d/init.d/iscsi
W: iscsi-initiator-utils no-reload-entry /etc/rc.d/init.d/iscsid

The only which should probably be looked at is to get FESCO approval for
statically-linked-binary /sbin/iscsistart.

As I get following warning from the compiler:
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -Wall -Wstrict-prototypes -I../include -DLinux
-DNETLINK_ISCSI=8 -D_GNU_SOURCE -static netlink.o util.o io.o auth.o login.o
log.o md5.o sha1.o iscsi_sysfs.o idbm.o initiator.o queue.o actor.o mgmt_ipc.o
isns.o transport.o iscsistart.o statics.o -o iscsistart
login.o: In function `resolve_address':
warning: Using 'getaddrinfo' in statically linked applications requires at
runtime the shared libraries from the glibc version used for linking

I am not sure, with this warning message, if static linking makes much sense here.

Comment 2 Adrian Reber 2007-06-22 12:37:38 UTC
Created attachment 157612 [details]
Patch against devel branch

Comment 3 Mike Christie 2007-06-22 18:10:18 UTC
Very nice. Thanks a lot. I was trying to fix up the fc7 rpm, but when I ran
rpmlint I did not see the statically built warning. I also just got stuck on
some other warning I see you fixed.

Do you just check in the patch or do I?

Also /sbin/iscsistart is statically linked because it is used in the initramfs
like how nash or some of the lvm tools are. If we are doing dymaically linked
tools in the initramfs then I can fix that up. If not how do I get approval?

Comment 4 Adrian Reber 2007-06-22 19:04:16 UTC
I can check in the changes on the F7 and/or devel branch if you remove the
pkg.acl file or add me to it.

I do not know about initramfs but if it is necessary FESCO will probably
approve. I can point FESCO to this ticket so that they can discuss this on their
next meeting.

Comment 5 Jeremy Katz 2007-06-22 19:18:10 UTC
Dynamically linked in the initramfs is fine for F7 and later.

Comment 6 Mike Christie 2007-06-25 20:05:20 UTC
I am not sure how to do the ACL stuff yet. I got to read that, but am a little
short on time.

I merged your patch into devel. I am also working on porting it to FC7. I will
also fix up the package so it does not compile iscsistart as static. Thanks
again for the review and patch.

Comment 7 Jeremy Katz 2007-06-25 20:11:46 UTC
Changing the service to not be enabled by default will break things for users
who actually have iSCSI as an FS during install.  We don't do service enablement
in anaconda (and really, would prefer to keep things that way just because it's
the slope to madness...)

Really, the iscsi startup bits should probably be integrated into rc.sysinit
instead of being a separate initscript.

Comment 8 Adrian Reber 2007-06-25 20:37:41 UTC
I have no problem if the iscsi services are enabled by default if there is (and
there seems to be) a good reason for it. I was just proposing the change because
rpmlint complained.

From my point of view this review is approved because all open items have been
addressed and you close this bug.

A minor thing: I saw your check in into CVS and for F-7 the line in the
changelog was much longer than 80 characters. You should probably wrap that line

Comment 9 Mike Christie 2007-06-25 21:10:18 UTC
Jeremy, I will fix that up and start looking into rc.sysinit integration.

Adrian, I will follow 80 chars from now on. I already built the package and did
the release stuff, so it is on its way out. I will check in the change, so on
the next release that will be fixed.

Comment 10 Mike Christie 2007-06-26 09:48:27 UTC
Hey, one question about this package,

For iscsi it was requested that we create a library that would read special
iscsi boot values. For example IBM/intel want to read the value to use for boot
from the sysfs acpi tree, or for IBM and ppc from the open firmware tree.
iscsistart would use this library, and anaconda could then also use this library
if you wanted to do something like when you hit the advanced storage button,
anaconda could check for those special values being set and could then use them
for install and boot.

For the library, should I create another package/rpm and submit it for review or
can I just include it in this one? I thought I read that it must go in a new
devel package, but I cannot find where I read that.

Comment 11 Till Maas 2008-12-12 22:17:48 UTC
According to comment: 8 and the fedora-review flag, this review was successfully completed, therefore I close it.

Note You need to log in before you can comment on or make changes to this bug.