Bug 165314 - Review Request: kismet -- A WLAN detector, sniffer and IDS
Review Request: kismet -- A WLAN detector, sniffer and IDS
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
David Lawrence
http://www.kismetwireless.net
:
: 173182 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-07 13:41 EDT by Enrico Scholz
Modified: 2007-11-30 17:11 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-04-29 13:57:20 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Updated -alias patch which fixes compilation on x86_64 (9.50 KB, patch)
2006-04-26 01:11 EDT, Hans de Goede
no flags Details | Diff
Patch fixing 2 warnings (and real problems) on x84_64 (1.15 KB, patch)
2006-04-26 01:14 EDT, Hans de Goede
no flags Details | Diff

  None (edit)
Description Enrico Scholz 2005-08-07 13:41:03 EDT
Spec Name or Url: http://ensc.de/fedora/kismet.spec
SRPM Name or Url: http://ensc.de/fedora/kismet-0.0.2005.07.R1a-1.src.rpm
Description: 

Kismet is an 802.11 layer2 wireless network detector, sniffer, and
intrusion detection system. Kismet will work with any wireless card
which supports raw monitoring (rfmon) mode, and can sniff 802.11b,
802.11a, and 802.11g traffic.

Kismet identifies networks by passively collecting packets and detecting
standard named networks, detecting (and given time, decloaking) hidden
networks, and infering the presence of nonbeaconing networks via data
traffic.
Comment 1 Matthias Saou 2005-08-18 05:55:04 EDT
Just a few remarks :
- 2005-08-R1 is out and fixes important security issues.
- I find it really harder to read spec files where %variables aren't in
%{curly_braces} (breaks some displaying in vim). It also adds some possible
doubts about where the variable name stops.
- Why add the subst() function when perl or inplace sed edit works fine?
- I don't really like relying on the external "fedora-usermgmt" from the
fedora.us era for use creation/deletion. Also, where does the 12 come from and
does it guarantee there will be no clash?
- The name "IP sentinel user" for the user seems like a copy/paste mistake,
unless you're trying to share a user between multiple packages/daemons (which
IMHO should be avoided).
Comment 2 Enrico Scholz 2005-08-18 07:59:27 EDT
http://ensc.de/fedora/kismet.spec
http://ensc.de/fedora/kismet-0.0.2005.08.R1-1.src.rpm

=================

> - 2005-08-R1 is out and fixes important security issues.

thx; updated


> - I find it really harder to read spec files where %variables aren't
>   in %{curly_braces}

mmh... I find it really harder to read spec files where %variables are
in %{curly_braces} ;)


>   (breaks some displaying in vim).

I tested it and it seems that there is no difference between %var and
%{var} in vim.


>   It also adds some possible doubts about where the variable name
>   stops.

I took care about this and added braces where needed.


> - Why add the subst() function when perl or inplace sed edit works fine?

ok; removed it. It happens in %prep where the files *will* be modified
so that I do not need to keep the timestamp.


> - I don't really like relying on the external "fedora-usermgmt" from
>   the fedora.us era for use creation/deletion.

I do not see an alternative as I need consistent uids across different
systems.


>   Also, where does the 12 come from and does it guarantee there will
>   be no clash?

Ok; copied http://www.fedora.us/wiki/PackageUserRegistry to
http://fedoraproject.org/wiki/PackageUserRegistry and updated it.


> - The name "IP sentinel user" for the user seems like a copy/paste
>   mistake, unless you're trying to share a user between multiple
>   packages/daemons (which IMHO should be avoided).

oops; should be fixed now
Comment 3 Matthias Saou 2005-08-18 08:19:22 EDT
> >   (breaks some displaying in vim).
> 
> I tested it and it seems that there is no difference between %var and
> %{var} in vim.

That's because it isn't recognized as an existing base macro. Try %{_var} or
%{_localstatedir} and you'll see a different color.

> >   It also adds some possible doubts about where the variable name
> >   stops.
> 
> I took care about this and added braces where needed.

It's easier to just put them everywhere :-D

> > - I don't really like relying on the external "fedora-usermgmt" from
> >   the fedora.us era for use creation/deletion.
> 
> I do not see an alternative as I need consistent uids across different
> systems.

I'm not saying I'm against any fixed uid/gid scheme, just that I don't really
like adding a dependency on an external package instead os using plain
useradd/userdel.

> >   Also, where does the 12 come from and does it guarantee there will
> >   be no clash?
> 
> Ok; copied http://www.fedora.us/wiki/PackageUserRegistry to
> http://fedoraproject.org/wiki/PackageUserRegistry and updated it.

That page is definitely missing some information (which I don't know), as the
names there simply cannot have uids 1 to 12. I guess there is a fixed value
automatically added by the additional user management tools like 200 or 300? It
should definitely be added to that page.
From there, I'd really prefer simply putting the full real uid/gid to that page
and use plain useradd with it in packages. Maybe a discussion for
fedora-packaging list?
Comment 4 Enrico Scholz 2005-08-18 09:20:36 EDT
> > >   (breaks some displaying in vim).
> > 
> > I tested it and it seems that there is no difference between %var and
> > %{var} in vim.
> 
> That's because it isn't recognized as an existing base macro. Try %{_var} or
> %{_localstatedir} and you'll see a different color.

really, there is absolutely no difference between these variants. An
'vim /tmp/kismet.spec' gives

		      http://ensc.de/kismet.png

(vim-enhanced-6.3.071-3)


> > I do not see an alternative as I need consistent uids across different
> > systems.
> 
> I'm not saying I'm against any fixed uid/gid scheme, just that I
> don't really like adding a dependency on an external package instead
> os using plain useradd/userdel.

I could do it with %scriptlets also, but with errorhandling and
other features it would be very complicated. And somehow, I dislike
scriptlets with >100 lines...

I imported http://fedoraproject.org/wiki/PackageUserCreation also,
which explains this.


> Ok; copied http://www.fedora.us/wiki/PackageUserRegistry to
> http://fedoraproject.org/wiki/PackageUserRegistry and updated it.

That page is definitely missing some information (which I don't know),
as the names there simply cannot have uids 1 to 12. I guess there is a
fixed value automatically added by the additional user management
tools like 200 or 300? It should definitely be added to that page.

ok; I imported the other related pages from the fedora.us wiki

http://fedoraproject.org/wiki/PackageUserCreation
http://fedoraproject.org/wiki/PackageDynamicUserCreationConsideredBad


Some information are outdated but these are minor issues only and I do
not have time now to update it.



> From there, I'd really prefer simply putting the full real uid/gid
> to that page and use plain useradd with it in packages. Maybe a
> discussion for fedora-packaging list?

There was a recent discussion but without a result. Summarized, there
were three parties:

* those, who want really fixed UIDs (and ignore problems with existing
  systems)
* those, who want semi-static UIDs (the fedora-usermgmt way)
* those, who do not want a usercreation with deterministic UIDs

http://listman.redhat.com/archives/fedora-packaging/2005-July/msg00000.html
Comment 5 John Mahowald 2005-12-09 23:13:32 EST
*** Bug 173182 has been marked as a duplicate of this bug. ***
Comment 6 Hans de Goede 2006-01-31 16:21:45 EST
I might be willing todo a review, one gotcha I'll have to believe you on your
blue eyes that this works as I don't have a wireless LAN (or card).

Here's a couple of mustfixes for starters:
-make sure all the fedora-usermgmt Wiki pages are uptodate, also include a link
 to: http://listman.redhat.com/archives/fedora-packaging/2005-July/msg00000.html
 on atleast one of them and a link to to this bug under the kismet entry in
 the table.
-put a link to them on one of the main pages (scriptlets perhaps?) so that
 it can be found by navigating / google can find it, instead of only
 being able to get there by knowing the URL.
-use %{xxx} everywhere, I know this isn't in the guidelines but _everyone_
 does it (well appearantly not everyone).

Comment 7 Enrico Scholz 2006-03-12 19:01:38 EST
ok, I added some links to maillist discussions at
http://fedoraproject.org/wiki/Packaging/UserCreation

But the %xxx instead of %{xxx} will stay.
Comment 8 Hans de Goede 2006-03-13 00:24:43 EST
Hehe,

Just as stuborn as me, ok although you're still the first packager I've met who
doesn't always use the %{xxx} notation, have it your way.

The usercreation discussion indeed seems endless and fedora-usermanagment seems
like a clean way to me, so i see no more obstacles for starting a full review.

I'll try todo the full review soon, as time permits.
Comment 9 Ralf Corsepius 2006-03-13 02:37:00 EST
(In reply to comment #7)
> But the %xxx instead of %{xxx} will stay.
Please explain why.
Comment 10 Enrico Scholz 2006-03-13 03:49:25 EST
>> But the %xxx instead of %{xxx} will stay.
> Please explain why.

Because it is valid syntax and I like it.
Comment 11 Ralf Corsepius 2006-03-13 04:22:28 EST
(In reply to comment #10)
> >> But the %xxx instead of %{xxx} will stay.
> > Please explain why.
> 
> Because it is valid syntax and I like it.

Yes, it's valid syntax, ...

... but fully quoted expressions are much easier to parse and much less
error-prone to maintain than those using implicit short cuts.

Comment 12 Enrico Scholz 2006-03-13 05:11:07 EST
The same arguments apply to shell variables and although in cases like

| mkdir -p $RPM_BUILD_ROOT%_libdir

quoting is required, only are very small amount of people are doing it.


The quoting of macros in my spec files is absolutely correct, I like the
curly-less way and won't change it.


Sorry, I do not want to start yet another metadiscussion. Especially when your
side uses phrases like "much easier" and "much less" without telling numbers or
facts.
Comment 13 Ralf Corsepius 2006-03-13 08:05:10 EST
(In reply to comment #12)
> The same arguments apply to shell variables
Right.
 
> The quoting of macros in my spec files is absolutely correct, I like the
> curly-less way and won't change it.
Try to grep/sed to check/exchange for macros (or shell variables), you'd soon
recognize how easy it is to miss them because of lack of quotes.

> Sorry, I do not want to start yet another metadiscussion. Especially when your
> side uses phrases like "much easier" and "much less" without telling numbers 
> or facts.
Well, apparently everybody must commit each mistake themselves, before they can
accept others' advise. You're not the first person having the "sloppy-quoting"
learning curve ahead.



Comment 14 Hans de Goede 2006-03-16 12:39:12 EST
Okay, I took a serious look today. I must say I don't like your deviation from
the standard FE practices all the %global stuff is ugly and is clearly meant for
building outside the FE infrastructure, which is not something we wish to support.

However your other packages already in FE (and thus approved) use similar
constructions and if you ever orphan this package stripping this uglyness will
be easy enough. So concedering this and since besides your (smallish) FE
practices deviations you do great work I'll let this slip / be as is.

What however is a problem is the fact that the SRPM does not build as a normal user:

+ /usr/bin/make DESTDIR=/var/tmp/kismet-0.0.2005.08.R1-1-root install
/usr/bin/make -e commoninstall
make[1]: Entering directory `/usr/src/redhat/BUILD/kismet-2005-08-R1'
mkdir -p /var/tmp/kismet-0.0.2005.08.R1-1-root/etc/kismet
mkdir -p /var/tmp/kismet-0.0.2005.08.R1-1-root/usr/bin
install -o "root" -g "root" -m 755 scripts/kismet
/var/tmp/kismet-0.0.2005.08.R1-1-root/usr/bin/kismet
install: cannot change ownership of
`/var/tmp/kismet-0.0.2005.08.R1-1-root/usr/bin/kismet': Operation not permitted
make[1]: *** [commoninstall] Error 1
make[1]: Leaving directory `/usr/src/redhat/BUILD/kismet-2005-08-R1'

This also explains why I didn't see any special rights and user %attr under
%files. Please make the compile work as a normal user and use %attr todo _all_
the special stuff, so that one can see from the spec that this is a suid binary
which currently cannot be seen from the spec.

Also why exactly does kismet need its own user? I assume this is done so that
any possible exploits in kismet can't do much damage since the exploit will run
as user kismet?

And why does kismet have /var/lib/kismet as homedir? since its a disabled
account it doesn't need one couldn't you just create a /var/log/kismet, drop the
logs there and not under /var/lib/kismet/logs and also use /var/log/kismet as
the homedir? AFAIK there will be no files needed / created under the homedir so
there is no need for a seperate home and logs dir.

And whats with the gps being disabled with ./configure and then packaged as
-extras that doesn't make sense?


Comment 15 Enrico Scholz 2006-03-16 13:37:59 EST
* Thu Mar 16 2006 Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de> - 0.0.2005.08.R1-2
- set *USR and *GRP variables to avoid problems with certain 'install'
  versions

http://ensc.de/fedora/kismet.spec
http://ensc.de/fedora/kismet-0.0.2005.08.R1-2.src.rpm


========


> all the %global stuff is ugly

The %global is required because %define is buggy


> install -o "root" -g "root" -m 755 scripts/kismet
> /var/tmp/kismet-0.0.2005.08.R1-1-root/usr/bin/kismet
> install: cannot change ownership of ...

strange... my 'install' ignores this kind of errors:

| [ensc@kosh kismet]$ install -o root -g man kismet.spec /tmp/
| [ensc@kosh kismet]$ ll /tmp/kismet.spec 
| -rwxr-xr-x  1 ensc ensc 3944 16. Mär 19:13 /tmp/kismet.spec
| [ensc@kosh rpmspecs]$ rpm -qf $(which install)
| coreutils-5.2.1-48.1


> so that one can see from the spec that this is a suid binary

??? There should be no SUID binary in the 'kismet' package


> Also why exactly does kismet need its own user?

See section 6, "Suidroot & Security" in the shipped README


> And why does kismet have /var/lib/kismet as homedir?

kismet stores information about found networks under ~/.kismet (see
the 'configdir' variable in kismet.conf)


> And whats with the gps being disabled with ./configure and then
> packaged as -extras that doesn't make sense?

??? My spec file states

| %configure --enable-ipv6 --sysconfdir=%cfgdir

GPS support won't be disabled there.
Comment 16 Hans de Goede 2006-03-16 14:55:03 EST
(In reply to comment #15)
> > all the %global stuff is ugly
> 
> The %global is required because %define is buggy
> 

I wasn't talking about the use of %global vs %define, I was talking about the
version and release stuff, which is unnescesarry and very different from how the
rest of FE does it. You clearly have a different style and don't want to adapt 
(see curly braces discussion), since there is no hard required codingstyle for
FE like there is for the kernel I'll accept this, but I don't like it.
Now lets stop discussing this and focus on the stuff that matters.

> > so that one can see from the spec that this is a suid binary
> 
> ??? There should be no SUID binary in the 'kismet' package
> 

You're right I thought there was / would be because of the extra user now that
I've actually been able to build (and install) it my knowledge of kismet has
improved and much of the questions below have been answered already, non the
less thanks for the answer. 

> > Also why exactly does kismet need its own user?
> 
> See section 6, "Suidroot & Security" in the shipped README
> 

I already thougt as much.

> 
> > And whats with the gps being disabled with ./configure and then
> > packaged as -extras that doesn't make sense?
> 
> ??? My spec file states
> 
> | %configure --enable-ipv6 --sysconfdir=%cfgdir
> 
> GPS support won't be disabled there.
> 

My bad, gps support was disabled in a sed script not in the configure I now
understand the sed script only edits the default config file and does not impact
compilation.


> 
> > And why does kismet have /var/lib/kismet as homedir?
> 
> kismet stores information about found networks under ~/.kismet (see
> the 'configdir' variable in kismet.conf)
> 

Yes, thanks for the new spec much better. I do have one problem left I don't
like the /var/lib/kismet/.kismet and /var/lib/kismet/logs construction at all.

Since both the state (configdir configfile option) and the log directory can be
configured from the config file why not use:
/var/log/kismet for the logs
and:
/var/lib/kismet for the state (thus without the .kismet behind it)

Thats much cleaner IMHO. If we can get that one sorted out I'll do a full review
soon.
Comment 17 Enrico Scholz 2006-03-18 06:24:33 EST
* Fri Mar 17 2006 Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de> - 0.0.2005.08.R1-3
- fixed the usermgmt in the %postun script: test for uninstallation
  and swap order of user- and groupdel operations
- moved logs to /var/log/kismet
- placed status information directly under /var/lib/kismet instead of
  /var/lib/kismet/.kismet
- added /etc/cron.dail/tmpwatch.kismet to cleanup the generated
  logfiles; used tmpwatch because kismet creates new, differently
  named logfiles.
- added -jobcontrol patch


http://ensc.de/fedora/kismet.spec
http://ensc.de/fedora/kismet-0.0.2005.08.R1-3.src.rpm


===========

> I was talking about the version ... stuff,

This has not to do very much with personal style. It just deals with
the upstream version (2005-08-R1) which is not allowed for an rpm
version tag.
Comment 18 Hans de Goede 2006-04-08 02:38:57 EDT
Sorry I somehow never got a bugzilla mail on your update I'll do a full review
of your new version soon.
Comment 19 Hans de Goede 2006-04-22 09:22:44 EDT
Once again apologies for my slowness I was really swamped with work at work
lately so much I've even been working my weekends, anyways here's a full review
as promised:

MUST:
=====
* rpmlint output is:
W: kismet conffile-without-noreplace-flag /etc/cron.daily/tmpwatch.kismet
E: kismet non-standard-gid /etc/kismet kismet
E: kismet non-standard-dir-perm /etc/kismet 0750
E: kismet non-standard-gid /var/lib/kismet kismet
E: kismet non-standard-dir-perm /var/lib/kismet 0770
E: kismet executable-marked-as-config-file /etc/cron.daily/tmpwatch.kismet
E: kismet non-standard-gid /var/log/kismet kismet
E: kismet non-standard-dir-perm /var/log/kismet 0730
W: kismet log-files-without-logrotate /var/log/kismet
W: kismet-debuginfo dangling-relative-symlink
/usr/src/debug/kismet-2005-08-R1/libpcap-0.9.1-kis/bpf_filter.c
./bpf/net/bpf_filter.c
Most of these are OK / have a good reason, so they are ok, it would be nice if
you could fix the last one though, but that is not a blocker.
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-x86_64
* BR: ok (see below)
* No locales
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs (with some strangeness see Must fix below)
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* no gui -> no .desktop file required


MUST fix:
=========
*Summary must not start with "A ...." drop the "A " .
*These:
 Requires(pre):          %crontabdir
 Requires(postun):       %crontabdir

 Seem rather odd neither script does anything with cron, the packages does 
 install files in %crontabdir and does not own them, so the package should 
 plain require %crontabdir or better maybe just own it since you made the other
 related requires (missingok).

Should fix:
===========
*Redundant BR: ImageMagick-devel already requires libtiff-devel and
 libjpeg-devel, please remove those from the BR-s
*This comment:
 # set our 'kismet' user, disable GPS and log into ~kismet/logs by
 Is no longer correct as logs now go to /var/log/kismet
*These compiler warnings look really suspicious:
gpsmap.cc:868: warning: comparison is always true due to limited range of data 
gpsmap.cc:876: warning: comparison is always true due to limited range of data 

Nice to have:
=============
* Upstream has a much newer version, it would be nice to upgrade to that 
  version.


Thats it, if you fix the 2 MUST fix items and post a new spec / srpm I'll aprove
it ASAP.
Comment 20 Enrico Scholz 2006-04-23 07:23:25 EDT
* Sat Apr 22 2006 Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de> - 0.0.2006.04.R1-1
- updated to 2006-04-R1
- fixed/improved some ./configure checks
- removed the starting 'A' from the summary
- added a bunch of patches fixing compiler warnings

http://ensc.de/fedora/kismet/kismet.spec
http://ensc.de/fedora/kismet/

===========

> *Summary must not start with "A ...." drop the "A " .

I could not find such a rule in the packaging guidelines and from my
linguistic feeling, a leading "A " sounds better.

But ok; I am not a native english speaker so I believe you and removed
the "A ".


> *These:
>  Requires(pre):          %crontabdir
>  Requires(postun):       %crontabdir

... are required resp. the best current way to express:

* the directory must exist before the package places files into it. Else,
  when the directory is a symlink (e.g. compare /etc/init.d) in the owning
  package, you will create oddities.

* the package must be removed before the directory. Else, the directory
  can not be removed because it still contains files from 'kismet' and
  becomes orphaned.  Therefore, a strict '%crontabdir -> kismet' order
  on installation, and 'kismet -> %crontabdir' order on uninstallation
  is required. A plain 'Requires:' does not *guarantees* such an order.


> W: kismet-debuginfo dangling-relative-symlink /usr/src/debug/kismet-2005-08-R1/libpcap-0.9.1-kis/bpf_filter.c ./bpf/net/bpf_filter.c
> Most of these are OK / have a good reason, so they are ok, it would
> be nice if you could fix the last one though, but that is not a
> blocker.

I think this is a bug in rpm's debuginfo-generator and do not know how
to solve it cleanly.


> *Redundant BR: ImageMagick-devel already requires libtiff-devel and
>  libjpeg-devel, please remove those from the BR-s

required for the FC4 ImageMagick-devel and they do not hurt on FC5; I
added a comment to explain it.

> *This comment:
>  # set our 'kismet' user, disable GPS and log into ~kismet/logs by
>  Is no longer correct as logs now go to /var/log/kismet

thx, fixed it


> *These compiler warnings look really suspicious:

don't occur anymore in recent version, but...


> * Upstream has a much newer version, it would be nice to upgrade to
>   that version.

ok, updated to it. But... it causes ugly warnings and I created a lot
of patches fixing them. They are reported upstream; see

  http://www.kismetwireless.net/Forum/General/Messages/1145789909.7993579

I know that they are making the new package different to this one
reviewed by you, but I think they are required (at least the -packed
and -alias patches).
Comment 21 Hans de Goede 2006-04-25 16:18:00 EDT
(In reply to comment #20)
> > *Summary must not start with "A ...." drop the "A " .
> 
> I could not find such a rule in the packaging guidelines and from my
> linguistic feeling, a leading "A " sounds better.
> 

You're right its not (no longer) in the guidelines, somehow it got in my head as
being a guideline, I think it has been on the MUST FIX list from reviews of my
own packages too, I don't know the history.


> > *These:
> >  Requires(pre):          %crontabdir
> >  Requires(postun):       %crontabdir
> 
> ... are required resp. the best current way to express:
> 
> * the directory must exist before the package places files into it. Else,
>   when the directory is a symlink (e.g. compare /etc/init.d) in the owning
>   package, you will create oddities.
> 
> * the package must be removed before the directory. Else, the directory
>   can not be removed because it still contains files from 'kismet' and
>   becomes orphaned.  Therefore, a strict '%crontabdir -> kismet' order
>   on installation, and 'kismet -> %crontabdir' order on uninstallation
>   is required. A plain 'Requires:' does not *guarantees* such an order.
> 

I understand / already thought as much, but in essence this means that you
require the crontabs package so why not just replace these 3 lines:
Requires(missingok):	crontabs
Requires(pre):		%crontabdir
Requires(postun):	%crontabdir

with:
Requires:		crontabs

? Anyways this is just a remark, not a blocker. However unfortunatly the
following x86_64 (rawhide) builderror is a blocker:
gpsmap.cc:2217: error: cast from 'void*' to 'int' loses precision

and the 2 suspicious warnings are still there, probably also 64 bit related:
gpsmap.cc:868: warning: comparison is always true due to limited range of data
gpsmap.cc:876: warning: comparison is always true due to limited range of data

I'll take a look at/into this, but my time for Fedora is scarce at the moment.


> 
> > W: kismet-debuginfo dangling-relative-symlink
/usr/src/debug/kismet-2005-08-R1/libpcap-0.9.1-kis/bpf_filter.c
./bpf/net/bpf_filter.c
> > Most of these are OK / have a good reason, so they are ok, it would
> > be nice if you could fix the last one though, but that is not a
> > blocker.
> 
> I think this is a bug in rpm's debuginfo-generator and do not know how
> to solve it cleanly.
> 

Yes it is, I hit it very rescently too, it doesn't handle symlinks properly (it
packages the symlink and  not the link target instead, or both.

> > * Upstream has a much newer version, it would be nice to upgrade to
> >   that version.
> 
> ok, updated to it. But... it causes ugly warnings and I created a lot
> of patches fixing them. They are reported upstream; see
> 
>   http://www.kismetwireless.net/Forum/General/Messages/1145789909.7993579
> 
> I know that they are making the new package different to this one
> reviewed by you, but I think they are required (at least the -packed
> and -alias patches).

I haven't thoroughly re-reviewed this, I did take a peek. You would have been
free under the guidelines to make this version jump after the initial import of
the previous version anyways, thus there is no need for a complete re-review and
this review is taking way too long already so I'm not restarting it from scratch.
Comment 22 Jesse Keating 2006-04-25 16:25:42 EDT
(In reply to comment #21)
> (In reply to comment #20)

> > ... are required resp. the best current way to express:
> > 
> > * the directory must exist before the package places files into it. Else,
> >   when the directory is a symlink (e.g. compare /etc/init.d) in the owning
> >   package, you will create oddities.
> > 
> > * the package must be removed before the directory. Else, the directory
> >   can not be removed because it still contains files from 'kismet' and
> >   becomes orphaned.  Therefore, a strict '%crontabdir -> kismet' order
> >   on installation, and 'kismet -> %crontabdir' order on uninstallation
> >   is required. A plain 'Requires:' does not *guarantees* such an order.
> > 
> 
> I understand / already thought as much, but in essence this means that you
> require the crontabs package so why not just replace these 3 lines:
> Requires(missingok):	crontabs
> Requires(pre):		%crontabdir
> Requires(postun):	%crontabdir
> 
> with:
> Requires:		crontabs
> 

Because with a transaction that includes the removal or addition of crontabs,
unless (pre) and (postun) are used this could get installed prior to crontabs
and thus you run into the chicken before the egg syndrome.  If the other package
contents need to be on the file system before this package can install, and need
to remain on the file system until after this package is removed, then it needs
(pre) and (postun) requirements.
Comment 23 Hans de Goede 2006-04-26 01:10:09 EDT
For some reason I woke up (insane) early this morningL: 6 A.M. so i had some
time to take a look.

It turns out that one of your warning fixes in the -alias.patch caused the
compile to fail on 64 bit. I've replaced this part of the -alias.patch with a
new version. I'll attach the new -alias.patch. I only touched the gpsmap.cc
chunck, so thats all you need to review.

Also the 2 warnings on 64 bit are really a problem, I have a patch for those
too, which I will attach also.
Comment 24 Hans de Goede 2006-04-26 01:11:55 EDT
Created attachment 128236 [details]
Updated -alias patch which fixes compilation on x86_64
Comment 25 Hans de Goede 2006-04-26 01:14:00 EDT
Created attachment 128237 [details]
Patch fixing 2 warnings (and real problems) on x84_64

With these 2 patches the package is ok. IOW approved! under the conditition
that you agree with adding both patches.
Comment 26 Hans de Goede 2006-04-27 13:12:20 EDT
One last note, the debuginfo rpmlint warning can be fixed by replacing the
symlink with a hardlink in %prep. I filed a bug against rpm about this, see bug
189928.
Comment 27 Enrico Scholz 2006-04-29 13:57:20 EDT
Thanks for the review and the patches. My first patchset is in upstream svn and
I reported your changes some minutes ago.

I did not fixed the -debuginfo bug; IMO it is a minor issue and I am not sure
about the impact in the build.


Package built for devel branch (after adding a freetype-devel BR)
Comment 28 Hans de Goede 2006-05-11 10:23:44 EDT
(In reply to comment #20)
> > *These:
> >  Requires(pre):          %crontabdir
> >  Requires(postun):       %crontabdir
> 
> ... are required resp. the best current way to express:
> 
> * the directory must exist before the package places files into it. Else,
>   when the directory is a symlink (e.g. compare /etc/init.d) in the owning
>   package, you will create oddities.
> 
> * the package must be removed before the directory. Else, the directory
>   can not be removed because it still contains files from 'kismet' and
>   becomes orphaned.  Therefore, a strict '%crontabdir -> kismet' order
>   on installation, and 'kismet -> %crontabdir' order on uninstallation
>   is required. A plain 'Requires:' does not *guarantees* such an order.
> 
> 

I just hit a problem in a package of mine being reviewed where I have exactly
this problem (package owning dir being removed before the packages which
requires the dir is). So I tried fixing it, unfortunatly this doesn't seem to
help, rpm bug? See: Bug #190878 .

Any help much appreciated.

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