Just a couple of quick comments:
- if the final upstream release will be 1.25 (i.e. drops the trailing "b"), change the version number to 1.25 and the release tag to 0.1.b%{dist} (see http://fedoraproject.org/wiki/PackageNamingGuidelines#Pre-Release_packages)
- the package doesn't build in mock because of a missing BR: libidn-devel
- in the %files section, replace %{_datadir}/*
with %{_datadir}/%{name}/
- also in the %files section, replace %attr(755,root,root) /usr/bin/skipfish
with %{_bindir}/%{name}

A couple of further remarks:
- you can drop "%attr(755,root,root)" from the %files section because the permissions are set automatically
- Directory /usr/share/skipfish/assets contains a file COPYING with the GPL 3.0 license text. You should ask upstream whether the images are actually intended to be licensed under GPLv3. If so, change the License tag to "ASL 2.0 and GPLv3".
- Please also upload the SPEC file to your server. The above SPEC URLs don't work (404).

SPEC URL: http://rebus.webz.cz/d/skipfish.spec
SRPM URL: http://rebus.webz.cz/d/skipfish-1.26-0.2.b.fc12.src.rpm
Hello Martin,
thank you for the review.
>- you can drop "%attr(755,root,root)" from the %files section because the
>permissions are set automatically
Done
>- Directory /usr/share/skipfish/assets contains a file COPYING with the GPL 3.0
>license text.
Thanks for noticing that. License is actually LGPLv3.
>You should ask upstream whether the images are actually intended
>to be licensed under GPLv3.
>If so, change the License tag to "ASL 2.0 and GPLv3".
Icons are comming from different project so they need to keep different license.
Following the licensing guideline I have split skipfish to 2 packages with different licenses (http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios)
>- Please also upload the SPEC file to your server.
> The above SPEC URLs don't work (404).
I a sorry for the typo ... it should have been .spec not .SPEC
Best regards
Michal Ambroz

Hi Michal,
two additional remarks:
- The header file string-inl.h is licensed under BSD (3 clause variant). Thus, you must add BSD to the License field.
- You can drop the redundant %dir lines in the %files section.

(In reply to comment #11)
> Please could you specify what dir lines you find redundant?
Hi Michal,
you can omit all %dir lines in your spec file. These lines simply add empty directories to the binary rpm and would only be necessary if no files were supposed to be added to these directories. But since the %file section contains lines that place files in there, rpm creates the required directories automatically:
%{_datadir}/%{name}/assets/index.html creates %{_datadir}/%{name} and %{_datadir}/%{name}/assets, and finally puts index.html in "assets". Thus, there's no need for
%dir %{_datadir}/%{name} and
%dir %{_datadir}/%{name}/assets.
Please also append a slash to "dictionaries" in order to make clear a folder (including all containing files) is added:
%{_datadir}/%{name}/dictionaries/
It's not required but this way it's easier to notice "dictionaries" is a folder and not a file.

To be a bit more clearer, I suggest to make the %files section look like this:
%files
%defattr(-,root,root,-)
%doc COPYING ChangeLog README
%{_bindir}/%{name}
%{_datadir}/%{name}/assets/index.html
%{_datadir}/%{name}/dictionaries/

(In reply to comment #12)
> %{_datadir}/%{name}/assets/index.html creates %{_datadir}/%{name} and
> %{_datadir}/%{name}/assets, and finally puts index.html in "assets". Thus,
> there's no need for
> %dir %{_datadir}/%{name} and
> %dir %{_datadir}/%{name}/assets.
Incorrect. See review guidelines:
"MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory."
If you drop the declarations %dir %{_datadir}/%{name} and %dir {_datadir}/%{name}/assets, these will stick around after uninstall.

(In reply to comment #13)
> To be a bit more clearer, I suggest to make the %files section look like this:
>
> %files
> %defattr(-,root,root,-)
> %doc COPYING ChangeLog README
> %{_bindir}/%{name}
> %{_datadir}/%{name}/assets/index.html
> %{_datadir}/%{name}/dictionaries/
and it has to own these, too
%dir %{_datadir}/%{name}
%dir %{_datadir}/%{name}/assets
since otherwise these wouldn't be owned by anything.

(In reply to comment #15)
> and it has to own these, too
>
> %dir %{_datadir}/%{name}
> %dir %{_datadir}/%{name}/assets
>
> since otherwise these wouldn't be owned by anything.
Oh, sorry for the confusion. I thought these kinds of ownership are detected automatically too. I obviously still have to dig into the details. Sorry again.

Hello guys,
yes I actually just came to the same conclusion, when removing all the %dir from the spec file.
$ sudo rpm -Uhv skipfish-1.31-0.3.b.fc12.i686.rpm skipfish-icons-1.31-0.3.b.fc12.noarch.rpm
Preparing... ########################################### [100%]
1:skipfish ########################################### [ 50%]
2:skipfish-icons ########################################### [100%]
$ rpm -qf /usr/share/skipfish
file /usr/share/skipfish is not owned by any package
$ rpm -qf /usr/share/skipfish/dictionaries
file /usr/share/skipfish/dictionaries is not owned by any package
$ rpm -qf /usr/share/skipfish/assets
file /usr/share/skipfish/assets is not owned by any package
So ... I will revert the changes.
Michal

I do not think it makes much sense to split out icons in an extra subpackage just for the licensing reason. Especially when the ASL-2.0 and LGPLv3 licenses are compatible. Just include both licenses properly in the License field and in the %doc. The split would make sense for example in case of library/tools with different licenses split or in similar cases where the split out files can be reasonably useful alone.

Hello Tomasi,
I was following the recommendation from the licensing guidelines about using AND in the license field:
"Fedora maintainers are highly encouraged to avoid this scenario whenever reasonably possible, by dividing files into subpackages (subpackages can each have their own License: field)."
Statement seems pretty strong and in this case it was low effort to separate subpackage. Files are not linked together into one binary.
Of course if you really do want to see this packaged under single package I will respect your opinion.
>The split would make sense for example in case of library/tools with
>different licenses split or in similar cases where the split out files can be
>reasonably useful alone.
I was rather thinking about possibility that someone can opt for installing different set of icons to use with skipfish. (not that such set would be available now)
Best regards
Michal Ambroz

I'm sorry but I really dislike this split and I have to say that I really dislike the wording of the guideline too. Artificially inflating the number of packages just for the different licensing of the files when we can not reasonably fix all the packages this way anyway does not seem like a way to improve Fedora to me.
I am not sure how much realistic scenario is it to have different set of icons for such kind of tool.

A few more notes:
I am not sure that including the string-inl.h as %doc is correct as the guideline says "If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. If the source package does not include the text of the license(s), the packager should contact upstream and encourage them to correct this mistake." The license is not in its own file. You have it also twice in the %files.
I see you workaround some problem with FORTIFY_SOURCE, is that really needed? I do not see any warning. It would be much better to fix the problematic code if possible.

Hello Tomas,
regarding the FORIFY_SOURCE - here are the links to the skipfish bugzilla:
http://code.google.com/p/skipfish/issues/detail?id=34http://code.google.com/p/skipfish/issues/detail?id=1
Result is that this won't be fixed in skipfish and is issue of the fortify code in gcc. Skipfish upstream distributes with makefile containing -DFORTIFY_SOURCE=0.
Issue which I have analyzed was working like that:
1) ask malloc to allocate buffer of size let say 97 bytes
2) malloc allocates 100 (according to malloc_usable_size)
3) to clean any leftovers memset(buffer, 0, 100) is called
4) this triggers coredump as FORTIFY code believes 97 bytes was requested, no more should be accessed.
Situation gets even more complicated with realloc.
In my opinion malloc_usable_size should be "FORTIFIED" (return only number of requested and not allocated bytes) or FORTIFY_SOURCE should use malloc_usable_size as the cap and not the requested bytes.
From security point of view - if I got from kernel more space than requested it is OK to clean it all from some lef-overs which have been there before to avoid some posibility of injecting some unwanted data.
Best regards
Michal Ambroz

Sorry but I just consulted this with Jakub Jelinek and this is clearly bug in the source code of skipfish. The malloc_usable_size() does not allow you to memset over the end of the length passed to malloc(). There might be very well some internal data of the allocator. This call just tells you that if you realloc the allocated memory it will not have to move the block if the newly requested size is up to the malloc_usable_size() length.
So please
1. report this to the upstream.
2. patch the memset calls so they clear just the allocated memory.
3. change the spec so it properly uses the optflags from rpm including the FORTIFY_SOURCE=2.

I do not understand why you patched it this way. It does not make sense to me to overallocate the memory to always occupy the malloc_usable_size(). Why do not just drop the malloc_usable_size() calls altogether and memset just the allocated memory. You cannot use the overallocated memory in the callers anyway.

I patched it this way because skipfish is using the memory this way.
If you do not like this way, please could you propose some better way. Do you know about better patch which would not need to rewrite most of the realloc calls in skipfish?
Michal Ambroz

You're right, that the ck_realloc would have to be changed to have possibly 2 more parameters - curr_nmemb, add_nmemb, size - but the rewrite would be pretty straightforward. I'll make the patch later.