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>
---
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(a)redhat.com>
John
[...]