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

[John Ferlan]

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 at 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.

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.

IIUC, the SendAll/RecvAll are merely an implementation of those
comments. So I'm left with that nagging doubt regarding why didn't the
original implementation always ensure to call virStreamAbort if -1 was
to be returned.

I'm not against the change, I just was hoping someone else would be able
to also speak up and say - yes, this is OK. Someone that perhaps has
more detailed knowledge of the stream code than I do.

Whether the code examples are updated in comments is entirely up to you,
but if it helps someone else out in the future that'd be good especially
since this is a "confusing" area.

John

>>
>> Would there be a chance of multiple callers to virStreamAbort?
> 
> Not over the same stream. The server side doesn't use these
> SendAll/RecvAll wrappers. They are for client only. And as such client
> is responsible for not calling them from two separate threads at the
> same time.
> 
>>
>> It'd be nice if there were any other opinions on this. I'd lean towards
>> saying looks good, but I'm also interested if there's any other opposing
>> opinions or concerns.
>>
>> John
> 
> Michal
>