[Qemu-devel] [PULL 0/8] Block patches for curl

[Qemu-devel] [PULL 0/8] Block patches for curl

[Jeff Cody]

The following changes since commit 3a8760664d5c1a1a93c9012bdb8ac07ab8fd4b0d:

  Merge tag 'tracing-pull-request' into staging (2017-05-12 10:39:35 -0400)

are available in the git repository at:

  git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request

for you to fetch changes up to 2bb5c936c5827e1d831002f7a7517cb8c2c2201d:

  curl: do not do aio_poll when waiting for a free CURLState (2017-05-16 10:34:50 -0400)

----------------------------------------------------------------
Curl patches
----------------------------------------------------------------

Paolo Bonzini (7):
  curl: strengthen assertion in curl_clean_state
  curl: never invoke callbacks with s->mutex held
  curl: avoid recursive locking of BDRVCURLState mutex
  curl: split curl_find_state/curl_init_state
  curl: convert CURLAIOCB to byte values
  curl: convert readv to coroutines
  curl: do not do aio_poll when waiting for a free CURLState

Peter Krempa (1):
  block: curl: Allow passing cookies via QCryptoSecret

 block/curl.c         | 239 ++++++++++++++++++++++++++++++---------------------
 qapi/block-core.json |  12 ++-
 2 files changed, 153 insertions(+), 98 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/8] block: curl: Allow passing cookies via QCryptoSecret

[Jeff Cody]

From: Peter Krempa <pkrempa@redhat.com>

Since cookies can contain sensitive data (session ID, etc ...) it is
desired to hide them from the prying eyes of users. Add a possibility to
pass them via the secret infrastructure.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1447413

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: f4a22cdebdd0bca6a13a43a2a6deead7f2ec4bb3.1493906281.git.pkrempa@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c         | 24 +++++++++++++++++++++++-
 qapi/block-core.json | 12 ++++++++++--
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index aa6e8cc..4382234 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -85,6 +85,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
 #define CURL_BLOCK_OPT_TIMEOUT "timeout"
 #define CURL_BLOCK_OPT_COOKIE    "cookie"
+#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
 #define CURL_BLOCK_OPT_USERNAME "username"
 #define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
 #define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
@@ -624,6 +625,11 @@ static QemuOptsList runtime_opts = {
             .help = "Pass the cookie or list of cookies with each request"
         },
         {
+            .name = CURL_BLOCK_OPT_COOKIE_SECRET,
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret used as cookie passed with each request"
+        },
+        {
             .name = CURL_BLOCK_OPT_USERNAME,
             .type = QEMU_OPT_STRING,
             .help = "Username for HTTP auth"
@@ -657,6 +663,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     const char *file;
     const char *cookie;
+    const char *cookie_secret;
     double d;
     const char *secretid;
     const char *protocol_delimiter;
@@ -693,7 +700,22 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
 
     cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
-    s->cookie = g_strdup(cookie);
+    cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
+
+    if (cookie && cookie_secret) {
+        error_setg(errp,
+                   "curl driver cannot handle both cookie and cookie secret");
+        goto out_noclean;
+    }
+
+    if (cookie_secret) {
+        s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
+        if (!s->cookie) {
+            goto out_noclean;
+        }
+    } else {
+        s->cookie = g_strdup(cookie);
+    }
 
     file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
     if (file == NULL) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6b974b9..ea0b3e8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2813,11 +2813,15 @@
 #               "name1=content1; name2=content2;" as explained by
 #               CURLOPT_COOKIE(3). Defaults to no cookies.
 #
+# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
+#                 secure way. See @cookie for the format. (since 2.10)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurlHttp',
   'base': 'BlockdevOptionsCurlBase',
-  'data': { '*cookie': 'str' } }
+  'data': { '*cookie': 'str',
+            '*cookie-secret': 'str'} }
 
 ##
 # @BlockdevOptionsCurlHttps:
@@ -2832,12 +2836,16 @@
 # @sslverify:   Whether to verify the SSL certificate's validity (defaults to
 #               true)
 #
+# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
+#                 secure way. See @cookie for the format. (since 2.10)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurlHttps',
   'base': 'BlockdevOptionsCurlBase',
   'data': { '*cookie': 'str',
-            '*sslverify': 'bool' } }
+            '*sslverify': 'bool',
+            '*cookie-secret': 'str'} }
 
 ##
 # @BlockdevOptionsCurlFtp:
-- 
2.9.3

[Qemu-devel] [PULL 2/8] curl: strengthen assertion in curl_clean_state

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

curl_clean_state should only be called after all AIOCBs have been
completed.  This is not so obvious for the call from curl_detach_aio_context,
so assert that.

Cc: qemu-stable@nongnu.org
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20170515100059.15795-2-pbonzini@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 4382234..562340f 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -533,6 +533,11 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
 
 static void curl_clean_state(CURLState *s)
 {
+    int j;
+    for (j = 0; j < CURL_NUM_ACB; j++) {
+        assert(!s->acb[j]);
+    }
+
     if (s->s->multi)
         curl_multi_remove_handle(s->s->multi, s->curl);
 
-- 
2.9.3

[Qemu-devel] [PULL 3/8] curl: never invoke callbacks with s->mutex held

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

All curl callbacks go through curl_multi_do, and hence are called with
s->mutex held.  Note that with comments, and make curl_read_cb drop the
lock before invoking the callback.

Likewise for curl_find_buf, where the callback can be invoked by the
caller.

Cc: qemu-stable@nongnu.org
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20170515100059.15795-3-pbonzini@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 562340f..18b82bc 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -148,6 +148,7 @@ static void curl_multi_do(void *arg);
 static void curl_multi_read(void *arg);
 
 #ifdef NEED_CURL_TIMER_CALLBACK
+/* Called from curl_multi_do_locked, with s->mutex held.  */
 static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
 {
     BDRVCURLState *s = opaque;
@@ -164,6 +165,7 @@ static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
 }
 #endif
 
+/* Called from curl_multi_do_locked, with s->mutex held.  */
 static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
                         void *userp, void *sp)
 {
@@ -213,6 +215,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     return 0;
 }
 
+/* Called from curl_multi_do_locked, with s->mutex held.  */
 static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     BDRVCURLState *s = opaque;
@@ -227,6 +230,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
     return realsize;
 }
 
+/* Called from curl_multi_do_locked, with s->mutex held.  */
 static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     CURLState *s = ((CURLState*)opaque);
@@ -265,7 +269,9 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
                                   request_length - offset);
             }
 
+            qemu_mutex_unlock(&s->s->mutex);
             acb->common.cb(acb->common.opaque, 0);
+            qemu_mutex_lock(&s->s->mutex);
             qemu_aio_unref(acb);
             s->acb[i] = NULL;
         }
@@ -306,8 +312,6 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
             if (clamped_len < len) {
                 qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
             }
-            acb->common.cb(acb->common.opaque, 0);
-
             return FIND_RET_OK;
         }
 
@@ -854,8 +858,8 @@ static void curl_readv_bh_cb(void *p)
     // we can just call the callback and be done.
     switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
         case FIND_RET_OK:
-            qemu_aio_unref(acb);
-            // fall through
+            ret = 0;
+            goto out;
         case FIND_RET_WAIT:
             goto out;
         default:
-- 
2.9.3

[Qemu-devel] [PULL 4/8] curl: avoid recursive locking of BDRVCURLState mutex

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

The curl driver has a ugly hack where, if it cannot find an empty CURLState,
it just uses aio_poll to wait for one to be empty.  This is probably
buggy when used together with dataplane, and the simplest way to fix it
is to use coroutines instead.

A more immediate effect of the bug however is that it can cause a
recursive call to curl_readv_bh_cb and recursively taking the
BDRVCURLState mutex.  This causes a deadlock.

The fix is to unlock the mutex around aio_poll, but for cleanliness we
should also take the mutex around all calls to curl_init_state, even if
reaching the unlock/lock pair is impossible.  The same is true for
curl_clean_state.

Reported-by: Kun Wei <kuwei@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20170515100059.15795-4-pbonzini@redhat.com
Cc: qemu-stable@nongnu.org
Cc: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 18b82bc..c160810 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -282,6 +282,7 @@ read_end:
     return size * nmemb;
 }
 
+/* Called with s->mutex held.  */
 static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
                          CURLAIOCB *acb)
 {
@@ -454,6 +455,7 @@ static void curl_multi_timeout_do(void *arg)
 #endif
 }
 
+/* Called with s->mutex held.  */
 static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
 {
     CURLState *state = NULL;
@@ -472,7 +474,9 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
             break;
         }
         if (!state) {
+            qemu_mutex_unlock(&s->mutex);
             aio_poll(bdrv_get_aio_context(bs), true);
+            qemu_mutex_lock(&s->mutex);
         }
     } while(!state);
 
@@ -535,6 +539,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
     return state;
 }
 
+/* Called with s->mutex held.  */
 static void curl_clean_state(CURLState *s)
 {
     int j;
@@ -566,6 +571,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
     BDRVCURLState *s = bs->opaque;
     int i;
 
+    qemu_mutex_lock(&s->mutex);
     for (i = 0; i < CURL_NUM_STATES; i++) {
         if (s->states[i].in_use) {
             curl_clean_state(&s->states[i]);
@@ -581,6 +587,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
         curl_multi_cleanup(s->multi);
         s->multi = NULL;
     }
+    qemu_mutex_unlock(&s->mutex);
 
     timer_del(&s->timer);
 }
@@ -684,6 +691,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
         return -EROFS;
     }
 
+    qemu_mutex_init(&s->mutex);
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
@@ -769,7 +777,9 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     DPRINTF("CURL: Opening %s\n", file);
     s->aio_context = bdrv_get_aio_context(bs);
     s->url = g_strdup(file);
+    qemu_mutex_lock(&s->mutex);
     state = curl_init_state(bs, s);
+    qemu_mutex_unlock(&s->mutex);
     if (!state)
         goto out_noclean;
 
@@ -813,11 +823,12 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     }
     DPRINTF("CURL: Size = %zd\n", s->len);
 
+    qemu_mutex_lock(&s->mutex);
     curl_clean_state(state);
+    qemu_mutex_unlock(&s->mutex);
     curl_easy_cleanup(state->curl);
     state->curl = NULL;
 
-    qemu_mutex_init(&s->mutex);
     curl_attach_aio_context(bs, bdrv_get_aio_context(bs));
 
     qemu_opts_del(opts);
@@ -828,6 +839,7 @@ out:
     curl_easy_cleanup(state->curl);
     state->curl = NULL;
 out_noclean:
+    qemu_mutex_destroy(&s->mutex);
     g_free(s->cookie);
     g_free(s->url);
     qemu_opts_del(opts);
-- 
2.9.3

[Qemu-devel] [PULL 5/8] curl: split curl_find_state/curl_init_state

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

If curl_easy_init fails, a CURLState is left with s->in_use = 1.  Split
curl_init_state in two, so that we can distinguish the two failures and
call curl_clean_state if needed.

While at it, simplify curl_find_state, removing a dummy loop.  The
aio_poll loop is moved to the sole caller that needs it.

Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20170515100059.15795-5-pbonzini@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index c160810..a522381 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -456,34 +456,27 @@ static void curl_multi_timeout_do(void *arg)
 }
 
 /* Called with s->mutex held.  */
-static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
+static CURLState *curl_find_state(BDRVCURLState *s)
 {
     CURLState *state = NULL;
-    int i, j;
-
-    do {
-        for (i=0; i<CURL_NUM_STATES; i++) {
-            for (j=0; j<CURL_NUM_ACB; j++)
-                if (s->states[i].acb[j])
-                    continue;
-            if (s->states[i].in_use)
-                continue;
+    int i;
 
+    for (i = 0; i < CURL_NUM_STATES; i++) {
+        if (!s->states[i].in_use) {
             state = &s->states[i];
             state->in_use = 1;
             break;
         }
-        if (!state) {
-            qemu_mutex_unlock(&s->mutex);
-            aio_poll(bdrv_get_aio_context(bs), true);
-            qemu_mutex_lock(&s->mutex);
-        }
-    } while(!state);
+    }
+    return state;
+}
 
+static int curl_init_state(BDRVCURLState *s, CURLState *state)
+{
     if (!state->curl) {
         state->curl = curl_easy_init();
         if (!state->curl) {
-            return NULL;
+            return -EIO;
         }
         curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
         curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
@@ -536,7 +529,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
     QLIST_INIT(&state->sockets);
     state->s = s;
 
-    return state;
+    return 0;
 }
 
 /* Called with s->mutex held.  */
@@ -778,13 +771,18 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     s->aio_context = bdrv_get_aio_context(bs);
     s->url = g_strdup(file);
     qemu_mutex_lock(&s->mutex);
-    state = curl_init_state(bs, s);
+    state = curl_find_state(s);
     qemu_mutex_unlock(&s->mutex);
-    if (!state)
+    if (!state) {
         goto out_noclean;
+    }
 
     // Get file size
 
+    if (curl_init_state(s, state) < 0) {
+        goto out;
+    }
+
     s->accept_range = false;
     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
     curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
@@ -879,8 +877,18 @@ static void curl_readv_bh_cb(void *p)
     }
 
     // No cache found, so let's start a new request
-    state = curl_init_state(acb->common.bs, s);
-    if (!state) {
+    for (;;) {
+        state = curl_find_state(s);
+        if (state) {
+            break;
+        }
+        qemu_mutex_unlock(&s->mutex);
+        aio_poll(bdrv_get_aio_context(bs), true);
+        qemu_mutex_lock(&s->mutex);
+    }
+
+    if (curl_init_state(s, state) < 0) {
+        curl_clean_state(state);
         ret = -EIO;
         goto out;
     }
-- 
2.9.3

[Qemu-devel] [PULL 6/8] curl: convert CURLAIOCB to byte values

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

This is in preparation for the conversion from bdrv_aio_readv to
bdrv_co_preadv, and it also requires changing some of the size_t values
to uint64_t.  This was broken before for disks > 2TB, but now it would
break at 4GB.

Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20170515100059.15795-6-pbonzini@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index a522381..a71a428 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -97,8 +97,8 @@ typedef struct CURLAIOCB {
     BlockAIOCB common;
     QEMUIOVector *qiov;
 
-    int64_t sector_num;
-    int nb_sectors;
+    uint64_t offset;
+    uint64_t bytes;
 
     size_t start;
     size_t end;
@@ -116,7 +116,7 @@ typedef struct CURLState
     CURL *curl;
     QLIST_HEAD(, CURLSocket) sockets;
     char *orig_buf;
-    size_t buf_start;
+    uint64_t buf_start;
     size_t buf_off;
     size_t buf_len;
     char range[128];
@@ -127,7 +127,7 @@ typedef struct CURLState
 typedef struct BDRVCURLState {
     CURLM *multi;
     QEMUTimer timer;
-    size_t len;
+    uint64_t len;
     CURLState states[CURL_NUM_STATES];
     char *url;
     size_t readahead_size;
@@ -258,7 +258,7 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
             continue;
 
         if ((s->buf_off >= acb->end)) {
-            size_t request_length = acb->nb_sectors * BDRV_SECTOR_SIZE;
+            size_t request_length = acb->bytes;
 
             qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
                                 acb->end - acb->start);
@@ -283,18 +283,18 @@ read_end:
 }
 
 /* Called with s->mutex held.  */
-static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
+static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
                          CURLAIOCB *acb)
 {
     int i;
-    size_t end = start + len;
-    size_t clamped_end = MIN(end, s->len);
-    size_t clamped_len = clamped_end - start;
+    uint64_t end = start + len;
+    uint64_t clamped_end = MIN(end, s->len);
+    uint64_t clamped_len = clamped_end - start;
 
     for (i=0; i<CURL_NUM_STATES; i++) {
         CURLState *state = &s->states[i];
-        size_t buf_end = (state->buf_start + state->buf_off);
-        size_t buf_fend = (state->buf_start + state->buf_len);
+        uint64_t buf_end = (state->buf_start + state->buf_off);
+        uint64_t buf_fend = (state->buf_start + state->buf_len);
 
         if (!state->orig_buf)
             continue;
@@ -810,7 +810,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     }
 #endif
 
-    s->len = (size_t)d;
+    s->len = d;
 
     if ((!strncasecmp(s->url, "http://", strlen("http://"))
         || !strncasecmp(s->url, "https://", strlen("https://")))
@@ -819,7 +819,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
                 "Server does not support 'range' (byte ranges).");
         goto out;
     }
-    DPRINTF("CURL: Size = %zd\n", s->len);
+    DPRINTF("CURL: Size = %" PRIu64 "\n", s->len);
 
     qemu_mutex_lock(&s->mutex);
     curl_clean_state(state);
@@ -859,14 +859,14 @@ static void curl_readv_bh_cb(void *p)
     BlockDriverState *bs = acb->common.bs;
     BDRVCURLState *s = bs->opaque;
 
-    size_t start = acb->sector_num * BDRV_SECTOR_SIZE;
-    size_t end;
+    uint64_t start = acb->offset;
+    uint64_t end;
 
     qemu_mutex_lock(&s->mutex);
 
     // In case we have the requested data already (e.g. read-ahead),
     // we can just call the callback and be done.
-    switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
+    switch (curl_find_buf(s, start, acb->bytes, acb)) {
         case FIND_RET_OK:
             ret = 0;
             goto out;
@@ -894,7 +894,7 @@ static void curl_readv_bh_cb(void *p)
     }
 
     acb->start = 0;
-    acb->end = MIN(acb->nb_sectors * BDRV_SECTOR_SIZE, s->len - start);
+    acb->end = MIN(acb->bytes, s->len - start);
 
     state->buf_off = 0;
     g_free(state->orig_buf);
@@ -909,9 +909,9 @@ static void curl_readv_bh_cb(void *p)
     }
     state->acb[0] = acb;
 
-    snprintf(state->range, 127, "%zd-%zd", start, end);
-    DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
-            (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);
+    snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
+    DPRINTF("CURL (AIO): Reading %" PRIu64 " at %" PRIu64 " (%s)\n",
+            acb->bytes, start, state->range);
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
 
     curl_multi_add_handle(s->multi, state->curl);
@@ -936,8 +936,8 @@ static BlockAIOCB *curl_aio_readv(BlockDriverState *bs,
     acb = qemu_aio_get(&curl_aiocb_info, bs, cb, opaque);
 
     acb->qiov = qiov;
-    acb->sector_num = sector_num;
-    acb->nb_sectors = nb_sectors;
+    acb->offset = sector_num * BDRV_SECTOR_SIZE;
+    acb->bytes = nb_sectors * BDRV_SECTOR_SIZE;
 
     aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), curl_readv_bh_cb, acb);
     return &acb->common;
-- 
2.9.3

[Qemu-devel] [PULL 7/8] curl: convert readv to coroutines

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

This is pretty simple.  The bottom half goes away because, unlike
bdrv_aio_readv, coroutine-based read can return immediately without
yielding.  However, for simplicity I kept the former bottom half
handler in a separate function.

Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20170515100059.15795-7-pbonzini@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 92 ++++++++++++++++++++++++------------------------------------
 1 file changed, 37 insertions(+), 55 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index a71a428..1c04903 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -76,10 +76,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 #define CURL_TIMEOUT_DEFAULT 5
 #define CURL_TIMEOUT_MAX 10000
 
-#define FIND_RET_NONE   0
-#define FIND_RET_OK     1
-#define FIND_RET_WAIT   2
-
 #define CURL_BLOCK_OPT_URL       "url"
 #define CURL_BLOCK_OPT_READAHEAD "readahead"
 #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
@@ -94,11 +90,12 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 struct BDRVCURLState;
 
 typedef struct CURLAIOCB {
-    BlockAIOCB common;
+    Coroutine *co;
     QEMUIOVector *qiov;
 
     uint64_t offset;
     uint64_t bytes;
+    int ret;
 
     size_t start;
     size_t end;
@@ -269,11 +266,11 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
                                   request_length - offset);
             }
 
+            acb->ret = 0;
+            s->acb[i] = NULL;
             qemu_mutex_unlock(&s->s->mutex);
-            acb->common.cb(acb->common.opaque, 0);
+            aio_co_wake(acb->co);
             qemu_mutex_lock(&s->s->mutex);
-            qemu_aio_unref(acb);
-            s->acb[i] = NULL;
         }
     }
 
@@ -283,8 +280,8 @@ read_end:
 }
 
 /* Called with s->mutex held.  */
-static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
-                         CURLAIOCB *acb)
+static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
+                          CURLAIOCB *acb)
 {
     int i;
     uint64_t end = start + len;
@@ -313,7 +310,8 @@ static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
             if (clamped_len < len) {
                 qemu_iovec_memset(acb->qiov, clamped_len, 0, len - clamped_len);
             }
-            return FIND_RET_OK;
+            acb->ret = 0;
+            return true;
         }
 
         // Wait for unfinished chunks
@@ -331,13 +329,13 @@ static int curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
             for (j=0; j<CURL_NUM_ACB; j++) {
                 if (!state->acb[j]) {
                     state->acb[j] = acb;
-                    return FIND_RET_WAIT;
+                    return true;
                 }
             }
         }
     }
 
-    return FIND_RET_NONE;
+    return false;
 }
 
 /* Called with s->mutex held.  */
@@ -382,11 +380,11 @@ static void curl_multi_check_completion(BDRVCURLState *s)
                         continue;
                     }
 
+                    acb->ret = -EIO;
+                    state->acb[i] = NULL;
                     qemu_mutex_unlock(&s->mutex);
-                    acb->common.cb(acb->common.opaque, -EIO);
+                    aio_co_wake(acb->co);
                     qemu_mutex_lock(&s->mutex);
-                    qemu_aio_unref(acb);
-                    state->acb[i] = NULL;
                 }
             }
 
@@ -844,19 +842,11 @@ out_noclean:
     return -EINVAL;
 }
 
-static const AIOCBInfo curl_aiocb_info = {
-    .aiocb_size         = sizeof(CURLAIOCB),
-};
-
-
-static void curl_readv_bh_cb(void *p)
+static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
 {
     CURLState *state;
     int running;
-    int ret = -EINPROGRESS;
 
-    CURLAIOCB *acb = p;
-    BlockDriverState *bs = acb->common.bs;
     BDRVCURLState *s = bs->opaque;
 
     uint64_t start = acb->offset;
@@ -866,14 +856,8 @@ static void curl_readv_bh_cb(void *p)
 
     // In case we have the requested data already (e.g. read-ahead),
     // we can just call the callback and be done.
-    switch (curl_find_buf(s, start, acb->bytes, acb)) {
-        case FIND_RET_OK:
-            ret = 0;
-            goto out;
-        case FIND_RET_WAIT:
-            goto out;
-        default:
-            break;
+    if (curl_find_buf(s, start, acb->bytes, acb)) {
+        goto out;
     }
 
     // No cache found, so let's start a new request
@@ -889,7 +873,7 @@ static void curl_readv_bh_cb(void *p)
 
     if (curl_init_state(s, state) < 0) {
         curl_clean_state(state);
-        ret = -EIO;
+        acb->ret = -EIO;
         goto out;
     }
 
@@ -904,7 +888,7 @@ static void curl_readv_bh_cb(void *p)
     state->orig_buf = g_try_malloc(state->buf_len);
     if (state->buf_len && state->orig_buf == NULL) {
         curl_clean_state(state);
-        ret = -ENOMEM;
+        acb->ret = -ENOMEM;
         goto out;
     }
     state->acb[0] = acb;
@@ -921,26 +905,24 @@ static void curl_readv_bh_cb(void *p)
 
 out:
     qemu_mutex_unlock(&s->mutex);
-    if (ret != -EINPROGRESS) {
-        acb->common.cb(acb->common.opaque, ret);
-        qemu_aio_unref(acb);
-    }
 }
 
-static BlockAIOCB *curl_aio_readv(BlockDriverState *bs,
-        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
-        BlockCompletionFunc *cb, void *opaque)
+static int coroutine_fn curl_co_preadv(BlockDriverState *bs,
+        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
-    CURLAIOCB *acb;
+    CURLAIOCB acb = {
+        .co = qemu_coroutine_self(),
+        .ret = -EINPROGRESS,
+        .qiov = qiov,
+        .offset = offset,
+        .bytes = bytes
+    };
 
-    acb = qemu_aio_get(&curl_aiocb_info, bs, cb, opaque);
-
-    acb->qiov = qiov;
-    acb->offset = sector_num * BDRV_SECTOR_SIZE;
-    acb->bytes = nb_sectors * BDRV_SECTOR_SIZE;
-
-    aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), curl_readv_bh_cb, acb);
-    return &acb->common;
+    curl_setup_preadv(bs, &acb);
+    while (acb.ret == -EINPROGRESS) {
+        qemu_coroutine_yield();
+    }
+    return acb.ret;
 }
 
 static void curl_close(BlockDriverState *bs)
@@ -971,7 +953,7 @@ static BlockDriver bdrv_http = {
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
-    .bdrv_aio_readv             = curl_aio_readv,
+    .bdrv_co_preadv             = curl_co_preadv,
 
     .bdrv_detach_aio_context    = curl_detach_aio_context,
     .bdrv_attach_aio_context    = curl_attach_aio_context,
@@ -987,7 +969,7 @@ static BlockDriver bdrv_https = {
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
-    .bdrv_aio_readv             = curl_aio_readv,
+    .bdrv_co_preadv             = curl_co_preadv,
 
     .bdrv_detach_aio_context    = curl_detach_aio_context,
     .bdrv_attach_aio_context    = curl_attach_aio_context,
@@ -1003,7 +985,7 @@ static BlockDriver bdrv_ftp = {
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
-    .bdrv_aio_readv             = curl_aio_readv,
+    .bdrv_co_preadv             = curl_co_preadv,
 
     .bdrv_detach_aio_context    = curl_detach_aio_context,
     .bdrv_attach_aio_context    = curl_attach_aio_context,
@@ -1019,7 +1001,7 @@ static BlockDriver bdrv_ftps = {
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
-    .bdrv_aio_readv             = curl_aio_readv,
+    .bdrv_co_preadv             = curl_co_preadv,
 
     .bdrv_detach_aio_context    = curl_detach_aio_context,
     .bdrv_attach_aio_context    = curl_attach_aio_context,
-- 
2.9.3

[Qemu-devel] [PULL 8/8] curl: do not do aio_poll when waiting for a free CURLState

[Jeff Cody]

From: Paolo Bonzini <pbonzini@redhat.com>

Instead, put the CURLAIOCB on a wait list and yield; curl_clean_state will
wake the corresponding coroutine.

Because of CURL's callback-based structure, we cannot easily convert
everything to CoMutex/CoQueue; keeping the QemuMutex is simpler.  However,
CoQueue is a simple wrapper around a linked list, so we can easily
use QSIMPLEQ and open-code a CoQueue, protected by the BDRVCURLState
QemuMutex instead of a CoMutex.

Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20170515100059.15795-8-pbonzini@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/curl.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 1c04903..2a244e2 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -99,6 +99,8 @@ typedef struct CURLAIOCB {
 
     size_t start;
     size_t end;
+
+    QSIMPLEQ_ENTRY(CURLAIOCB) next;
 } CURLAIOCB;
 
 typedef struct CURLSocket {
@@ -134,6 +136,7 @@ typedef struct BDRVCURLState {
     bool accept_range;
     AioContext *aio_context;
     QemuMutex mutex;
+    QSIMPLEQ_HEAD(, CURLAIOCB) free_state_waitq;
     char *username;
     char *password;
     char *proxyusername;
@@ -533,6 +536,7 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
 /* Called with s->mutex held.  */
 static void curl_clean_state(CURLState *s)
 {
+    CURLAIOCB *next;
     int j;
     for (j = 0; j < CURL_NUM_ACB; j++) {
         assert(!s->acb[j]);
@@ -549,6 +553,14 @@ static void curl_clean_state(CURLState *s)
     }
 
     s->in_use = 0;
+
+    next = QSIMPLEQ_FIRST(&s->s->free_state_waitq);
+    if (next) {
+        QSIMPLEQ_REMOVE_HEAD(&s->s->free_state_waitq, next);
+        qemu_mutex_unlock(&s->s->mutex);
+        aio_co_wake(next->co);
+        qemu_mutex_lock(&s->s->mutex);
+    }
 }
 
 static void curl_parse_filename(const char *filename, QDict *options,
@@ -766,6 +778,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     DPRINTF("CURL: Opening %s\n", file);
+    QSIMPLEQ_INIT(&s->free_state_waitq);
     s->aio_context = bdrv_get_aio_context(bs);
     s->url = g_strdup(file);
     qemu_mutex_lock(&s->mutex);
@@ -866,8 +879,9 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
         if (state) {
             break;
         }
+        QSIMPLEQ_INSERT_TAIL(&s->free_state_waitq, acb, next);
         qemu_mutex_unlock(&s->mutex);
-        aio_poll(bdrv_get_aio_context(bs), true);
+        qemu_coroutine_yield();
         qemu_mutex_lock(&s->mutex);
     }
 
-- 
2.9.3

Re: [Qemu-devel] [Qemu-block] [PULL 0/8] Block patches for curl

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On Tue, May 16, 2017 at 11:54:12AM -0400, Jeff Cody wrote:
> The following changes since commit 3a8760664d5c1a1a93c9012bdb8ac07ab8fd4b0d:
> 
>   Merge tag 'tracing-pull-request' into staging (2017-05-12 10:39:35 -0400)
> 
> are available in the git repository at:
> 
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
> 
> for you to fetch changes up to 2bb5c936c5827e1d831002f7a7517cb8c2c2201d:
> 
>   curl: do not do aio_poll when waiting for a free CURLState (2017-05-16 10:34:50 -0400)
> 
> ----------------------------------------------------------------
> Curl patches
> ----------------------------------------------------------------
> 
> Paolo Bonzini (7):
>   curl: strengthen assertion in curl_clean_state
>   curl: never invoke callbacks with s->mutex held
>   curl: avoid recursive locking of BDRVCURLState mutex
>   curl: split curl_find_state/curl_init_state
>   curl: convert CURLAIOCB to byte values
>   curl: convert readv to coroutines
>   curl: do not do aio_poll when waiting for a free CURLState
> 
> Peter Krempa (1):
>   block: curl: Allow passing cookies via QCryptoSecret
> 
>  block/curl.c         | 239 ++++++++++++++++++++++++++++++---------------------
>  qapi/block-core.json |  12 ++-
>  2 files changed, 153 insertions(+), 98 deletions(-)
> 
> -- 
> 2.9.3
> 
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]