Red Hat Bugzilla – Bug 223633
Review Request: csync2 - A cluster synchronization tool
Last modified: 2011-02-15 14:30:23 EST
Spec URL: http://rubenkerkhof.com/packages/csync2.spec
SRPM URL: http://rubenkerkhof.com/packages/csync2-1.33-1.src.rpm
Csync2 is a cluster synchronization tool. It can be used to keep files on
multiple hosts in a cluster in sync. Csync2 can handle complex setups with
much more than just 2 hosts, handle file deletions and can detect conflicts.
It is expedient for HA-clusters, HPC-clusters, COWs and server farms.
The fact that you install csync2.xinetd makes me think that you must
Also, since the policy in Fedora is to NOT install services in a
configuration which leaves them activated by default, I think that you should
patch csync2.xinetd and replace "disable=no" with "disable=yes", so that it
would not automatically start when the next service xinetd start/restart occurs.
The admin should specifically activate the new service.
>>The fact that you install csync2.xinetd makes me think that you must require xinetd.
>> Also, since the policy in Fedora is to NOT install services in a
>> configuration which leaves them activated by default, I think that you should
>> patch csync2.xinetd and replace "disable=no" with "disable=yes", so that it
>> would not automatically start when the next service xinetd start/restart occurs.
>> The admin should specifically activate the new service.
Fixed as well.
- rpmlint checks do not return anything either on the source or on the binary rpm
- package meets naming guidelines
- package meets packaging guidelines
- buildroot has the recommended value
- license ( GPL ) is OK, text in %doc, matches source
- spec file legible, in am. english
- source is last version and matches upstream, sha1sum
- package builds in mock for devel (i386) and FC6 (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates, does not own foreign files or directories
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- available documentation is included, but small so no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- no static, headers, libtool or .pc files
- builds fine in mock (FC6/x86_64, devel/i386)
- runs OK on FC6/x86_64 (once the proper instructions -- included in the rpm --
are followed, that is csync2 is added to /etc/services)
Suggestion: the changelog entry should describe the actual modifications
(especially since they are small - a one liner should suffice) rather then point
to a bugzilla entry (it makes life simpler for someone examining the rpms,
avoiding the need for an extra web query; not to mention that bugzilla might be
down or simply not accessible)
As a sidenote, you could have used a 1 line sed command in %install in order to
modify the offending line in xinetd.conf, rather then creating a separate patch,
but your method is perfectly fine.
In this moment I see no blockers, but before approving this package I would
like for someone more experienced to comment on the following:
In order to start the program (via xinetd), /etc/services must be modified,
adding a line specifying the port and protocol used by csync2 and SSL
certificates must be generated for each box where the program will run
Editing /etc/services could be handled either in a %post entry (in the sources
there is a spec file which does the addition, but does not verify if the entry
already exists and neither removes it at uninstall time), or (as the packager
has chosen) manually by the admin.
My question is: which one of this two methods is the proper procedure for
Thanks for the review Manuel.
My comments inline:
>> As a sidenote, you could have used a 1 line sed command in %install in
>> order to modify the offending line in xinetd.conf, rather then creating a
>> separate patch, but your method is perfectly fine.
I actually changed 2 lines in csync2.xinetd ;-) I prefer clean patches to
regular expressions in spec files. That way, if something changes in the
source, the patch fails to apply and I can fix it.
>> In order to start the program (via xinetd), /etc/services must be modified,
>> adding a line specifying the port and protocol used by csync2 and SSL
>> certificates must be generated for each box where the program will run
There's no need to edit /etc/services, since xinetd doesn't require it.
I tried creating the ssl certificates in %install (make cert does this), but
in mock, openssl seems to have problems opening a random device. I'm not sure
what's the best way to handle this. Maybe just ship a self-signed cert with
I fully agree with clean patches, but for very small modifications I prefer
one liners to separate patches. And since you could have done "s/default
on/default off; s/disable=no/disable=yes" one line would have been enough :)
Serious stuff now:
In my case, the first attempt to run csync2 on FC6 (without editing
/etc/services) triggered this error:
Jan 24 03:27:18 wolfy Installed: xinetd.x86_64 2:2.3.14-8
Jan 24 03:27:19 wolfy Installed: csync2.x86_64 1.33-2.fc6
Jan 24 03:27:58 wolfy xinetd: Port not specified and can't find service:
csync2 with getservbyname
Jan 24 03:27:58 wolfy xinetd: xinetd Version 2.3.14 started with libwrap
loadavg labeled-networking options compiled in.
Jan 24 03:27:58 wolfy xinetd: Started working: 0 available services
As of creating the certs, I'd include the relevant part of the Makefile
rather then creating the certificates by default. Maybe because I generate ALL
certificates in only one place and transfer them wherever they are needed...
man xinetd(8): xinetd is not limited to services listed in /etc/services.
Therefore, anybody can use xinetd to start special-purpose servers.
But for that to work, you probably have to set "type" to UNLISTED.
I'm at work now, but I'll see what I can come up with tonight.
Just tested. One can either edit /etc/services or add the following two lines to
type = UNLISTED
port = <desired port value>
Which makes me suggest the following
- modify a bit your patch as to include these two lines, using the default port
value assumed by csync2 ( 30865 )
- add a README.FEDORA pointing this difference towards the included docs
- bug upstream to modify the docs (the paper.pdf)
What do you think ?
Sounds good to me. Will do that tonight.
One rpmlint error though because I changed permissions to 640 on the ssl key, but that's a lot safer than
I am sorry, but I definitely MUST veto the inclusion of certificates in the
binary rpm. The reason is that this would lead to each and every computer where
the rpm is installed to have the very same certificates. Which kind of defeats
the whole purpose of certificates.
If you want to help the users, you could use for instance one of the
- extract the certificate generation part from the Makefile included in sources
and save it as a separate mkcert.sh
- install this file as a %doc
- document it in the README.FEDORA file I have suggested in comment #7
- generate the certificates in %post
Personally I believe that the first approach is better, allowing for the
admins to have the certificates generated only if they feel like needing them.
You could take a look at mod_ssl and dovecot. The first one includes the
directories where Apache SSL certificates are to be stored, as well as the
scripts needed to generate the certificates, but does not include the
certificates themselves. In addition a set of sample certificates are generated
at install time. A cleaner approach (in my opinion...) is used by dovecot which
includes a dovecot-openssl.cnf file (in /etc/pki) and a mkcert.sh script
(included as doc/sample).
I had a look at how dovecot did it, and that's definitely a better approach
Added a mkcert.sh script and a README.fedora file with a short Getting Started text.
Excelent job. All tests pass as per the original review (comment #3), the
package builds OK (as before) in mock for FC6/x86_64, the new sources are
installed cleanly, all permissions are sane, changelog correctly reflects all
This package is APPROVED by me.
Thanks for the review Manuel.
*** This bug has been marked as a duplicate of bug 676187 ***