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

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(a)redhat.com&gt; > --- > 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]