[libvirt] [PATCH] Eliminate large stack buffer in doTunnelSendAll

[Laine Stump]

On 03/05/2010 04:40 PM, Eric Blake wrote:
> On 03/05/2010 02:30 PM, Laine Stump wrote:
>    
>> doTunnelSendAll function (used by QEMU migration) uses a 64k buffer on
>> the stack, which could be problematic. This patch replaces that with a
>> buffer from the heap.
>>      
> There's a lot more large stacks noted by Coverity, but this is a good start.
>    

Heh. Yeah, this one happened to catch my eye last week while I was in 
this file, and I couldn't let it stand ;-)

>> While in the neighborhood, this patch also improves error reporting in
>> the case that saferead fails - previously, virStreamAbort() was called
>> (resetting errno) before reporting the error. It's been changed to
>> report the error first.
>>      
> Yep.
>
>    
>> -    char buffer[65536];
>> -    int nbytes = sizeof(buffer);
>> +    char *buffer;
>> +    int nbytes = TUNNEL_SEND_BUF_SIZE;
>> +
>> +    if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE)<  0) {
>> +        virStreamAbort(st);
>> +        virReportOOMError();
>>      
> Is this backwards?
>    

Order isn't as important in this case as the other (the one I changed), 
since virReportOOMError() doesn't need to look at errno. But I agree it 
would be more consistent to do them both in the same order. I just 
resubmitted (and forgot to set the In-Reply-To header so it would show 
up in this thread. #&%$!)

>    
>> +        return -1;
>> +    }
>>
>>       /* XXX should honour the 'resource' parameter here */
>>       for (;;) {
>>           nbytes = saferead(sock, buffer, nbytes);
>>           if (nbytes<  0) {
>> -            virStreamAbort(st);
>>               virReportSystemError(errno, "%s",
>>                                    _("tunnelled migration failed to read from qemu"));
>> +            virStreamAbort(st);
>> +            VIR_FREE(buffer);
>>      
> Especially given that you just reordered the virStreamAbort to come last
> here?  If the above was not a mistake, then ACK to the patch.
>
>