[Bug 456892] Review Request: aget - multi-threaded download accelerator

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=456892


Rakesh Pandit <rakesh.pandit at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rakesh.pandit at gmail.com




--- Comment #5 from Rakesh Pandit <rakesh.pandit at gmail.com>  2008-08-16 07:33:43 EDT ---
This is an unofficial review (As I am not a sponsor) Just to help this package
move on.


There are few warnings are cosmetic but if reported upstream, will be good(as
code base is too small & warnings are two trivial). They are very easy to fix
also:

First set:
----------
tables -D_GNU_SOURCE   -c -o Signal.o Signal.c
In function 'snprintf',
    inlined from 'resume_get' at Aget.c:179:
/usr/include/bits/stdio2.h:65: warning: call to __builtin___snprintf_chk will
always overflow destination buffer
In function 'snprintf',
    inlined from 'get' at Aget.c:101:
/usr/include/bits/stdio2.h:65: warning: call to __builtin___snprintf_chk will
always overflow destination buffer


Both of these can be fixed with snprintf(<char_ptr>, size .... 
size greater then or equal to size actually allocated to <char_ptr>.

Second set:
-----------
tables -D_GNU_SOURCE   -c -o Resume.o Resume.c
Resume.c: In function 'save_log':
Resume.c:45: warning: ignoring return value of 'fwrite', declared with
attribute warn_unused_result
Resume.c: In function 'read_log':
Resume.c:77: warning: ignoring return value of 'fread', declared with attribute
warn_unused_result
gcc -o aget main.o Aget.o Misc.o Head.o Signal.o Download.o Resume.o -pthread

Handling the return value of fwrite and fread. In case these calls face,
printing an error message to stderr.


Your patch for including error.h places include directive at not so good place.
May you move it along with standard header files included in source files.

Between have you reported the already attached patch upstream?

Note: These are cosmetic issues and don't block. But it would be great if they
are resolved.


[x] name
[x] md5sum
     1d32390f5ea2ddd82dfbb1794cdfa92f upstream source 
     1d32390f5ea2ddd82dfbb1794cdfa92f package source
[x] license -- except COPYING file there is no mention of license in code
files.
     Have you confirmed about BSD license?
[x] Spec file is in American Eng and legible
[x] Build successfully
[x] BuildRequires 
[x] Duplicate files - nil
[NA] locale
[x] permissions -- okay
[x]  source link correct
[x] packaging guidlines
[x] Buildroot correct
[x] owns every directory it creates
[x] file encoding - checked
[x] package has no dependency on files in %doc
[NA] GUI
[x] No dependencies outside FHS guidelines

Optional suggestions:
[x] A small patch to correct warnings and Makefile.


Key NA = N/A, x = Check, ! = Problem, ? = Not evaluate

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.