[libvirt] [PATCH v2 15/38] Introduce virStreamSparseSendAll

[Michal Privoznik]

On 05/04/2017 11:35 PM, John Ferlan wrote:
> 
> 
> On 04/20/2017 06:01 AM, Michal Privoznik wrote:
>> This is just a wrapper over new function that have been just
>> introduced: virStreamSkip() . It's very similar to
>> virStreamSendAll() except it handles sparse streams well.
> 
> You could have some changes here due to previous API name changes.
> Still the commit msg is a bit terse needs some cleanup anyway.
> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  include/libvirt/libvirt-stream.h |  59 +++++++++++++++++++--
>>  src/libvirt-stream.c             | 107 +++++++++++++++++++++++++++++++++++++++
>>  src/libvirt_public.syms          |   1 +
>>  3 files changed, 164 insertions(+), 3 deletions(-)
>>


>>  /**
>> + * virStreamSparseSendAll:
>> + * @stream: pointer to the stream object
>> + * @handler: source callback for reading data from application
>> + * @holeHandler: source callback for determining holes
>> + * @skipHandler: skip holes as reported by @holeHandler
>> + * @opaque: application defined data
>> + *
>> + * Some dummy description here.
> 
> Some smart-aleck comment here ;-)

Oh, I thought I've fixed this before sending O:-)

> 
>> + *
>> + * Opaque data in @opaque are shared between @handler, @holeHandler and @skipHandler.
>> + *
>> + * Returns 0 if all the data was successfully sent. The caller
>> + * should invoke virStreamFinish(st) to flush the stream upon
>> + * success and then virStreamFree.
>> + *
>> + * Returns -1 upon any error, with virStreamAbort() already
>> + * having been called,  so the caller need only call
>> + * virStreamFree().
>> + */
>> +int virStreamSparseSendAll(virStreamPtr stream,
>> +                           virStreamSourceFunc handler,
>> +                           virStreamSourceHoleFunc holeHandler,
>> +                           virStreamSourceSkipFunc skipHandler,
>> +                           void *opaque)
>> +{
>> +    char *bytes = NULL;
>> +    size_t want = VIR_NET_MESSAGE_LEGACY_PAYLOAD_MAX;
>> +    int ret = -1;
>> +    unsigned long long dataLen = 0;
>> +
>> +    VIR_DEBUG("stream=%p handler=%p holeHandler=%p opaque=%p",
>> +              stream, handler, holeHandler, opaque);
>> +
>> +    virResetLastError();
>> +
>> +    virCheckStreamReturn(stream, -1);
>> +    virCheckNonNullArgGoto(handler, cleanup);
>> +    virCheckNonNullArgGoto(holeHandler, cleanup);
>> +    virCheckNonNullArgGoto(skipHandler, cleanup);
>> +
>> +    if (stream->flags & VIR_STREAM_NONBLOCK) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("data sources cannot be used for non-blocking streams"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (VIR_ALLOC_N(bytes, want) < 0)
>> +        goto cleanup;
>> +
>> +    for (;;) {
>> +        int inData, got, offset = 0;
>> +        unsigned long long sectionLen;
>> +
>> +        if (!dataLen) {
>> +            if (holeHandler(stream, &inData, &sectionLen, opaque) < 0) {
>> +                virStreamAbort(stream);
>> +                goto cleanup;
>> +            }
>> +
>> +            if (!inData && sectionLen) {
> 
> So if !inData and sectionLen == 0, we don't go here, but
> 
>> +                if (virStreamSkip(stream, sectionLen) < 0) {
>> +                    virStreamAbort(stream);
>> +                    goto cleanup;
>> +                }
>> +
>> +                if (skipHandler(stream, sectionLen, opaque) < 0) {
>> +                    virReportSystemError(errno, "%s",
>> +                                         _("unable to skip hole"));
>> +                    virStreamAbort(stream);
>> +                    goto cleanup;
>> +                }
>> +                continue;
>> +            } else {
>> +                dataLen = sectionLen;
> 
> We go here indicating 'dataLen = 0' which is a misnomer since we're not
> inData, rather we at EOF, right?  Or did I miss something a bit subtle.

Yeah, we go through another iteration of @handler. We are at EOF so it
doesn't matter. Basically this is for consistence with daemon
counterpart.  NB with current implementation there should never ever be
STREAM_SKIP packet with len = 0. I've wanted pre-existing code to handle
EOFs (i.e. virStreamRecv) instead of sending useless packet every time.
Consider following example:

data -> hole -> data -> eof.

So the sequence of packets sent looks like this then:

STREAM + data -> SKIP -> STREAM + data -> STREAM + no data

Now, the last packet sent is STREAM without any data and header.status =
VIR_NET_OK meaning there are no other packets in the stream. This is the
last one. If we were to treat the implicit hole at eof just like the
regular one STREAM, either there would be dummy SKIP packet in between
the last two STREAM packets, or it could substitute the last STREAM & no
data packet. Either way, we don't need two possible representations of
EOF currently. Lets have just one for now. In order to achieve that we
iterate over the loop once again (despite knowing that we are at EOF)
and have the code send the EOF representation.

Michal