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@xxxxxxxxxx> >>>> --- >>>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
