Bug 492479 - Review Request: razertool - Tool for controlling Razer Copperhead mice
Summary: Review Request: razertool - Tool for controlling Razer Copperhead mice
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Christian Krause
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-03-26 22:03 UTC by Andreas Osowski
Modified: 2009-05-28 08:03 UTC (History)
5 users (show)

Fixed In Version: 0.0.7-5.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-30 07:05:26 UTC
Type: ---
chkr: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
sample hal policy file (512 bytes, text/plain)
2009-04-21 23:10 UTC, Christian Krause
no flags Details

Description Andreas Osowski 2009-03-26 22:03:07 UTC
Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-1.fc10.src.rpm

Description:
RazerTool is an unofficial tool for controlling Razer Copperhead mice in
Linux. It has both command-line and gtk+-2.x versions.

Comment 1 Christian Krause 2009-04-14 19:44:27 UTC
I've reviewed your package and it looks quite good. There is one TODO item and I have a small usage question (see below).

* rpmlint: OK
rpmlint SPECS/razertool.spec RPMS/i386/razertool-* SRPMS/razertool-0.0.7-1.fc10.src.rpm
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

* naming: OK

* spec file name: OK

* License: OK
- GPLv2+: acceptable
- matches the actual license
- license file packaged

* spec file in English and legible: OK

* Source0: OK
- newest upstream version used
- source file matches upstream - md5sum:
dad8236b2fface39f054a7ca6d1a448c  razertool_0.0.7.orig.tar.gz
- spectool -g works

* compilation: OK
- mock (F10)
- koji: F11, F10 and F9
- %{optflags} are used correctly
- smpflags used

* locale handling: OK

* debuginfo: OK
- debuginfo files are not empty and can be used in gdb

* handling of shared/static libraries, header files, pkgconfig files: OK (n/a)

* directory ownership: OK
- no directories created
- only directories used which are included in the base filesystem

* no files listed twice: OK

* file permissions and ownership: OK

* %clean section: OK

* macro usage: OK

* code vs. content: OK (only code)

* subpackage for large documentation: OK (n/a)

* %doc: OK

* rm -rf in %install: OK

* filenames UTF8: OK

* desktop file: OK
- it may be debatable in which menu the razortool appears (Preferences vs. Administration), but since it changes the configuration values globally (in the mouse), Administration seems the best option...

* buildrequires: OK

* scriptlets: OK

* final requires: OK

* functional test: TODO
- programs run without segfault ;-)
- since I don't have such a device, I can't do a full functional test
- razertool-gtk doesn't find its icon:
** (razertool-gtk:12930): WARNING **: Couldn't find pixmap file: razertool-icon.png

This is caused by moving the pixmaps in the spec file (the program will search the icon in the old place).

I would suggest the following:
- don't move %{_datadir}/%{name}/pixmaps, but package %{_datadir}/%{name} completly
- use the full path to the icon in the *.desktop file (That's OK according to http://fedoraproject.org/wiki/Packaging/Guidelines#Icon_tag_in_Desktop_Files)

Additionally I have one (optional) question: I've seen that libusb tries to open the files in /dev/bus/usb/*/*. On my system the user has no write permissions to any of these files (not even the one of the mouse). Is there a need to run this program as root or do you have to explicitly change the permissions of these files?

Comment 2 Andreas Osowski 2009-04-15 13:58:00 UTC
Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-2.fc10.src.rpm

I fixed the pixmaps issue.
Concerning that question regarding access rights...
You are right, due to the permissions the tool has to be either run as root or a rule has to be added to udev.
As I'm no fan of the adding-a-rule scenario, I'm wondering how to realize a password request here. Any ideas?

Comment 3 Christoph Wickert 2009-04-15 14:29:07 UTC
With pam:

/etc/security/console.apps/razertool:
. config-util
PROGRAM=/usr/sbin/razertool
SESSION=true

In order to install razertool to /usr/sbin instead of /usr/bin, configure with
%configure --bindir=%{_sbindir}

/etc/pam.d/razertool:
#%PAM-1.0
auth            include         config-util
account         include         config-util
session         include         config-util

If you choose the pam approach, don't forget to require usermode-gtk.

I would however prefer not to run this tool as root. Are these per-user or global settings? Running as root is ok if it's golbal, but as downside a weakness in the code could be exploited with root privileges.
On the other hand I don't like the idea of making _all_ the usb device files writable. Does the program really need write access or is it just probing the devices? Or is there a special device for the razer mice? If so, a file in /etc/security/console.perms.d/ might suffice.

Comment 4 Andreas Osowski 2009-04-15 14:50:43 UTC
Well, I'm pretty unsure.
The risk when using pam is clear to me... and should be avoided.

<snip>
ACTION=="add", BUS=="usb", SYSFS{idVendor}=="1532", SYSFS{idProduct}=="0101", MODE="0660", GROUP="plugdev"
ACTION=="add", BUS=="usb", SYSFS{idVendor}=="1532", SYSFS{idProduct}=="0009", MODE="0660", GROUP="plugdev"
</snip>

This is the udev example rulefile.
As you can see, this rulefile restricts the permission changes to Razer-only devices. There is no special device for Razer mice.

The changes are rather hardware-side, as they are directly transmitted to the Razer (mouse) hardware.

Comment 5 Christoph Wickert 2009-04-15 15:05:36 UTC
In this case udev should IMO be preferred.

Comment 6 Andreas Osowski 2009-04-15 15:10:03 UTC
Then I shall go for this.
One last question remains though:
in the example udev case, the permissions are assigned to a group.
As on Fedora, the user is -- by default -- in no group, should permissions to write be set for everyone here (equaling 0666)?

Comment 7 Christian Krause 2009-04-15 21:13:34 UTC
(In reply to comment #2)
> Spec URL:
> http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec
> 
> SRPM URL:
> http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-2.fc10.src.rpm
> 
> I fixed the pixmaps issue.

Thanks! Yes, that fixed the problem.

A minor issue has leaked in with the latest fix:

Since you've used
%{_datadir}/%{name}/*
to package the files in /usr/share/razertool/*, only the contents of the directory but not the directory itself is packaged.
This lead to an directory which isn't owned by any package:

rpm -qf /usr/share/razertool
file /usr/share/razertool is not owned by any package

The fix is quite easy, just use
%{_datadir}/%{name}/
or
%{_datadir}/%{name}
to package the complete directory.

Just for completness: please also removed the unused line:
#rm -rf %{buildroot}%{_datadir}/%{name}/
from the spec file. ;-)

Comment 8 Christoph Wickert 2009-04-15 21:40:39 UTC
No correction of Christian's comment, just a more elaborate explanation for our new contributor:

(In reply to comment #7)
> %{_datadir}/%{name}/
> or
> %{_datadir}/%{name}

I would prefer %{_datadir}/%{name}/. It doesn't make a difference from the technical point of view, but another human that reads the spec will realize that it's a dir and not a file.

Comment 9 Andreas Osowski 2009-04-16 05:46:38 UTC
Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-3.fc10.src.rpm

* Thu Apr 16 2009 Andreas Osowski <th0br0@mkdir.name> - 0.0.7-3
- Code cleanup
- Permission fix
- Added the rules file for udev

Comment 10 Andreas Osowski 2009-04-16 11:15:24 UTC
Christoph Wickert is sponsoring me, FE-NEEDSPONSOR removed.

Comment 11 Christian Krause 2009-04-21 23:08:17 UTC
(In reply to comment #9)

I've looked at the newest package and it looks quite good besides the following enhancement of the permission management for the mouse's device file.

> - Added the rules file for udev  

Instead of the udev rule which sets the permissions of the device file to 0666 I would suggest the following:

Add a hal rule to /usr/share/hal/fdi/policy/10osvendor:

- let hal set for the specific vendor/device ID of the mice additional permissions for the local user (it is already done for mice for the /dev/input/event* device) for the "linux.device_file" (/dev/bus/usb/*/*).
- make sure the number is lower than 20 (which defines the acl actions)

I've attached a sample file which changes the permissions of linux's device file associated with a specific vendor and device id (which must be filled out).

Comment 12 Christian Krause 2009-04-21 23:10:29 UTC
Created attachment 340650 [details]
sample hal policy file

Comment 13 Andreas Osowski 2009-04-27 13:23:58 UTC
Hello,
first of all sorry for the bi~g delay.

I added the hal policy and removed the udev rule.

Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-4.fc10.src.rpm

Comment 14 Christian Krause 2009-04-27 22:17:39 UTC
Hi Andreas,

Thanks for the new pacakge - we're very close to approval now. ;-) Unfortunately one minor issue appeared by adding the hal rule:

Since the package contains a file in /usr/share/hal/fdi/policy/10osvendor/ and this directory belongs to hal, razertool must require hal:

http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

Best regards,
Christian

Comment 15 Andreas Osowski 2009-04-28 04:19:27 UTC
Oh, that's true. I totally forgot about that requirement :)

Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-5.fc10.src.rpm

Comment 16 Christian Krause 2009-04-28 20:48:09 UTC
Thanks for the update!

All open issues were addressed in the last package:

-> APPROVED

Comment 17 Andreas Osowski 2009-04-29 05:00:36 UTC
thanks a lot for your review


New Package CVS Request
=======================
Package Name: razertool
Short Description: Tool for controlling Razer Copperhead mice
Owners: th0br0
Branches: F-10 F-11
InitialCC:

Comment 18 Kevin Fenzi 2009-04-30 04:46:51 UTC
cvs done.

Comment 19 Fedora Update System 2009-04-30 06:41:09 UTC
razertool-0.0.7-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/razertool-0.0.7-5.fc10

Comment 20 Fedora Update System 2009-04-30 06:51:26 UTC
razertool-0.0.7-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/razertool-0.0.7-5.fc11

Comment 21 Fedora Update System 2009-05-25 21:20:41 UTC
razertool-0.0.7-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 22 Fedora Update System 2009-05-28 08:02:59 UTC
razertool-0.0.7-5.fc10 has been pushed to the Fedora 10 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.