[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 3/6] virStream*All: Call virStreamAbort() more frequently



On 06/13/2017 03:41 PM, John Ferlan wrote:
> 
> 
> On 06/13/2017 07:31 AM, Michal Privoznik wrote:
>> On 06/13/2017 12:45 AM, John Ferlan wrote:
>>>
>>>
>>> On 06/05/2017 04:22 AM, Michal Privoznik wrote:
>>>> Our documentation to all four virStreamRecvAll, virStreamSendAll,
>>>
>>> s/all four/the/
>>>
>>>> virStreamSparseRecvAll, virStreamSparseSendAll says that if these
>>>
>>> s/RecvAll, virStream/RecvAll, and virStream/
>>>
>>> s/says that if these functions fail,/functions indicates that on failure/
>>>
>>>> functions fail, virStreamAbort() is called. But that is not
>>>> necessarily true. For instance all of these functions allocate a
>>>> buffer to work with. If the allocation fails, no virStreamAbort()
>>>> is called despite -1 being returned. It's the same story with
>>>> argument sanity checks and a lot of other checks.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn redhat com>
>>>> ---
>>>>  src/libvirt-stream.c | 49 ++++++++++++++++++++-----------------------------
>>>>  1 file changed, 20 insertions(+), 29 deletions(-)
>>>>
>>>
>>> This patch particularly scares me because there are so many more paths
>>> that could now call virStreamAbort. Yes, I know that's the point
>>
>> Exactly :-)
>>
>>> , but
>>> nonetheless it's large leap of faith to only calling Abort as a result
>>> of some Send/Recv callback failure to calling Abort for the ancillary
>>> stuff surrounding or supporting those calls as well.
>>
>> Why is that? The documentation to this function says that if this
>> function fails, virStreamAbort has been called. So in fact currently
>> this function might fail without calling virStreamAbort.
>>
> 
> I understand/agree - it's still a large leap of faith though! It's
> always the nagging proposition of why was the code originally done the
> way it was where abort only happens in specific cases. Trying to peer
> back through the space and time continuum at commit id '182eba1b' and
> consider if there was a specific reason the "other" paths that could
> return -1 didn't call virStreamAbort? Why was it *only* the (handler) paths.

Well. it's 2009. The code looked very differently then. Or maybe it jsut
slipped review.

> 
> Beyond the setting for the NONBLOCK and VIR_ALLOC_N failures, was the
> the expectation that somewhere in the virStreamSend/virStreamRecv calls
> that virStreamAbort would have been called on failure.  Both note that
> the stream will be marked as aborted.
> 
> Another "irony" is that the comment code example for virStreamSend shows
> the need to call virStreamAbort after a virStreamSend failure, but the
> same is not true for virStreamRecv failure example.

Ah, I haven't realized that! Sure. this definitely needs to be fixed
too. In fact, it belongs to this patch IMO - since it's fixing the very
same issue.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]