[asterisk-dev] [asterisk-commits] kpfleming: branch 1.4 r153337 - in /branches/1.4: agi/ apps/ channels/ format...

[Luigi Rizzo]

On Sat, Nov 1, 2008 at 7:22 PM, SVN commits to the Asterisk project
<asterisk-commits at lists.digium.com> wrote:
>
> Author: kpfleming
> Date: Sat Nov  1 13:22:39 2008
> New Revision: 153337
>
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=153337
> Log:
> fix a bunch of potential problems found by gcc 4.3.x, primarily bare strings being passed to printf()-like functions and ignored results from read()/write() and friends
>

some of the changes ni this patch (e.g. printf arguments) are definitely
correct and welcome.
Some of the others, unfortunately, are just silencing the compiler without
really fixing the underlying problems, so it would be good to take this chance
to fix the code in the right way.
In particular:

+ For the read()/write() return values, just adding an ast_log() or
fprintf() doesn't
    really fix the bug, and causes a lot of code bloat. Both read()
and write() can
    fail with short ops due e.g to the delivery of a signal. It might
make sense to
    define wrapper functions that loop around EINTR, or have a flag to
operate on
    the exact length requested, or just print a warning if needed.

For the read(), things are more critical because just logging the failure
is normally not enough, the rest of the processing is often invalidated;
see e.g. the change to app_festival() where if the read fails, there is no
point in keep using the value (which is unset) for further computations.
Also remember that a read() may fail for the delivery of a signal, which
means we should check for EINTR and retry rather than just aborting.

For the fgets(), which is often used together with feof(), things are a bit more
convolute.
The manpage for fgets() says it returns NULL if it cannot read anything
(due to an EOF) or in case of error, includes a signal (same as for read()).
As a consequence, in many of the cases touched by this patch the typical code
pattern should be something like this:

         while ( !feof(fd) ) {
             if (!fgets(...)) continue;
             /* do not call feof() here */
             ...process the result of fgets...
        }

so a number of the existing feof() checks in the code are wrong, and
the change to eagi-sphinx-test.c is wrong too (while some of the others
which do a 'continue;' are more correct.

        cheers
        luigi