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

[Michal Privoznik]

On 05/16/2017 10:26 PM, John Ferlan wrote:
> 
> 
> 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.

A-ha! So this function might return uninitialized value (variable?),
which is propagated to virFDStreamThread where it hits an error. Well,
one one hand compiler tries to be smart (neither of those checks
including @got in the parent function cause the compilation error since
in that case @got is initialized); but on the other hand compiler fails
to see there's no way with the current code for msg->type to be
something different than MSG_TYPE_DATA in which case @got is set a value.
Sigh.

> 
>> +    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]

Actually, it doesn't matter. At this point, @fdst should have at least
one reference held by parent process (I mean the other thread that
spawned this thread). Not even in the case when unfair thread scheduling
occurs. I mean, imagine streamOpen() & streamClose() to be called
immediately one after another. steamOpen() spawns the thread, but lets
assume that the scheduler is unfair and does not schedule the thread for
a while. Well, streamClose() waits for the thread to join anyway,
therefore it doesn't really matter on ordering of lock() & ref() operations.

Michal