Bug 225915

Summary: Merge Review: iscsi-initiator-utils
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: adrian, mchristi, opensource
Target Milestone: ---Flags: adrian: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-12-12 22:17:48 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Patch against devel branch none

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

http://cvs.fedora.redhat.com/viewcvs/devel/iscsi-initiator-utils/
Initial Owner: mchristi

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':
/home/adrian/devel/fedora/rpms/iscsi-initiator-utils/F-7/open-iscsi-2.0-865/usr/login.c:168:
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
somewhere.

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.