Re: [PATCH] maint: avoid new coverity warnings

[Pádraig Brady]

On 29/05/15 23:08, Bernhard Voelker wrote:
> On 05/29/2015 07:39 PM, Pádraig Brady wrote:
>> * src/sync.c (sync_arg): Initialise variable to avoid
>> unitialized access if assert is disabled.
>> * src/head.c (elide_tail_bytes_file): Support this function
>> with ---presume-input-pipe and larger files,
>> which regressed with commit v8.23-47-g2662702.
>> (elide_tail_lines_file): Likewise.
>> * src/dd.c (dd_copy): Explicitly don't try to ftruncate()
>> upon failure to lseek() (the existing check against
>> st_size was already protecting that).
>> * src/factor.c (factor_using_squfof): Assert (only when
>> linting due to performance) to avoid the implication of
>> divide by zero.
>> * src/od.c (read_block): Remove dead code.
>> * src/tac.c (tac_seekable): Likewise.
>> ---
>>  src/dd.c     | 2 +-
>>  src/factor.c | 3 ++-
>>  src/head.c   | 4 ++--
>>  src/od.c     | 3 ---
>>  src/sync.c   | 2 +-
>>  src/tac.c    | 3 ---
>>  6 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/dd.c b/src/dd.c
>> index 6b09bc6..e647294 100644
>> --- a/src/dd.c
>> +++ b/src/dd.c
>> @@ -2280,7 +2280,7 @@ dd_copy (void)
>>        if (S_ISREG (stdout_stat.st_mode) || S_TYPEISSHM (&stdout_stat))
>>          {
>>            off_t output_offset = lseek (STDOUT_FILENO, 0, SEEK_CUR);
>> -          if (output_offset > stdout_stat.st_size)
>> +          if (0 <= output_offset && stdout_stat.st_size < output_offset)
>>              {
>>                if (iftruncate (STDOUT_FILENO, output_offset) != 0)
>>                  {
>> diff --git a/src/factor.c b/src/factor.c
>> index 4e4d0c7..f27bf22 100644
>> --- a/src/factor.c
>> +++ b/src/factor.c
>> @@ -2055,8 +2055,9 @@ factor_using_squfof (uintmax_t n1, uintmax_t n0, 
>> struct factors *factors)
>>            div_smallq (q, rem, S+P, Q);
>>            P1 = S - rem; /* P1 = q*Q - P */
>>  
>> +          IF_LINT (assert (q > 0 && Q > 0));
>> +
>>  #if STAT_SQUFOF
>> -          assert (q > 0);
>>            q_freq[0]++;
>>            q_freq[MIN (q, Q_FREQ_SIZE)]++;
>>  #endif
>> diff --git a/src/head.c b/src/head.c
>> index 3ea81b6..410cc4f 100644
>> --- a/src/head.c
>> +++ b/src/head.c
>> @@ -458,7 +458,7 @@ elide_tail_bytes_file (const char *filename, int fd, 
>> uintmax_t n_elide,
>>                         struct stat const *st, off_t current_pos)
>>  {
>>    off_t size = st->st_size;
>> -  if (size <= ST_BLKSIZE (*st))
>> +  if (presume_input_pipe || size <= ST_BLKSIZE (*st))
>>      return elide_tail_bytes_pipe (filename, fd, n_elide, current_pos);
>>    else
>>      {
>> @@ -747,7 +747,7 @@ elide_tail_lines_file (const char *filename, int fd, 
>> uintmax_t n_elide,
>>                         struct stat const *st, off_t current_pos)
>>  {
>>    off_t size = st->st_size;
>> -  if (size <= ST_BLKSIZE (*st))
>> +  if (presume_input_pipe || size <= ST_BLKSIZE (*st))
>>      return elide_tail_lines_pipe (filename, fd, n_elide, current_pos);
>>    else
>>      {
>> diff --git a/src/od.c b/src/od.c
>> index c1241f5..0ca3ca7 100644
>> --- a/src/od.c
>> +++ b/src/od.c
>> @@ -1291,9 +1291,6 @@ read_block (size_t n, char *block, size_t 
>> *n_bytes_in_buffer)
>>  
>>    *n_bytes_in_buffer = 0;
>>  
>> -  if (n == 0)
>> -    return true;
>> -
>>    while (in_stream != NULL) /* EOF.  */
>>      {
>>        size_t n_needed;
>> diff --git a/src/sync.c b/src/sync.c
>> index b71b8bc..85d77c0 100644
>> --- a/src/sync.c
>> +++ b/src/sync.c
>> @@ -117,7 +117,7 @@ sync_arg (enum sync_mode mode, char const *file)
>>  
>>    if (ret == true)
>>      {
>> -      int sync_status;
>> +      int sync_status = -1;
>>  
>>        switch (mode)
>>          {
>> diff --git a/src/tac.c b/src/tac.c
>> index 2d73c6e..295ac77 100644
>> --- a/src/tac.c
>> +++ b/src/tac.c
>> @@ -331,9 +331,6 @@ tac_seekable (int input_fd, const char *file, off_t 
>> file_pos)
>>                  xalloc_die ();
>>                newbuffer = xrealloc (G_buffer - offset, G_buffer_size);
>>                newbuffer += offset;
>> -              /* Adjust the pointers for the new buffer location.  */
>> -              match_start = newbuffer + match_start_offset;
>> -              past_end = newbuffer + past_end_offset;
>>                G_buffer = newbuffer;
>>              }
>>  
>>
> 
> Nice!
> All but the last look good - the last results in a new unused variable
> warning/error:
> 
> src/tac.c: In function 'tac_seekable':
> src/tac.c:325:25: error: unused variable 'past_end_offset' 
> [-Werror=unused-variable]
>                ptrdiff_t past_end_offset = past_end - G_buffer;
>                          ^
> src/tac.c:324:25: error: unused variable 'match_start_offset' 
> [-Werror=unused-variable]
>                ptrdiff_t match_start_offset = match_start - G_buffer;
>                          ^
> 
> BTW: you could squash in the following for another new coverity
> warning to avoid numerical comparison with a bool:

Pushed with that change.


> diff --git a/src/dircolors.c b/src/dircolors.c
> index 3a03f1f..6cf0cdb 100644
> --- a/src/dircolors.c
> +++ b/src/dircolors.c
> @@ -442,7 +442,7 @@ main (int argc, char **argv)
>        usage (EXIT_FAILURE);
>      }
> 
> -  if ((!print_database) < argc)
> +  if ((print_database ? 0 : 1) < argc)

I dislike that, since the other is shorter and obvious.
The coverity warning is tagged, so we shouldn't see it again.

thanks,
Pádraig.