[libvirt] [PATCH 1/2] esx: Add libcurl based stream driver

[Matthias Bolte]

2014-03-31 15:46 GMT+02:00 Michal Privoznik <mprivozn at redhat.com>:
>
> On 30.03.2014 21:03, Matthias Bolte wrote:
>>
>> This allows to implement libvirt functions that use streams, such as
>> virDoaminScreenshot, without the need to store the downloaded data in
>> a temporary file first. The stream driver directly interacts with
>> libcurl to send and receive data.
>>
>> The driver uses the libcurl multi interface that allows to do a transfer
>> in multiple curl_multi_perform() calls. The easy interface would do the
>> whole transfer in a single curl_easy_perform() call. This doesn't work
>> with the libvirt stream API that is driven by multiple calls to the
>> virStreamSend() and virStreamRecv() functions.
>>
>> The curl_multi_wait() function is used to do blocking operations. But it
>> was added in libcurl 7.28.0. For older versions it is emulated using the
>> socket callback of the multi interface.
>>
>> The current driver only supports blocking operations. There is already
>> some code in place for non-blocking mode but it's incomplete. As you can
>> tell from the copyright date I implemeted this in 2012, but never came
>> around to publish it then. I did some work in 2013 and now it's 2014 and
>> I don't want to hold it back any longer.
>> ---
>>   po/POTFILES.in       |   1 +
>>   src/Makefile.am      |   1 +
>>   src/esx/esx_stream.c | 478 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/esx/esx_stream.h |  32 ++++
>>   src/esx/esx_vi.c     | 222 +++++++++++++++++++++++-
>>   src/esx/esx_vi.h     |  19 +-
>>   6 files changed, 749 insertions(+), 4 deletions(-)
>>   create mode 100644 src/esx/esx_stream.c
>>   create mode 100644 src/esx/esx_stream.h
>>
>>
>
>> +static int
>> +esxStreamClose(virStreamPtr stream, bool finish)
>> +{
>> +    int result = 0;
>> +    esxStreamPrivate *priv = stream->privateData;
>> +
>> +    if (!priv)
>> +        return 0;
>> +
>> +    virMutexLock(&priv->curl->lock);
>> +
>> +    if (finish && priv->backlog_used > 0) {
>
>
> I think you want to unlock the curl lock here.
>

No, because there is no return statement in the if block, so the
unlock call after the if block is sufficient.

>>
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Stream has untransferred data left"));
>> +        result = -1;
>> +    }
>> +
>> +    stream->privateData = NULL;
>> +
>> +    virMutexUnlock(&priv->curl->lock);
>> +
>> +    esxFreeStreamPrivate(&priv);
>> +
>> +    return result;
>> +}
>> +
>
>
>> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
>> index 6188139..ba34bfd 100644
>> --- a/src/esx/esx_vi.c
>> +++ b/src/esx/esx_vi.c
>> @@ -2,7 +2,7 @@
>>    * esx_vi.c: client for the VMware VI API 2.5 to manage ESX hosts
>>    *
>>    * Copyright (C) 2010-2012 Red Hat, Inc.
>> - * Copyright (C) 2009-2012 Matthias Bolte <matthias.bolte at googlemail.com>
>> + * Copyright (C) 2009-2012, 2014 Matthias Bolte <matthias.bolte at googlemail.com>
>>    *
>>    * This library is free software; you can redistribute it and/or
>>    * modify it under the terms of the GNU Lesser General Public
>> @@ -22,6 +22,7 @@
>>
>>   #include <config.h>
>>
>> +#include <poll.h>
>>   #include <libxml/parser.h>
>>   #include <libxml/xpathInternals.h>
>>
>> @@ -662,6 +663,68 @@ esxVI_SharedCURL_Remove(esxVI_SharedCURL *shared, esxVI_CURL *curl)
>>    * MultiCURL
>>    */
>>
>> +#if ESX_EMULATE_CURL_MULTI_WAIT
>> +
>> +static int
>> +esxVI_MultiCURL_SocketCallback(CURL *handle ATTRIBUTE_UNUSED,
>> +                               curl_socket_t fd, int action,
>> +                               void *callback_opaque,
>> +                               void *socket_opaque ATTRIBUTE_UNUSED)
>> +{
>> +    esxVI_MultiCURL *multi = callback_opaque;
>> +    size_t i;
>> +    struct pollfd *pollfd = NULL;
>> +    struct pollfd dummy;
>> +
>> +    if (action & CURL_POLL_REMOVE) {
>> +        for (i = 0; i < multi->npollfds; ++i) {
>> +            if (multi->pollfds[i].fd == fd) {
>> +                VIR_DELETE_ELEMENT(multi->pollfds, i, multi->npollfds);
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        for (i = 0; i < multi->npollfds; ++i) {
>> +            if (multi->pollfds[i].fd == fd) {
>> +                pollfd = &multi->pollfds[i];
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (pollfd == NULL) {
>> +            if (VIR_APPEND_ELEMENT(multi->pollfds, multi->npollfds, dummy) < 0) {
>> +                return 0;
>
>
> Okay, this is strange. But I see why you can't return -1. From the curl_multi_socket() documentation:
>
> "The callback MUST return 0."

I added a comment about this now.

>> +            }
>> +
>> +            pollfd = &multi->pollfds[multi->npollfds - 1];
>> +        }
>> +
>> +        pollfd->fd = fd;
>> +        pollfd->events = 0;
>> +
>> +        if (action & CURL_POLL_IN)
>> +            pollfd->events |= POLLIN;
>> +
>> +        if (action & CURL_POLL_OUT)
>> +            pollfd->events |= POLLOUT;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>
>
> ACK with the nits fixed.
>
> Michal

Thanks, pushed... finally :)

-- 
Matthias Bolte
http://photron.blogspot.com