Bug 483543 - Review Request: systemtapguiserver
Review Request: systemtapguiserver
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-02 05:47 EST by Anithra
Modified: 2009-05-09 00:03 EDT (History)
7 users (show)

See Also:
Fixed In Version: 1.0-8.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-21 21:05:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Avoid coding the compiler flags in the Makefile (1017 bytes, patch)
2009-02-03 18:26 EST, William Cohen
no flags Details | Diff
Revised spec file to use compiler options passed in by rpmbuild (1012 bytes, application/octet-stream)
2009-02-03 18:29 EST, William Cohen
no flags Details

  None (edit)
Description Anithra 2009-02-02 05:47:08 EST
Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/SystemTapGuiServer.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/SystemTapGuiServer-1.0-1.src.rpm
Description: SystemtapGui is a tool being developed to assist in writing SystemTap
scripts and viewing kernel performance.This package installs the server side
of SystemtapGui
Comment 1 William Cohen 2009-02-03 17:45:14 EST
This RPM will build on different architectures. Remove the following from the spec file:

BuildArch: i386 

Make the rpm file name agree with the one in:

https://bugzilla.redhat.com/show_bug.cgi?id=483205

Allow someone to do:

yum install "*stapgui*"
Comment 2 William Cohen 2009-02-03 18:26:59 EST
Created attachment 330797 [details]
Avoid coding the compiler flags in the Makefile

When RPMs are built through the build system CFLAGS and CXXFLAGS are passed in. This patch removes the hardcode options. The attached patch shows the changes in the makefile. I would think that these would be folded into the source file rather than carried as a patch in the RPM.

The spec file needs minor change to pass in the flags. That will be a separate attachment.

When compiling with the x86-64 F10 got the following warning during the build that should be examined more closely:

subscriptionMgr.cpp: In member function 'int datamanager::SubscriptionMgr::handleRequest(datamanager::dmRequest*, datamanager::dmResponse*, int)':
subscriptionMgr.cpp:130: warning: 'rc' may be used uninitialized in this function
datamanager.cpp: In member function 'void datamanager::DataManager::destroyStapProcess(datamanager::stap_process_t*)':
datamanager.cpp:413: warning: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result
datamanager.cpp: In member function 'void datamanager::DataManager::execStap(datamanager::dmRequest*)':
datamanager.cpp:799: warning: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result
Comment 3 William Cohen 2009-02-03 18:29:51 EST
Created attachment 330798 [details]
Revised spec file to use compiler options passed in by rpmbuild

This spec file makes use of the CFLAGS and CXXFLAGS passed into the rpmbuild. This should make the rpmbuild a bit more flexible as the compiler options may be changed in the builds, e.g. stricter checking of code syntax.
Comment 4 Anithra 2009-02-04 14:55:54 EST
(In reply to comment #1)
> This RPM will build on different architectures. Remove the following from the
> spec file:
> 
> BuildArch: i386 
> 
> Make the rpm file name agree with the one in:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=483205
> 
> Allow someone to do:
> 
> yum install "*stapgui*"

Thanks Will. I will rename the rpm to systemtapguiserver (removing camel casing) and will be renaming the client (bug 483205) to eclipse-systemtapgui. Will post the new package with all changes mentioned above shortly.
Comment 5 Anithra 2009-02-05 06:02:27 EST
Spec URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-2.src.rpm 

The package includes changes to the Makefile and spec as per Will's comments. The warnings have been resolved.
Comment 7 Armin 2009-02-09 12:57:57 EST
I don't think it's a good idea to have

INSTALL_DIR = /usr/sbin/stapgui-server

hard-coded in the makefile.  Do it in the specfile instead.
Comment 8 Anithra 2009-02-10 05:20:36 EST
(In reply to comment #7)
> I don't think it's a good idea to have
> 
> INSTALL_DIR = /usr/sbin/stapgui-server
> 
> hard-coded in the makefile.  Do it in the specfile instead.
This was actually redundant,  removed it from the makefile. 

Spec URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-4.src.rpm
Comment 9 William Cohen 2009-02-17 11:59:14 EST
The packaging for systemtapguiserver is looking okay to me. A couple minor changes:

- instead of $RPM_BUILD_ROOT use %{buildroot}

-Maybe make the %description more like to describe what this package is:

Server for Eclipse SystemTap GUI plugin. This eclipse plugin assists
writing SystemTap scripts and viewing kernel performance.
Comment 10 manuel wolfshant 2009-02-17 19:03:03 EST
William, if you'll take the time to read carefully https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS, you will certainly notice that there is no reason to replace $RPM_BUILD_ROOT with %{buildroot} or viceversa. The only rule is to not mix them in the same spec.
And to be honest, the %description from the spec looks more clear to me than the version suggested in comment #9. I especially like this part: "This package installs the server side of systemtapgui". Part which should end with a dot (hint, hint!).
Comment 12 William Cohen 2009-03-16 16:37:58 EDT
Re-read the packaging guidelines and used Bug 483205 (review of eclipse-systemtapgui) comment #23 to do a more systematic review. Found a couple minor things:

X %description, minor typo, "client.It"  Should be a space in there
X License: EPL but FSF GPLv2 based COPYING and INSTALL files in root of build.
X Empty AUTHORS, NEWS, README, and ChangeLog files in root of build directory

* package is named appropriately
* it is legal for Fedora to distribute this
* license field matches the actual license.
* license is open source-compatible.
* specfile name matches %{name}
* md5sum matches upstream
* skim the summary and description for typos, etc.
* correct buildroot
* %{?dist} used correctly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
* rpmlint on <this package>.srpm gives no output
* changelog format okay
* Summary tag does not end in a period 
* no PreReq
* specfile is legible
* package successfully compiles and builds on x86_64 (but is correctly noarch)
* summary and description fine
* specfile written in American English
* no -doc sub-package necessary
* not native, so no rpath, static linking, etc.
* no config files
* not a GUI app
* no -devel necessary
* install section begins with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no translations so no locale handling
* no Requires(pre,post) 
* package not relocatable
* package contains code
* package owns all directories and files
* no %files duplicates
* file permissions fine
* %clean present
* %doc files do not affect runtime
* not a web app
* verify the final provides and requires of the binary RPMs
  - these look good to me
* run rpmlint on the binary RPMs => no output
* package includes license text in the package and marks it with %doc 


Just starting to look over the internals of systemtapgui-server code.

In SubscriptionMgr::SubscriptionMgr() is the "sleep(1);" necessary?
Comment 13 Anithra 2009-03-17 08:25:40 EDT
Thanks Will, 

Updated spec and src rpm:

Spec URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-6.src.rpm  

I've removed the empty files, and INSTALL and COPYING. They were created cos i had used automake to create makefile.in, 

I agree the sleep(1)  doesn't look too good, its a stop gap solution , added it after there were some sync issues.
Comment 14 William Cohen 2009-04-01 00:09:26 EDT
Started playing with the systemtapgui-server (and finally got it to work).

The instructions are not clear but the current code needs to be run as root. It then figures out user from packet information and su to that user. It would be much better if people could run it as normal user for the case where the eclipse and the server are run by the same user.

There are a couple questionable cases in  datamanager.cpp:DataManager::execStap():

case (SHELL): allows executing arbitary script (as root this seems like a bad idea). what would prevent someone from using this to just connect and run arbitrary commands.

cases (BLUEDYE): mentions a package (Bluedye) that doesn't appear to be available in fedora


Why scp the file to the server machine? why not send it to the stapgui-server with the command and run with stap -e 'script...'? Currently, the plugin stores the password in plantext in a possibly world readable file. Also the current checks in the plugin do not seem to notice if the transfer failed (due to missing password).

The compile server does compile code, but it doesn't perform the other aspects of systemtapgui server such as execute the script and collect stdout/stderr. Could systemtapgui be stripped down just to use staprun to run a compiled script? Make it possible to run systemtap scripts on stripped down machines. This would be useful for cases of running code on compute nodes in a cluster.
Comment 15 Anithra 2009-04-01 11:20:58 EDT
(In reply to comment #14)
> It would be
> much better if people could run it as normal user for the case where the
> eclipse and the server are run by the same user.

I agree that users could be apprehensive about running the app as root and this is one of the issues that we are looking to fix soon. eclipse and the server in most cases may not be run by the same user as the server could be on a different machine. The server minimally needs to be run as root or users of group stapdev/stapusr to be able to run systemtap scripts, although the current code mandates that the server is run as root. 

> There are a couple questionable cases in 
> datamanager.cpp:DataManager::execStap():
> 
> case (SHELL): allows executing arbitary script (as root this seems like a bad
> idea). what would prevent someone from using this to just connect and run
> arbitrary commands.
> 
> cases (BLUEDYE): mentions a package (Bluedye) that doesn't appear to be
> available in fedora

The server was designed to be able to run any script(not just systemtap) and hence the above cases. This part of the code is there for future expansion. Currently these cases are never true  and so there is no chance of arbitrary commands being run.

> 
> Why scp the file to the server machine? why not send it to the stapgui-server
> with the command and run with stap -e 'script...'? Currently, the plugin stores

I had some problems with protocol management due to the variable length of the script. It might be better to transfer as part of the command through the socket connection and remove one step from the setup. Will redesign in subsequent releases. 

> the password in plantext in a possibly world readable file. Also the current
> checks in the plugin do not seem to notice if the transfer failed (due to
> missing password).
>

You should have got an error message saying "File transferred failed". Will look into it
 
> The compile server does compile code, but it doesn't perform the other aspects
> of systemtapgui server such as execute the script and collect stdout/stderr.
> Could systemtapgui be stripped down just to use staprun to run a compiled
> script? Make it possible to run systemtap scripts on stripped down machines.
> This would be useful for cases of running code on compute nodes in a cluster.  

This is one feature we are looking at in future releases of the client. Possibly by the end of the year. We are exploring the option of using the compile-server for compilation, and systemtapguiserver for execution. 

Thanks for the review. Can we treat the code changes/bug fixes as upstream issues so that this review is not blocked?.
Comment 16 Lubomir Rintel 2009-04-05 17:11:27 EDT
What's the status of this? Seems like it's being reviewed, but not flags or assignee is set.
Comment 17 Anithra 2009-04-06 08:21:18 EDT
(In reply to comment #16)
> What's the status of this? Seems like it's being reviewed, but not flags or
> assignee is set.  

The request is still waiting for a formal review and approval. It has not been assigned to anybody.
Comment 18 Lubomir Rintel 2009-04-14 15:50:16 EDT
- compiler flags used correctly
- spec file clean, legible, using American english
- filelist sane
- builds fine in mock

1.) Don't override reqprov generator please

AutoReqProv:    no

2.) This is useless, coreutils is in the build group
(see "koji list-groups dist-f11-build")

BuildRequires:  coreutils

3.) Don't strip the binary
Debuginfo generator will do that

strip stapgui-server

4.) Please use %{_smp_mflags} make flags

make

5.) You don't need to redundantly set 0755 mode 
0755 is install's default anyways
Moreover, you have a duplicate entry in filelist

install -m0755 stapgui-server ${RPM_BUILD_ROOT}/%{_bindir}/stapgui-server
...
%{_bindir}/*
...
%attr(0755,root,root) %{_bindir}/stapgui-server

6.) RPMlint whines
Both problems failry simple to fix

Source package:

systemtapguiserver.src: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 2)
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

And lots of these for debuginfo:

systemtapguiserver-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/systemtapguiserver-1.0/logger.cpp
The file is installed with executable permissions, but was identified as one
that probably should not be executable.  Verify if the executable bits are
desired, and remove if not.
Comment 19 Anithra 2009-04-15 08:47:10 EDT
Spec URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-7.src.rpm  

I had always had debuginfo disabled so have never verified it with rpmlint before :(. 

rpmlint output:

[root@localhost SOURCES]# rpmlint -i systemtapguiserver.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[root@localhost SOURCES]# rpmlint -i /usr/src/redhat/RPMS/i586/systemtapguiserver-1.0-7.i586.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@localhost SOURCES]# rpmlint -i /usr/src/redhat/RPMS/i586/systemtapguiserver-debuginfo-1.0-7.i586.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@localhost SOURCES]# rpmlint -i /usr/src/redhat/SRPMS/systemtapguiserver-1.0-7.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1300001
Comment 20 Lubomir Rintel 2009-04-16 04:03:56 EDT
* Wed Apr 15 2009 Anithra P Janakiraman <anithra@linux.vnet.ibm.com> 1.0-7
- Changes to spec file.

Anithra, this is the extreme example of poorly written changelog entry. When writing change log entries, describe what you changed, such as:

- Don't strip binary anymore
- Correct the file modes
...

Anyways, not anything that would block the review.

- compiler flags used correctly
- spec file clean, legible, using American english
- filelist sane
- builds fine in mock
- requires fine
- provides fine
- builds in mock
- rpmlint happy

The package is

APPROVED
Comment 21 Anithra 2009-04-16 04:58:52 EDT
(In reply to comment #20)
> Anithra, this is the extreme example of poorly written changelog entry. When
> writing change log entries, describe what you changed, such as:
> 
> - Don't strip binary anymore
> - Correct the file modes

Point taken!. will do. 

> 
> The package is
> 
> APPROVED  

Thanks Lubomir!
Comment 22 Anithra 2009-04-16 05:03:51 EDT
New Package CVS Request
=======================
Package Name: systemtapguiserver
Short Description: Server for the eclipse-systemtapgui client
Owners: anithra
Branches: F-10 F-11
InitialCC:
Comment 23 Kevin Fenzi 2009-04-17 12:28:21 EDT
cvs done.
Comment 24 Fedora Update System 2009-04-19 15:02:04 EDT
systemtapguiserver-1.0-8.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/systemtapguiserver-1.0-8.fc11
Comment 25 Fedora Update System 2009-04-19 15:02:10 EDT
systemtapguiserver-1.0-8.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/systemtapguiserver-1.0-8.fc10
Comment 26 Fedora Update System 2009-04-21 21:05:20 EDT
systemtapguiserver-1.0-8.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2009-05-09 00:03:02 EDT
systemtapguiserver-1.0-8.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

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