On 02.01.2013 20:22, John Ferlan wrote:
> 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.
I think we should check for the return value of select() / waitsocket().
But I also think we can silently ignore EINTR. So the code should look
something like this:
errno = 0;
while (some_libssh2-ish_rubbish) {
if (waitsocket(...) < 0 && errno != EINTR) {
virReportSystemError(errno, "%s", _("unable to wait on libssh2 socket");
goto err;
}
}
>
> 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?
In fact on Linux (yet another difference from *BSD) it does make sense to set
SO_REUSEADDR even for outgoing connections, because on Linux, even outgoing
connection stays in TIME_WAIT state for some time. This means, port cannot be
reused at this time, unless reuse was set in both previous and current process.
On a machine with higher count of connections, port reusing can be handy then.
However, I don't think failing to set the bit is a fatal error. It's only
a hint for the kernel, so I'd just VIR_WARN() about it.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list