Bug 218408 - Review Request: xl2tpd - replacement of l2tpd
Review Request: xl2tpd - replacement of l2tpd
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-05 00:36 EST by Paul Wouters
Modified: 2014-07-28 15:14 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-07 13:12:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Diff to xl2tpd.spec (1.27 KB, patch)
2006-12-07 12:37 EST, Mamoru TASAKA
no flags Details | Diff
l2tpd -> xl2tpd log (13.26 KB, text/plain)
2006-12-07 12:47 EST, Mamoru TASAKA
no flags Details

  None (edit)
Description Paul Wouters 2006-12-05 00:36:20 EST
Spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-1.src.rpm
Description: xl2tpd is an implementation of the Layer 2 Tunnelling Protocol (RFC 2661).
L2TP allows you to tunnel PPP over UDP. Some ISPs use L2TP to tunnel user
sessions from dial-in servers (modem banks, ADSL DSLAMs) to back-end PPP
servers. Another important application is Virtual Private Networks where
the IPsec protocol is used to secure the L2TP connection (L2TP/IPsec,
RFC 3193). The L2TP/IPsec protocol is mainly used by Windows and 
Mac OS X clients. On Linux, xl2tpd can be used in combination with IPsec
implementations such as Openswan.
Example configuration files for such a setup are included in this RPM.

xl2tpd works by opening a pseudo-tty for communicating with pppd.
It runs completely in userspace.
Comment 1 Mamoru TASAKA 2006-12-05 00:37:45 EST
I will review this after I go back from shopping
Comment 2 Mamoru TASAKA 2006-12-05 00:49:05 EST
Well, before I go out for shopping:

I tried to rebuild in mockbuild and rpmlint complains about:
---------------------------------------
E: xl2tpd tag-not-utf8 %changelog
E: xl2tpd obsolete-not-provided l2tpd
E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
W: xl2tpd dangerous-command-in-%post mv
E: xl2tpd no-chkconfig-line /etc/rc.d/init.d/xl2tpd
E: xl2tpd tag-not-utf8 %changelog
E: xl2tpd non-utf8-spec-file xl2tpd.spec
W: xl2tpd unversioned-explicit-obsoletes l2tpd
W: xl2tpd mixed-use-of-spaces-and-tabs (spaces: line 65, tab: line 52)
E: xl2tpd-debuginfo tag-not-utf8 %changelog
------------------------------------------------------------
Please fix these or explain the reason when these rpmlint message
can be ignored.

Also, please add %?dist tag (I have not yet checked this package,
I only rebuilt this)
Comment 3 Paul Wouters 2006-12-05 01:09:49 EST
* Tue Dec  5 2006 Paul Wouters <paul@xelerance.com> 1.1.06-2
- Changed Mr. Karlsen's name to not be a utf8 problem
- Fixed Obosoletes/Provides to be more specific wrt l2tpd.

Spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-2.src.rpm
Comment 4 Paul Wouters 2006-12-05 02:11:23 EST
two comments of me got lost, and i think two of you :(

I aded the dist tag. fixes the spaces, and the obsolete/provides issue in the
above srpm.
Comment 5 Mamoru TASAKA 2006-12-05 07:09:21 EST
Well, again full review of this package.

A. From http://fedoraproject.org/wiki/Packaging/Guidelines :
* rpmlint:
- rpmlint output is not silent.
-----------------------------------------------------
1. E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
2. E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
3. W: xl2tpd dangerous-command-in-%post mv
4. E: xl2tpd no-chkconfig-line /etc/rc.d/init.d/xl2tpd
-----------------------------------------------------
1. 2. : I don't know well about this package, however,
        please check if the permisson of these files are
        correct.
3. : I think that this warnings may ignorable, however:
     * By the way which is done first, doing %post scriptlet
       of xl2tpd, or obsoleting (i.e. deleting) xl2tpd?
       I vaguely remember several case (by yum) that
       obsoleting a package is done _before_ installing
       a substitute rpm.

       I think moving or copying should be done by 
       %triggerun
4. : This error can be suppressed by replacing tab with space
     in '# chkconfig:    - 80 30' line.

* Timestamps
- Use 'install -p' to keep timestamps.
  And I recommend that Makefile should be also fixed to
  use 'install -p'.

B. http://fedoraproject.org/wiki/Packaging/ScriptletSnippets :
* Initscripts Conventions
  - Please add the appropriate requirement in
    Requires(post), etc.

C. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
   (Okay).
Comment 6 Paul Wouters 2006-12-05 22:58:45 EST
spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-3.src.rpm

* Tue Dec  5 2006 Paul Wouters <paul@xelerance.com> 1.1.06-3
- Added Requires(post) / Requires(preun)
- changed init file to create /var/run/xl2tpd, fixed a tab/space
- changed control file to be within /var/run/xl2tpd/

1/2 is correct. The secrets should only be readable by root
3 I've tested migration and the order is correct. the orignally installed
  xl2tpd configs are kept as rmpsave files, the ones from /etc/l2tpd/ are
  migrated into /etc/xl2tpd/, and then l2tpd gets uninstalled, and creates
  its own rpmsave files if needed.
4 done
Timestamps: I am not sure why we don't want to show the timestamps of the
package build?
B: done
Comment 7 Mamoru TASAKA 2006-12-06 09:54:13 EST
Before checking 1.1.06-3:

(In reply to comment #6)
> Timestamps: I am not sure why we don't want to show the timestamps of the
> package build?

I always strongly recommend to keep timestamps as:
- They show if vendor (like you) have added some modifications against
  the original sources.
- They show when the files are actually created. 

These are especially important when binary rpms contain a lot of
header files, image files, documentations, etc....
Setting the timestamps of these files to build time, not the time
when the contents of the files are actually changed confuses people
when they want to check how those files are changed.

Comment 8 Mamoru TASAKA 2006-12-06 11:52:04 EST
Well,

* Please keep timestamps, as I said in comment #7 and the section
  'timestamps' in http://fedoraproject.org/wiki/Packaging/Guidelines
  Please use 'install -p'. Timestamps should be changed when the
  contents of the files are actually changed, this should be especially
  for text files.

  (In reply to comment #5)
* I still get the rpmlint complaint:
------------------------------------------
E: xl2tpd no-chkconfig-line /etc/rc.d/init.d/xl2tpd
------------------------------------------
> 4. : This error can be suppressed by replacing tab with space
>      in '# chkconfig:    - 80 30' line.
Well, what I wanted to say by this is: "please use space in 
'#chkconfig: - 80 30' line, not tab.

> B. http://fedoraproject.org/wiki/Packaging/ScriptletSnippets :
> * Initscripts Conventions
>   - Please add the appropriate requirement in
>     Requires(post), etc.
Well, requirement for /sbin/service is still missing

(In reply to comment #6)
> 3 I've tested migration and the order is correct. the orignally installed
>   xl2tpd configs are kept as rmpsave files, the ones from /etc/l2tpd/ are
>   migrated into /etc/xl2tpd/, and then l2tpd gets uninstalled, and creates
>   its own rpmsave files if needed.

I still have a concern about the case in which some people may
customize original l2tpd rpm and reinstall it with release number
incremented by themself.
In this case, l2tpd is not uninstalled, which is correct that 
customizing by themself means that they don't want to have l2tpd
automatically removed by xl2tpd. However, current %post scripts
anyway 'moves' files in l2tpd, then l2tpd is installed with some
files missing.

And... I don't know about l2tpd, however, if removing l2tpd is
done after installing x12tpd, then there arises a moment anyway 
when l2tpd is installed while some files in l2tpd is missing.
In the case uninstalling l2tpd exits with non-zeron status and
it won't be completed.

This state is critical when these files are 'required' for 
uninstalling l2tpd. Even if not, removing files in l2tpd should
be done when l2tpd is exactly about being removed.
Comment 9 Paul Wouters 2006-12-06 15:59:55 EST
spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-4.src.rpm

- I still prefer to see the build timestamps compared to the original tar ball
  timestampts. But I added -p, beacause I just want to get over this.
- the space vs tab issue was still there because that error is in the original
  tarball. I dont want to release the usptream version just for this, so I
  added a temp sed line to the install process.
  writing a patch to just cover a week before that release seems a bit overkill.
- Added /sbin/service requirement.
- as for the "mv". Please give me another method to distinguish between an
upgrade from l2tpd->xl2tpd and one from xl2tpd->xl2tpd. AFAIK, I cannot tell in
any scriplets. If I use "mv", the migration will not happen more then once. If I
use "cp", it will be possible to happen more then once. Alternatively, if you
are that worried I can leave out the entire migration and let users do it
manually, but then it will surely need human intervention after installing the
upgrade.

AS for your last notes, as I told before, there are NO errors when doing an
l2tpd -> xl2tpd upgrade. l2tpd uninstalls fine. xl2tpd installs fine. Please
test if you do not believe me.

What's left now is:
[root@bofh devel]# rpmlint /usr/src/redhat/RPMS/x86_64/xl2tpd-1.1.06-4.x86_64.rpm
E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
W: xl2tpd dangerous-command-in-%post mv
[root@bofh devel]# rpmlint /usr/src/redhat/SRPMS/xl2tpd-1.1.06-4.src.rpm 

the secrets are supposed to be root-only readable.
the "mv" is discussed above.
Comment 10 Mamoru TASAKA 2006-12-07 12:37:54 EST
Created attachment 143068 [details]
Diff to xl2tpd.spec

(In reply to comment #9)
> - the space vs tab issue was still there because that error is in the
original
>   tarball. I dont want to release the usptream version just for this, so I
>   added a temp sed line to the install process.
For this purpose, applying the patch attached will be more
smart.
Comment 11 Mamoru TASAKA 2006-12-07 12:47:45 EST
Created attachment 143071 [details]
l2tpd -> xl2tpd log

> 
> What's left now is:
> [root@bofh devel]# rpmlint
/usr/src/redhat/RPMS/x86_64/xl2tpd-1.1.06-4.x86_64.rpm
> E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
> E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
> W: xl2tpd dangerous-command-in-%post mv
> [root@bofh devel]# rpmlint /usr/src/redhat/SRPMS/xl2tpd-1.1.06-4.src.rpm 
> 
> the secrets are supposed to be root-only readable.
> the "mv" is discussed above.

I checked this for to make sure that your script works,
and it is okay (at least it seems okay for me).

Please check if the log attached is what you expect.
Comment 12 Mamoru TASAKA 2006-12-07 12:51:25 EST
Now I approve this package

-------------------------------------------------------
  This package (xl2tpd) is APPROVED by me.
-------------------------------------------------------

Please consider the patch for dealing with init file tab issue
attached on comment #9 .
Comment 13 Paul Wouters 2006-12-07 12:53:43 EST
looks good to me.

I used your sed command

spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-5.src.rpm
Comment 14 Mamoru TASAKA 2006-12-07 13:09:29 EST
Please change the status to CLOSED NEXTRELEASE when
rebuilding for FE-devel is done.
Comment 15 Paul Wouters 2014-07-28 15:06:22 EDT
Package Change Request
======================
Package Name: xl2tpd
New Branches: epel7
Owners: pwouters
Comment 16 Jon Ciesla 2014-07-28 15:14:20 EDT
Git done (by process-git-requests).

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