[libvirt] Coverity scan

[John Ferlan]

On 12/20/2012 04:24 PM, Eric Blake wrote:
> On 12/20/2012 08:25 AM, John Ferlan wrote:
>>
>> First allow me to introduce myself - I'm John Ferlan a new Red Hat employee (3 weeks).  I came from the closed world at HP where for the last 7 years I worked in a group developing/supporting HP's Integrity Virtual Machine software prior to it being outsourced to India this past May. I primarily worked in the CLI/API and daemon space, although I also spent quite a bit of time in the lower virtualization layers which mimicked the Integrity instructions. I am very happy to be in the open world and look forward to contributing.  Everyone has to start some where.
> 
> Welcome to libvirt!  Based on recent list traffic, there are several
> people, not just John, and not just at Red Hat, that are aiming to
> provide initial contributions.  Be sure to provide your feedback and
> questions on the list, and hopefully we can use that to make our hacking
> documentation even easier to follow for future newcomers.
> 
> You may want to figure out why your mailer doesn't automatically wrap
> long lines.  Webmail interfaces (gmail, zimbra) and Microsoft Exchange
> tend to be the worst mail agents when it comes to RFC compliance and
> lack of knobs to tune for improving the situation, which in turn can
> make it harder to read your messages and apply patches you submit.  You
> may notice that many people on this list use mutt or Thunderbird rather
> than web mailers, if only to have better formatted mail, but it's not a
> hard pre-requisite (we won't reject a patch just because of how it was
> sent, although it might take longer to apply if we have to jump through
> hoops to get it extracted from the mail).  For mailers that have the
> right knobs, turning on format=flowed is one way of sending reasonably
> formatted mail [but be aware that Thunderbird has had a rather nasty bug
> for several years now where defaulting to format=flowed can result in
> botching the whitespace of the quoted portion of a message you are
> replying to].
> 
>>
>> My first task here at Red Hat was to triage a Coverity scan executed against libvirt-1.0.0-1.fc19.src.rpm done in late November.  There were 285 issues documented. I quickly found that some of the defects found there were already fixed in later submits upstream, so I ran a new Coverity scan last Friday and it came back with 297 issues broken down as follows:
>>
>>      1 ARRAY_VS_SINGLETON
>>     33 BAD_SIZEOF
>>     17 CHECKED_RETURN
>>      1 CONSTANT_EXPRESSION_RESULT
>>      5 COPY_PASTE_ERROR
>>     13 DEADCODE
>>     46 FORWARD_NULL
>>      2 MISSING_RETURN
>>      2 NEGATIVE_RETURNS
>>      7 NULL_RETURNS
>>      1 OVERRUN
>>    137 RESOURCE_LEAK
>>     18 REVERSE_INULL
>>      1 SIGN_EXTENSION
>>      3 UNINIT
>>      8 UNUSED_VALUE
>>      2 USE_AFTER_FREE
> 
> Thanks for doing this.  Helping to reduce the false positives and
> eliminate the real bugs will make it easier to run Coverity on a
> periodic basis and check for recent regressions while the code is still
> fresh on our minds.  Looking forward to the patches you come up with.
> 


Ran a new scan recently - the number of defects increased slightly (+4
FORWARD_NULL, +2 MISSING_RETURN, and + 1 UNINIT).

I worked my way through the entire list and just figured I'd start with
an easier group for my first submit attempt. I have a series of
relatively easy changes for the "CHECKED_RETURN" condition; however, I
was hoping to perhaps get some feedback on two files which had more
ramifications from just checking the return status.

The first is 'src/phyp/phyp_driver.c'. Neither waitsocket() nor any of
the callers check the return status. The implication being if select()
fails the code will continue to wait. So is this "desired"? Conversely
what are the ramifications of checking status on select() and using
virReportSystemError()? Or perhaps some other way to log the failure?
For any of the callers, any concerns over checking status return == -1
and jumping to the err label? I guess I'm concerned over making
logic/flow changes to some long standing assumption.  I've been on the
opposite end of debugging one of those.

The second is 'src/rpc/virnetsocket.c'.  The Coverity complaint is that
the setsockopt() call to set SO_REUSEADDR doesn't check the return
status. I think this particular case may be a cut-n-paste type change as
the from virNetSocketNewListenTCP(). Unless I'm missing something, does
setting the reuse addr on a connect socket do anything? Can it just
safely be removed?

Tks,

John