Bug 487114

Summary: Review Request: gvrpcd - A program for announcing VLANs using GVRP.
Product: [Fedora] Fedora Reporter: Jasper Capel <capel>
Component: Package ReviewAssignee: Michael Schwendt <bugs.michael>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: 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
Spec URL: http://bender.newnewyork.nl/review/gvrpcd.spec
SRPM URL: http://bender.newnewyork.nl/review/gvrpcd-1.1-1.fc11.src.rpm

Description:
gvrpcd implements end-node GVRP functionality as a user-space daemon.
It generates periodically GVRP "JOIN" packets with information about
VLANs that server has defined on given NIC, or more generically - that it
wants to be able to connect to.

The patch contains a small Makefile modification, a sysconfig-file for specifying the interface it operates on, and an init-script. I've sent this patch upstream.

Comment 1 Jasper Capel 2009-02-25 10:50:14 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

Comment 2 Michael Schwendt 2009-02-28 09:14:52 UTC
* 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.

Comment 3 Jasper Capel 2009-02-28 11:06:04 UTC
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?

Comment 4 Michael Schwendt 2009-03-02 08:26:01 UTC
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.

Comment 5 Jasper Capel 2009-03-03 18:58:39 UTC
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

Comment 6 Michael Schwendt 2009-03-05 12:51:38 UTC
> 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]

Comment 7 Jasper Capel 2009-03-05 13:13:02 UTC
(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.

Comment 8 Michael Schwendt 2009-03-05 19:07:49 UTC
Does it make a difference whether a service starts multiple processes or multiple threads? I don't think so.

Comment 9 Jasper Capel 2009-04-03 13:14:36 UTC
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

Comment 10 Michael Schwendt 2009-04-18 09:44:08 UTC
* 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

Comment 11 Jasper Capel 2009-04-24 09:51:16 UTC
(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

Comment 12 Jasper Capel 2009-05-13 19:00:13 UTC
I was wondering if there are still any issues that's keeping this from being accepted as a new package. :)

Comment 13 Jeroen van Meeuwen 2009-05-13 19:10:03 UTC
(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.

Comment 14 Jasper Capel 2009-05-14 07:04:14 UTC
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

Comment 15 Michael Schwendt 2009-05-14 07:59:01 UTC
Sorry, have been caught in other activities as well as duties and neglected my review folder due to that.

Comment 16 Jasper Capel 2009-05-14 09:59:01 UTC
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

Comment 17 Michael Schwendt 2009-05-15 08:37:16 UTC
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

Comment 18 Jasper Capel 2009-05-15 09:31:39 UTC
Thanks. :)

I'll change the requires too, it's nicer to depend on the binaries I actually need than on package names.

Comment 19 Jasper Capel 2009-05-15 09:32:38 UTC
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:

Comment 20 Kevin Fenzi 2009-05-15 23:42:11 UTC
cvs done.

Comment 21 Fedora Update System 2009-05-16 15:24:51 UTC
gvrpcd-1.3-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/gvrpcd-1.3-2.fc11

Comment 22 Fedora Update System 2009-05-16 15:25:58 UTC
gvrpcd-1.3-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/gvrpcd-1.3-2.fc10

Comment 23 Fedora Update System 2009-05-19 02:00:09 UTC
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.

Comment 24 Fedora Update System 2009-05-19 02:02:04 UTC
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.