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

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.

> > + 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(a)googlemail.com&gt; > + * Copyright (C) 2009-2012, 2014 Matthias Bolte <matthias.bolte(a)googlemail.com&gt; > * > * 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."

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