[libvirt] [PATCH v2 16/38] Introduce virStreamInData

[John Ferlan]

On 04/20/2017 06:01 AM, Michal Privoznik wrote:
> This is just an internal API, that calls corresponding function

s/API,/API   (e.g. no need for the comma)

s/calls/calls the/

> in stream driver. This function will set @data=1 if the

s/in/in the/

> underlying file is in data section, or @data=0 if it is in a
> hole. At any rate, @length is set to number of bytes remaining in
> the section the file currently is.

As discussed before @data will be true or false.
@length will be >= 0 if @ret == 0.  If @length == 0 and @data==false,
then at EOF.

In any case, I'd prefer that whatever the values "could be" would be
left in the function description section not commit message so that one
doesn't have to jump to the commit message to get that information.

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/driver-stream.h      |  6 ++++++
>  src/libvirt-stream.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_internal.h   |  3 +++
>  src/libvirt_private.syms |  1 +
>  4 files changed, 53 insertions(+)
> 
> diff --git a/src/driver-stream.h b/src/driver-stream.h
> index e196b6d..5d84b9a 100644
> --- a/src/driver-stream.h
> +++ b/src/driver-stream.h
> @@ -50,6 +50,11 @@ typedef int
>                          unsigned long long *length);
>  
>  typedef int
> +(*virDrvStreamInData)(virStreamPtr st,
> +                      int *data,
> +                      unsigned long long *length);
> +
> +typedef int
>  (*virDrvStreamEventAddCallback)(virStreamPtr stream,
>                                  int events,
>                                  virStreamEventCallback cb,
> @@ -78,6 +83,7 @@ struct _virStreamDriver {
>      virDrvStreamRecvFlags streamRecvFlags;
>      virDrvStreamSkip streamSkip;
>      virDrvStreamHoleSize streamHoleSize;
> +    virDrvStreamInData streamInData;
>      virDrvStreamEventAddCallback streamEventAddCallback;
>      virDrvStreamEventUpdateCallback streamEventUpdateCallback;
>      virDrvStreamEventRemoveCallback streamEventRemoveCallback;
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index 7ad5a38..975315e 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -473,6 +473,49 @@ virStreamHoleSize(virStreamPtr stream,
>  
>  
>  /**
> + * virStreamInData:
> + * @stream: stream
> + * @data: are we in data or hole
> + * @length: length to next section
> + *
> + * This function checks the underlying stream (typically a file)
> + * to learn whether the current stream position lies within a
> + * data section or a hold. Upon return @data is set to a nonzero

s/hold/hole

> + * value if former is the case, or to zero if @stream is in a
> + * hole. Moreover, @length is updated to tell caller how many
> + * bytes can be read from @stream until current section changes
> + * (from data to a hole or vice versa).
> + *
> + * As a special case, there's an implicit hole at EOF. In this

s/As a special case,/NB:/

> + * situation this function should set @data = false, @length = 0
> + * and return 0.

This is where it'd be nice to have that summary of possible @data and
@length values


> + *
> + * Returns 0 on success,
> + *        -1 otherwise
> + */
> +int
> +virStreamInData(virStreamPtr stream,
> +                int *data,

bool per previous discussion.

> +                unsigned long long *length)
> +{
> +    VIR_DEBUG("stream=%p, data=%p, length=%p", stream, data, length);
> +
> +    /* No checks of arguments or error resetting. This is an
> +     * internal function that just happen to live next to some
> +     * public functions of ours. */
> +
> +    if (stream->driver->streamInData) {
> +        int ret;
> +        ret = (stream->driver->streamInData)(stream, data, length);
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> +    return -1;
> +}
> +
> +
> +/**
>   * virStreamSendAll:
>   * @stream: pointer to the stream object
>   * @handler: source callback for reading data from application
> diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
> index 96439d8..0e945aa 100644
> --- a/src/libvirt_internal.h
> +++ b/src/libvirt_internal.h
> @@ -294,4 +294,7 @@ virTypedParameterValidateSet(virConnectPtr conn,
>                               virTypedParameterPtr params,
>                               int nparams);
>  
> +int virStreamInData(virStreamPtr stream,
> +                    int *data,
> +                    unsigned long long *lengtht);

s/lengtht/length/

Add an extra space before the #endif too.

(I'm just happy to know it's not only my fingers that keep trying extra
letters in places I don't expect!!)

This would be an ACK w/ minor adjustments.

John

>  #endif
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7132b3a..07633f2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1117,6 +1117,7 @@ virStateCleanup;
>  virStateInitialize;
>  virStateReload;
>  virStateStop;
> +virStreamInData;
>  
>  
>  # locking/domain_lock.h
>