Bug 487114
Summary: | Review Request: gvrpcd - A program for announcing VLANs using GVRP. | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jasper Capel <capel> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | bugs.michael, fedora-jasper, fedora-package-review, notting, vanmeeuwen+fedora |
Target Milestone: | --- | Flags: | bugs.michael:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 1.3-2.fc11 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-05-19 02:00:15 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: |
Description
Jasper Capel
2009-02-24 10:24:33 UTC
Updated files, made it respect the global compiler flags, and preserve timestamps when installing files. Spec URL: http://bender.newnewyork.nl/review/gvrpcd.spec SRPM URL: http://bender.newnewyork.nl/review/gvrpcd-1.1-2.fc11.src.rpm * Patches have been merged upstream into gvrpcd-1.2, which does not exist as tarball release yet, though. Quite a lot of issues with this package: * %{_sysconfdir}/sysconfig/gvrpcd - must not be executable - is a configuration file that ought to be marked %config(noreplace) - defaults to "eth0" (why?) - the default is also a bit strange considering that the initscript defaults to eth0 already https://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files * rpmlint /home/qa/tmp/rpm/RPMS/gvrpcd-1.1-2.fc10.i386.rpm gvrpcd.i386: E: script-without-shebang /etc/sysconfig/gvrpcd gvrpcd.i386: E: init-script-without-chkconfig-postin /etc/rc.d/init.d/gvrpcd gvrpcd.i386: E: init-script-without-chkconfig-preun /etc/rc.d/init.d/gvrpcd gvrpcd.i386: W: service-default-enabled /etc/rc.d/init.d/gvrpcd 1 packages and 0 specfiles checked; 3 errors, 1 warnings. Run "rpmlint -i ..." on the built rpms for helpful explanations. * Documentation for initscripts can be found here: https://fedoraproject.org/wiki/Packaging/SysVInitScript There even is a template you ought to use. * Upstream is advised to include a proper GPLv2 header as suggested in the GPL itself. The way it is done currently in gvrpcd.c is doubtful, because it is a "source" file and the header refers to "this license document". It doesn't even mention a link to the GPL. * The README points to http://wiki.ethereal.com/GVRP which gives 404 Not Found. * With defaults, but no /proc/net/vlan/config because of disabled VLAN interfaces, the daemon logs "fopen: /proc/net/vlan/config: No such file or directory", says it started successfully, but actually has terminated (gvrpcd dead but subsys locked). Shutting down prints FAILED, but logs "shutdown succeeded" and runs into the 8 seconds sleep. Thanks. I'll have another go at it. Still in the process of learning. :) Upstream modified the init-script I used a bit, so it works on other distributions as well, and asked me to test it. I'll do so, and ask him to release after I did. It deviates more from the Fedora SysV guidelines, so I'll make a new, proper one, and include that by patching the sources. Your last remark, about when /proc/net/vlan/config does not exist, is that something that I should test for in the init-script, or should this be fixed another way? Often daemon programs run into error conditions at startup, but too late to return a proper error code from the daemonised process. All you get is the parent's return code (and maybe a log message). This could be fixed in the program itself and is no packaging problem. Existance of the /proc/net/vlan/config file is mandatory, however. The daemon terminates early with return code 1 if it cannot open that file. Therefore it would be good to add a safety check to the initscript, so it refuses to start if the VLAN config cannot be loaded at all. I created a new init-script from scratch, this one should be compliant. I'll advise upstream about the license, and the broken link in the README (I think this should be http://wiki.wireshark.org/GVRP). I added some comments to the sysconfig-file, and commented the option-lines, so they should now only be set if a user wants to override defaults set in the init script. rpmlint does still complain: gvrpcd.x86_64: W: service-default-enabled /etc/rc.d/init.d/gvrpcd It made sense to me to enable gvrpcd by default when installed - it is not a network daemon listening on any sockets, so it shouldn't be a security risk. On the other hand, if it's not started the server might not even be able to communicate. If you don't share this view, I'll disable it as it's not a big deal to enable the daemon by hand either. :) gvrpcd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/gvrpcd $prog I think this is because of the $prog, instead of gvrpcd in the init-script and can be ignored. New files: SRPM: http://bender.newnewyork.nl/review/gvrpcd-1.1-3.fc11.src.rpm SPEC: http://bender.newnewyork.nl/review/gvrpcd.spec > gvrpcd.x86_64: W: service-default-enabled /etc/rc.d/init.d/gvrpcd This is acceptable. Especially due to the new initscript, which exits early when not finding a vlan config. > gvrpcd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/gvrpcd $prog A false positive as explained by rpmlint -i. If you have strong feelings about it, you can define lockfile=/var/lock/subsys/gvrpcd directly after the exec=/prog= definitions. I mean, it's not that you would change this value often, so reusing $prog during definition of lockfile is not much of a benefit. About the defaults in the initscript, this is less than ideal. Currently, there are _three_ places where configuration values are defined: 1) the daemon's built-in defaults (time=3, eth0, /proc/...) 2) your initscript defaults (time=3, eth0, /proc/...) 3) the sysconfig file (commented out) Isn't that overhead? Wouldn't it be better to use the daemon's defaults and let /etc/sysconfig/gvrpcd contain only GVRPCD_OPTIONS= or GVRPCD_OPTIONS="-i eth0" plus a comment on "gvrpcd -h" (or a future manual page)? In the initscript, you would simply source /etc/sysconfig/gvrpcd and add $GVRPCD_OPTIONS to the daemon's required -d arg. Much more simpler to rely on the daemon's defaults (also explained in -h output), which may change. You would not have multiple places that refer to different defaults. > Usage: gvrpcd [-dvh] [-f configfile] [-d iface] [-i time] This is wrong. Should be: ... [-i iface] [-t time] (In reply to comment #6) > > > gvrpcd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/gvrpcd $prog > > A false positive as explained by rpmlint -i. If you have strong feelings about > it, you can define > > lockfile=/var/lock/subsys/gvrpcd > > directly after the exec=/prog= definitions. I mean, it's not that you would > change this value often, so reusing $prog during definition of lockfile is not > much of a benefit. > True, I'll change it so rpmlint will shut up. :) > > About the defaults in the initscript, this is less than ideal. Currently, there > are _three_ places where configuration values are defined: > > 1) the daemon's built-in defaults (time=3, eth0, /proc/...) > 2) your initscript defaults (time=3, eth0, /proc/...) > 3) the sysconfig file (commented out) > > Isn't that overhead? Wouldn't it be better to use the daemon's defaults and let > /etc/sysconfig/gvrpcd contain only > > GVRPCD_OPTIONS= > > or > > GVRPCD_OPTIONS="-i eth0" > > plus a comment on "gvrpcd -h" (or a future manual page)? In the initscript, > you would simply source /etc/sysconfig/gvrpcd and add $GVRPCD_OPTIONS to the > daemon's required -d arg. Much more simpler to rely on the daemon's defaults > (also explained in -h output), which may change. You would not have multiple > places that refer to different defaults. > Ok, makes sense to me. It should be noted that upstream adapted my previous init-scripts to support spawning multiple daemons (one for each interface you want the daemon to send on). It does this, by making $INTERFACES a list. I think we'll want to support this as well. The reason I haven't yet, is because I still have to figure out the way in which this is acceptable in Fedora. To keep this option open, I think we should go for setting $INTERFACES, and putting the other parameters in $GVRPCD_OPTIONS. > > > Usage: gvrpcd [-dvh] [-f configfile] [-d iface] [-i time] > > This is wrong. Should be: ... [-i iface] [-t time] I'll patch it and send it upstream. Does it make a difference whether a service starts multiple processes or multiple threads? I don't think so. OK, it's been a while, I was distracted by other projects, but I have some results: * Upstream changed license header, included COPYING file * README corrected * Help text corrected * Init script modified to support spawning multiple daemons http://bender.newnewyork.nl/review/gvrpcd.spec http://bender.newnewyork.nl/review/gvrpcd-1.2-1.fc11.src.rpm * Source tarball URL gives 404 Not Found. It's missing the "gvrpcd" directory. Correct would be: Source0: http://sokrates.mimuw.edu.pl/~sebek/%{name}/%{name}-%{version}.tar.gz * A proper exit(..) call at end of main() would eliminate the following compiler warning: gvrpcd.c:290: warning: control reaches end of non-void function * "Short-Description" in initscript contains a typo: s/annoucing/announcing/ * You've still got the start-by-default problem (see bottom of comment 2) where the daemon is started even if VLAN configuration is missing: $ sudo service gvrpcd start Starting gvrpcd-eth0: [ OK ] $ sudo service gvrpcd status gvrpcd-eth0 dead but pid file exists (In reply to comment #10) > * Source tarball URL gives 404 Not Found. It's missing the "gvrpcd" directory. > Correct would be: > > Source0: http://sokrates.mimuw.edu.pl/~sebek/%{name}/%{name}-%{version}.tar.gz > Fixed. > * A proper exit(..) call at end of main() would eliminate the following > compiler warning: > gvrpcd.c:290: warning: control reaches end of non-void function > > I don't know C, if this is a blocker, could you propose a patch? Alternatively, I could ask upstream for a patch, but I think it'd be nicer if we just come up with the patch. > * "Short-Description" in initscript contains a typo: s/annoucing/announcing/ > > Fixed. > * You've still got the start-by-default problem (see bottom of comment 2) where > the daemon is started even if VLAN configuration is missing: > > $ sudo service gvrpcd start > Starting gvrpcd-eth0: [ OK ] > $ sudo service gvrpcd status > gvrpcd-eth0 dead but pid file exists Oops, missed that. $config was undefined, fixed now. http://bender.newnewyork.nl/review/gvrpcd-1.2-2.fc11.src.rpm http://bender.newnewyork.nl/review/gvrpcd.spec I was wondering if there are still any issues that's keeping this from being accepted as a new package. :) (In reply to comment #11) > (In reply to comment #10) > > * A proper exit(..) call at end of main() would eliminate the following > > compiler warning: > > gvrpcd.c:290: warning: control reaches end of non-void function > > > > > I don't know C, if this is a blocker, could you propose a patch? Alternatively, > I could ask upstream for a patch, but I think it'd be nicer if we just come up > with the patch. > Handed Jasper a patch adding a line exit(do_shutdown); to the end of main(). do_shutdown is the signal value gvrpcd uses to indicate what type of signal it got telling it to terminate so that's what I'm using here. Applied the patch, and sent it upstream. New files: http://bender.newnewyork.nl/review/gvrpcd-1.2-3.fc11.src.rpm http://bender.newnewyork.nl/review/gvrpcd.spec Sorry, have been caught in other activities as well as duties and neglected my review folder due to that. Upstream released 1.3 including the patch (and some other code cleanup), updated files: http://bender.newnewyork.nl/review/gvrpcd-1.3-1.fc11.src.rpm http://bender.newnewyork.nl/review/gvrpcd.spec Haven't found further issues. Instead of depending on "chkconfig" and "initscripts", you can prefer a dependency on /sbin/chkconfig and /sbin/service instead, since you execute those tools with absolute path. The dependency on those file locations is not more expensive than requiring the package names. The file Provides for files in many (or all) *bin* paths as well as files in /etc are covered by the primary metadata file. More packages do it already, too. $ repoquery --whatrequires /sbin/chkconfig|wc -l 364 $ repoquery --whatrequires chkconfig|wc -l 202 APPROVED : gvrpcd-1.3-1.fc11.src.rpm Thanks. :) I'll change the requires too, it's nicer to depend on the binaries I actually need than on package names. New Package CVS Request ======================= Package Name: gvrpcd Short Description: A program for announcing VLANs using GVRP. Owners: jasper Branches: EL-5, F-10, F-11 InitialCC: cvs done. gvrpcd-1.3-2.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/gvrpcd-1.3-2.fc11 gvrpcd-1.3-2.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/gvrpcd-1.3-2.fc10 gvrpcd-1.3-2.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report. gvrpcd-1.3-2.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report. |