[libvirt] [PATCH v3 01/31] virfdstream: Use messages instead of pipe

[John Ferlan]

On 05/16/2017 10:03 AM, Michal Privoznik wrote:
> One big downside of using the pipe to transfer the data is that
> we can really transfer just bare data. No metadata can be carried
> through unless some formatted messages are introduced. That would
> be quite painful to achieve so let's use a message queue. It's
> fairly easy to exchange info between threads now that iohelper is
> no longer used.
> 
> The reason why we cannot use the FD for plain files directly is
> that despite us setting noblock flag on the FD, any
> read()/write() blocks regardless (which is a show stopper since
> those parts of the code are run from the event loop) and poll()
> reports such FD as always readable/writable - even though the
> subsequent operation might block.
> 
> The pipe is still not gone though. It is used to signal to even

s/to even/the event/

> loop that an event occurred (e.g. data are available for reading

s/are/is   (yes, an oddity of the language)

> in the queue, or vice versa).
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virfdstream.c | 402 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 350 insertions(+), 52 deletions(-)
> 

I'm still getting a compilation error on this patch...

util/virfdstream.c: In function 'virFDStreamThread':
util/virfdstream.c:551:15: error: 'got' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
         total += got;
               ^~

> diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
> index 5ce78fe58..4b42939e7 100644
> --- a/src/util/virfdstream.c
> +++ b/src/util/virfdstream.c
> @@ -49,6 +49,27 @@

[...]

> +static ssize_t
> +virFDStreamThreadDoWrite(virFDStreamDataPtr fdst,
> +                         const int fdin,
> +                         const int fdout,
> +                         const char *fdinname,
> +                         const char *fdoutname)
> +{
> +    ssize_t got;

got = 0;

Fixes the compilation issue since got is only set for MSG_TYPE_DATA and
even though there is only that type, the compiler seems to somehow
believe it could be set ambiguously.

> +    virFDStreamMsgPtr msg = fdst->msg;
> +    bool pop = false;
> +
> +    switch (msg->type) {
> +    case VIR_FDSTREAM_MSG_TYPE_DATA:
> +        got = safewrite(fdout,
> +                        msg->stream.data.buf + msg->stream.data.offset,
> +                        msg->stream.data.len - msg->stream.data.offset);
> +        if (got < 0) {
> +            virReportSystemError(errno,
> +                                 _("Unable to write %s"),
> +                                 fdoutname);
> +            return -1;
> +        }
> +
> +        msg->stream.data.offset += got;
> +
> +        pop = msg->stream.data.offset == msg->stream.data.len;
> +        break;
> +    }
> +
> +    if (pop) {
> +        virFDStreamMsgQueuePop(fdst, fdin, fdinname);
> +        virFDStreamMsgFree(msg);
> +    }
> +
> +    return got;
> +}
> +
> +
>  static void
>  virFDStreamThread(void *opaque)
>  {
> @@ -304,14 +496,12 @@ virFDStreamThread(void *opaque)
>      int fdout = data->fdout;
>      char *fdoutname = data->fdoutname;
>      virFDStreamDataPtr fdst = st->privateData;
> -    char *buf = NULL;
> +    bool doRead = fdst->threadDoRead;

Should the fdst ref come eafter the ObjectLock(fdst) below? [1]

>      size_t buflen = 256 * 1024;
>      size_t total = 0;
>  
>      virObjectRef(fdst);
> -
> -    if (VIR_ALLOC_N(buf, buflen) < 0)
> -        goto error;
> +    virObjectLock(fdst);

^^^ [1]

Reviewed-by: John Ferlan <jferlan at redhat.com>


John

[...]