[Qemu-devel] [PATCH 0/4] block/curl: Fix FTP

[Qemu-devel] [PATCH 0/4] block/curl: Fix FTP

[Max Reitz]

At least for me, the FTP support of our curl block driver currently
doesn't work at all. This is due to (at least) three issues, for each of
which this series provides a patch (and the first patch is just a minor
clean-up).

1. When establishing an FTP connection, libcurl hands us some data we do
   not expect because we have not really been asking for it. Not an
   issue in theory, because we can just ignore it. Unfortunately, qemu
   has decided to be more direct about the issue and tell libcurl that
   we did not process any of that data. libcurl doesn't like that. At
   all. Therefore, it returns the favor and just cancels the connection.
   In effect, it is impossible to even open a connection to an FTP
   server (at least in my test environment).
   Patch 2 fixes this by just playing along nicely.

2. libcurl has an old function called curl_multi_socket_all(). It allows
   you to kick off action on all of the sockets there are.
   Unfortunately, it is deprecated. Therefore, our code decides to be
   good and use the non-deprecated curl_multi_socket_action() function.
   However, that one only works on a single socket and wants to know
   which. So our code remembers the socket of the current connection.
   Works great for HTTP which generally only uses one socket.
   Unfortunately, FTP generally uses at least two, one for the control
   and one for the data stream. So us remembering only one of those two
   results in qemu only being able to receive the first 16 kB of any
   request (and maybe even of any connection).
   Patch 3 fixes this by putting the sockets into a list and thus being
   able to remember more than one.

3. The first two patches make curl work on files with file sizes that
   are multiples of 512, but not so well with others. curl still uses
   the sector-based interface, so it may receive requests beyond the
   EOF into the partial last sector. While it will actually not pass a
   request beyond the EOF to libcurl, it will unfortunately still wait
   to receive data from there. Which of course will not happen. So every
   request into that last sector makes the whole BDS hang indefinitely.
   Patch 4 fixes this by letting go of the futile hope for data from
   where there is none.


Max Reitz (4):
  block/curl: Use BDRV_SECTOR_SIZE
  block/curl: Fix return value from curl_read_cb
  block/curl: Remember all sockets
  block/curl: Do not wait for data beyond EOF

 block/curl.c | 99 +++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 75 insertions(+), 24 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH 1/4] block/curl: Use BDRV_SECTOR_SIZE

[Max Reitz]

Currently, curl defines its own constant SECTOR_SIZE. There is no
advantage over using the global BDRV_SECTOR_SIZE, so drop it.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index e5eaa7b..12afa15 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -73,7 +73,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 
 #define CURL_NUM_STATES 8
 #define CURL_NUM_ACB    8
-#define SECTOR_SIZE     512
 #define READ_AHEAD_DEFAULT (256 * 1024)
 #define CURL_TIMEOUT_DEFAULT 5
 #define CURL_TIMEOUT_MAX 10000
@@ -738,12 +737,12 @@ static void curl_readv_bh_cb(void *p)
     CURLAIOCB *acb = p;
     BDRVCURLState *s = acb->common.bs->opaque;
 
-    size_t start = acb->sector_num * SECTOR_SIZE;
+    size_t start = acb->sector_num * BDRV_SECTOR_SIZE;
     size_t end;
 
     // 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 * SECTOR_SIZE, acb)) {
+    switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
         case FIND_RET_OK:
             qemu_aio_unref(acb);
             // fall through
@@ -762,7 +761,7 @@ static void curl_readv_bh_cb(void *p)
     }
 
     acb->start = 0;
-    acb->end = (acb->nb_sectors * SECTOR_SIZE);
+    acb->end = (acb->nb_sectors * BDRV_SECTOR_SIZE);
 
     state->buf_off = 0;
     g_free(state->orig_buf);
@@ -779,8 +778,8 @@ static void curl_readv_bh_cb(void *p)
     state->acb[0] = acb;
 
     snprintf(state->range, 127, "%zd-%zd", start, end);
-    DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
-            (acb->nb_sectors * SECTOR_SIZE), start, state->range);
+    DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
+            (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);
     curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
 
     curl_multi_add_handle(s->multi, state->curl);
-- 
2.10.1

[Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Max Reitz]

While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in that
the callback is supposed to return the number of bytes handled; what it
does not mention is that libcurl will throw an error if the callback did
not "handle" all of the data passed to it.

Therefore, if the callback receives some data that it cannot handle
(either because the receive buffer has not been set up yet or because it
would not fit into the receive buffer) and we have to ignore it, we
still have to report that the data has been handled.

Obviously, this should not happen normally. But it does happen at least
for FTP connections where some data (that we do not expect) may be
generated when the connection is established.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 12afa15..095ffda 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -212,12 +212,13 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 
     DPRINTF("CURL: Just reading %zd bytes\n", realsize);
 
-    if (!s || !s->orig_buf)
-        return 0;
+    if (!s || !s->orig_buf) {
+        goto read_end;
+    }
 
     if (s->buf_off >= s->buf_len) {
         /* buffer full, read nothing */
-        return 0;
+        goto read_end;
     }
     realsize = MIN(realsize, s->buf_len - s->buf_off);
     memcpy(s->orig_buf + s->buf_off, ptr, realsize);
@@ -238,7 +239,9 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
         }
     }
 
-    return realsize;
+read_end:
+    /* curl will error out if we do not return this value */
+    return size * nmemb;
 }
 
 static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
-- 
2.10.1

[Qemu-devel] [PATCH 3/4] block/curl: Remember all sockets

[Max Reitz]

For some connection types (like FTP, generally), more than one socket
may be used (in FTP's case: control vs. data stream). As of commit
838ef602498b8d1985a231a06f5e328e2946a81d ("curl: Eliminate unnecessary
use of curl_multi_socket_all"), we have to remember all of the sockets
used by libcurl, but in fact we only did that for a single one. Since
one libcurl connection may use multiple sockets, however, we have to
remember them all.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 095ffda..4fbba5c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -104,12 +104,17 @@ typedef struct CURLAIOCB {
     size_t end;
 } CURLAIOCB;
 
+typedef struct CURLSocket {
+    int fd;
+    QLIST_ENTRY(CURLSocket) next;
+} CURLSocket;
+
 typedef struct CURLState
 {
     struct BDRVCURLState *s;
     CURLAIOCB *acb[CURL_NUM_ACB];
     CURL *curl;
-    curl_socket_t sock_fd;
+    QLIST_HEAD(, CURLSocket) sockets;
     char *orig_buf;
     size_t buf_start;
     size_t buf_off;
@@ -163,10 +168,27 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
 {
     BDRVCURLState *s;
     CURLState *state = NULL;
+    CURLSocket *socket;
+
     curl_easy_getinfo(curl, CURLINFO_PRIVATE, (char **)&state);
-    state->sock_fd = fd;
     s = state->s;
 
+    QLIST_FOREACH(socket, &state->sockets, next) {
+        if (socket->fd == fd) {
+            if (action == CURL_POLL_REMOVE) {
+                QLIST_REMOVE(socket, next);
+                g_free(socket);
+            }
+            break;
+        }
+    }
+    if (!socket) {
+        socket = g_new0(CURLSocket, 1);
+        socket->fd = fd;
+        QLIST_INSERT_HEAD(&state->sockets, socket, next);
+    }
+    socket = NULL;
+
     DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, (int)fd);
     switch (action) {
         case CURL_POLL_IN:
@@ -354,6 +376,7 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 static void curl_multi_do(void *arg)
 {
     CURLState *s = (CURLState *)arg;
+    CURLSocket *socket, *next_socket;
     int running;
     int r;
 
@@ -361,10 +384,13 @@ static void curl_multi_do(void *arg)
         return;
     }
 
-    do {
-        r = curl_multi_socket_action(s->s->multi, s->sock_fd, 0, &running);
-    } while(r == CURLM_CALL_MULTI_PERFORM);
-
+    /* Need to use _SAFE because curl_multi_socket_action() may trigger
+     * curl_sock_cb() which might modify this list */
+    QLIST_FOREACH_SAFE(socket, &s->sockets, next, next_socket) {
+        do {
+            r = curl_multi_socket_action(s->s->multi, socket->fd, 0, &running);
+        } while (r == CURLM_CALL_MULTI_PERFORM);
+    }
 }
 
 static void curl_multi_read(void *arg)
@@ -468,6 +494,7 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
 #endif
     }
 
+    QLIST_INIT(&state->sockets);
     state->s = s;
 
     return state;
@@ -477,6 +504,14 @@ static void curl_clean_state(CURLState *s)
 {
     if (s->s->multi)
         curl_multi_remove_handle(s->s->multi, s->curl);
+
+    while (!QLIST_EMPTY(&s->sockets)) {
+        CURLSocket *socket = QLIST_FIRST(&s->sockets);
+
+        QLIST_REMOVE(socket, next);
+        g_free(socket);
+    }
+
     s->in_use = 0;
 }
 
-- 
2.10.1

[Qemu-devel] [PATCH 4/4] block/curl: Do not wait for data beyond EOF

[Max Reitz]

libcurl will only give us as much data as there is, not more. The block
layer will deny requests beyond the end of file for us; but since this
block driver is still using a sector-based interface, we can still get
in trouble if the file size is not a multiple of 512.

While we have already made sure not to attempt transfers beyond the end
of the file, we are currently still trying to receive data from there if
the original request exceeds the file size. This patch fixes this issue
and invokes qemu_iovec_memset() on the iovec's tail.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 4fbba5c..2cb875a 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -253,8 +253,17 @@ 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;
+
             qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
                                 acb->end - acb->start);
+
+            if (acb->end - acb->start < request_length) {
+                size_t offset = acb->end - acb->start;
+                qemu_iovec_memset(acb->qiov, offset, 0,
+                                  request_length - offset);
+            }
+
             acb->common.cb(acb->common.opaque, 0);
             qemu_aio_unref(acb);
             s->acb[i] = NULL;
@@ -271,6 +280,8 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
 {
     int i;
     size_t end = start + len;
+    size_t clamped_end = MIN(end, s->len);
+    size_t clamped_len = clamped_end - start;
 
     for (i=0; i<CURL_NUM_STATES; i++) {
         CURLState *state = &s->states[i];
@@ -285,12 +296,15 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
         // Does the existing buffer cover our section?
         if ((start >= state->buf_start) &&
             (start <= buf_end) &&
-            (end >= state->buf_start) &&
-            (end <= buf_end))
+            (clamped_end >= state->buf_start) &&
+            (clamped_end <= buf_end))
         {
             char *buf = state->orig_buf + (start - state->buf_start);
 
-            qemu_iovec_from_buf(acb->qiov, 0, buf, len);
+            qemu_iovec_from_buf(acb->qiov, 0, buf, clamped_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;
@@ -300,13 +314,13 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
         if (state->in_use &&
             (start >= state->buf_start) &&
             (start <= buf_fend) &&
-            (end >= state->buf_start) &&
-            (end <= buf_fend))
+            (clamped_end >= state->buf_start) &&
+            (clamped_end <= buf_fend))
         {
             int j;
 
             acb->start = start - state->buf_start;
-            acb->end = acb->start + len;
+            acb->end = acb->start + clamped_len;
 
             for (j=0; j<CURL_NUM_ACB; j++) {
                 if (!state->acb[j]) {
@@ -799,13 +813,13 @@ static void curl_readv_bh_cb(void *p)
     }
 
     acb->start = 0;
-    acb->end = (acb->nb_sectors * BDRV_SECTOR_SIZE);
+    acb->end = MIN(acb->nb_sectors * BDRV_SECTOR_SIZE, s->len - start);
 
     state->buf_off = 0;
     g_free(state->orig_buf);
     state->buf_start = start;
-    state->buf_len = acb->end + s->readahead_size;
-    end = MIN(start + state->buf_len, s->len) - 1;
+    state->buf_len = MIN(acb->end + s->readahead_size, s->len - start);
+    end = start + state->buf_len - 1;
     state->orig_buf = g_try_malloc(state->buf_len);
     if (state->buf_len && state->orig_buf == NULL) {
         curl_clean_state(state);
-- 
2.10.1

Re: [Qemu-devel] [PATCH 1/4] block/curl: Use BDRV_SECTOR_SIZE

[Eric Blake]

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

On 10/24/2016 09:54 PM, Max Reitz wrote:
> Currently, curl defines its own constant SECTOR_SIZE. There is no
> advantage over using the global BDRV_SECTOR_SIZE, so drop it.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

(Can curl technically be used to access single-byte granularity?  I
suppose that would be a larger patch, though, to implement the right
callbacks).

> +    DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
> +            (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);

Eww. start is seriously limited to size_t rather than off_t?  Doesn't
that cripple us to a maximum of 4G files on 32-bit hosts?  But unrelated
to this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Eric Blake]

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

On 10/24/2016 09:54 PM, Max Reitz wrote:
> While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in that
> the callback is supposed to return the number of bytes handled; what it
> does not mention is that libcurl will throw an error if the callback did
> not "handle" all of the data passed to it.
> 
> Therefore, if the callback receives some data that it cannot handle
> (either because the receive buffer has not been set up yet or because it
> would not fit into the receive buffer) and we have to ignore it, we
> still have to report that the data has been handled.
> 
> Obviously, this should not happen normally. But it does happen at least
> for FTP connections where some data (that we do not expect) may be
> generated when the connection is established.

Just to make sure, we aren't losing data by reporting this value, but
merely letting curl know that our callback has "dealt" with the data, so
that we don't error out, in order to get a second chance at the same
data later on?

Reviewed-by: Eric Blake <eblake@redhat.com>

But given that it undoes 38bbc0a, I'd rather that it gets reviewed by
Matthew and/or tested by Richard.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Kevin Wolf]

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

Am 25.10.2016 um 20:37 hat Eric Blake geschrieben:
> On 10/24/2016 09:54 PM, Max Reitz wrote:
> > While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in that
> > the callback is supposed to return the number of bytes handled; what it
> > does not mention is that libcurl will throw an error if the callback did
> > not "handle" all of the data passed to it.
> > 
> > Therefore, if the callback receives some data that it cannot handle
> > (either because the receive buffer has not been set up yet or because it
> > would not fit into the receive buffer) and we have to ignore it, we
> > still have to report that the data has been handled.
> > 
> > Obviously, this should not happen normally. But it does happen at least
> > for FTP connections where some data (that we do not expect) may be
> > generated when the connection is established.
> 
> Just to make sure, we aren't losing data by reporting this value, but
> merely letting curl know that our callback has "dealt" with the data, so
> that we don't error out, in order to get a second chance at the same
> data later on?
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> But given that it undoes 38bbc0a, I'd rather that it gets reviewed by
> Matthew and/or tested by Richard.

In that case, I guess we should CC them. (Hereby done.)

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/4] block/curl: Use BDRV_SECTOR_SIZE

[Richard W.M. Jones]

On Tue, Oct 25, 2016 at 04:54:28AM +0200, Max Reitz wrote:
> Currently, curl defines its own constant SECTOR_SIZE. There is no
> advantage over using the global BDRV_SECTOR_SIZE, so drop it.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index e5eaa7b..12afa15 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -73,7 +73,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
>  
>  #define CURL_NUM_STATES 8
>  #define CURL_NUM_ACB    8
> -#define SECTOR_SIZE     512
>  #define READ_AHEAD_DEFAULT (256 * 1024)
>  #define CURL_TIMEOUT_DEFAULT 5
>  #define CURL_TIMEOUT_MAX 10000
> @@ -738,12 +737,12 @@ static void curl_readv_bh_cb(void *p)
>      CURLAIOCB *acb = p;
>      BDRVCURLState *s = acb->common.bs->opaque;
>  
> -    size_t start = acb->sector_num * SECTOR_SIZE;
> +    size_t start = acb->sector_num * BDRV_SECTOR_SIZE;
>      size_t end;
>  
>      // 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 * SECTOR_SIZE, acb)) {
> +    switch (curl_find_buf(s, start, acb->nb_sectors * BDRV_SECTOR_SIZE, acb)) {
>          case FIND_RET_OK:
>              qemu_aio_unref(acb);
>              // fall through
> @@ -762,7 +761,7 @@ static void curl_readv_bh_cb(void *p)
>      }
>  
>      acb->start = 0;
> -    acb->end = (acb->nb_sectors * SECTOR_SIZE);
> +    acb->end = (acb->nb_sectors * BDRV_SECTOR_SIZE);
>  
>      state->buf_off = 0;
>      g_free(state->orig_buf);
> @@ -779,8 +778,8 @@ static void curl_readv_bh_cb(void *p)
>      state->acb[0] = acb;
>  
>      snprintf(state->range, 127, "%zd-%zd", start, end);
> -    DPRINTF("CURL (AIO): Reading %d at %zd (%s)\n",
> -            (acb->nb_sectors * SECTOR_SIZE), start, state->range);
> +    DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
> +            (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);
>      curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
>  
>      curl_multi_add_handle(s->multi, state->curl);

ACK, this change is obvious.

Eric's comment about use of size_t instead of off_t or int64_t in
curl_readv_bh_cb (and maybe elsewhere) is dead right too.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Richard W.M. Jones]

On Tue, Oct 25, 2016 at 04:54:29AM +0200, Max Reitz wrote:
> While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in that
> the callback is supposed to return the number of bytes handled; what it
> does not mention is that libcurl will throw an error if the callback did
> not "handle" all of the data passed to it.
> 
> Therefore, if the callback receives some data that it cannot handle
> (either because the receive buffer has not been set up yet or because it
> would not fit into the receive buffer) and we have to ignore it, we
> still have to report that the data has been handled.
> 
> Obviously, this should not happen normally. But it does happen at least
> for FTP connections where some data (that we do not expect) may be
> generated when the connection is established.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 12afa15..095ffda 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -212,12 +212,13 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  
>      DPRINTF("CURL: Just reading %zd bytes\n", realsize);
>  
> -    if (!s || !s->orig_buf)
> -        return 0;
> +    if (!s || !s->orig_buf) {
> +        goto read_end;
> +    }
>  
>      if (s->buf_off >= s->buf_len) {
>          /* buffer full, read nothing */
> -        return 0;
> +        goto read_end;
>      }
>      realsize = MIN(realsize, s->buf_len - s->buf_off);
>      memcpy(s->orig_buf + s->buf_off, ptr, realsize);
> @@ -238,7 +239,9 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>          }
>      }
>  
> -    return realsize;
> +read_end:
> +    /* curl will error out if we do not return this value */
> +    return size * nmemb;
>  }

Luckily I documented my test case last time:
https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04883.html

I went back and tried a similar test case with just this patch on top
of current qemu from git, and it doesn't appear to break anything for
the http case.

So it's fine as far as I can see.

Rich.

$ http_proxy= LIBGUESTFS_BACKEND=direct LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 guestfish -a http://onuma.home.annexia.org/media/installers/Fedora-23-Cloud-x86_64/Fedora-Cloud-Base-23-20151030.x86_64.qcow2 --ro -i

Welcome to guestfish, the guest filesystem shell for
editing virtual machine filesystems and disk images.

Type: 'help' for help on commands
      'man' to read the manual
      'quit' to quit the shell

Operating system: Fedora 23 (Cloud Edition)
/dev/sda1 mounted on /

><fs> ll /
total 84
dr-xr-xr-x. 18 root root  4096 Oct 30  2015 .
drwxr-xr-x  19 root root  4096 Oct 26 09:36 ..
lrwxrwxrwx.  1 root root     7 Sep 10  2015 bin -> usr/bin
dr-xr-xr-x.  5 root root  4096 Oct 30  2015 boot
drwxr-xr-x.  2 root root  4096 Oct 30  2015 dev
drwxr-xr-x. 68 root root  4096 Oct 30  2015 etc
drwxr-xr-x.  2 root root  4096 Oct 30  2015 home
lrwxrwxrwx.  1 root root     7 Sep 10  2015 lib -> usr/lib
lrwxrwxrwx.  1 root root     9 Sep 10  2015 lib64 -> usr/lib64
drwx------.  2 root root 16384 Oct 30  2015 lost+found
drwxr-xr-x.  2 root root  4096 Sep 10  2015 media
drwxr-xr-x.  2 root root  4096 Sep 10  2015 mnt
drwxr-xr-x.  2 root root  4096 Sep 10  2015 opt
drwxr-xr-x.  2 root root  4096 Oct 30  2015 proc
dr-xr-x---.  2 root root  4096 Oct 30  2015 root
drwxr-xr-x.  2 root root  4096 Oct 30  2015 run
lrwxrwxrwx.  1 root root     8 Sep 10  2015 sbin -> usr/sbin
drwxr-xr-x.  2 root root  4096 Sep 10  2015 srv
drwxr-xr-x.  2 root root  4096 Oct 30  2015 sys
drwxrwxrwt.  7 root root  4096 Oct 30  2015 tmp
drwxr-xr-x. 12 root root  4096 Oct 30  2015 usr
drwxr-xr-x. 18 root root  4096 Oct 30  2015 var

><fs> find / | wc -l
26532
><fs> 


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH 0/4] block/curl: Fix FTP

[Richard W.M. Jones]

As well as testing patch 2 on its own, I also tested all 4 patches
together on top of current qemu from git.

In summary, it seems to work fine and doesn't break http or https as
far as I can tell.

Rich.

$ http_proxy= LIBGUESTFS_BACKEND=direct LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 guestfish -a http://onuma.home.annexia.org/media/installers/Fedora-23-Cloud-x86_64/Fedora-Cloud-Base-23-20151030.x86_64.qcow2 --ro -i

Welcome to guestfish, the guest filesystem shell for
editing virtual machine filesystems and disk images.

Type: 'help' for help on commands
      'man' to read the manual
      'quit' to quit the shell

Operating system: Fedora 23 (Cloud Edition)
/dev/sda1 mounted on /

><fs> ll /
total 84
dr-xr-xr-x. 18 root root  4096 Oct 30  2015 .
drwxr-xr-x  19 root root  4096 Oct 26 09:42 ..
lrwxrwxrwx.  1 root root     7 Sep 10  2015 bin -> usr/bin
dr-xr-xr-x.  5 root root  4096 Oct 30  2015 boot
drwxr-xr-x.  2 root root  4096 Oct 30  2015 dev
drwxr-xr-x. 68 root root  4096 Oct 30  2015 etc
drwxr-xr-x.  2 root root  4096 Oct 30  2015 home
lrwxrwxrwx.  1 root root     7 Sep 10  2015 lib -> usr/lib
lrwxrwxrwx.  1 root root     9 Sep 10  2015 lib64 -> usr/lib64
drwx------.  2 root root 16384 Oct 30  2015 lost+found
drwxr-xr-x.  2 root root  4096 Sep 10  2015 media
drwxr-xr-x.  2 root root  4096 Sep 10  2015 mnt
drwxr-xr-x.  2 root root  4096 Sep 10  2015 opt
drwxr-xr-x.  2 root root  4096 Oct 30  2015 proc
dr-xr-x---.  2 root root  4096 Oct 30  2015 root
drwxr-xr-x.  2 root root  4096 Oct 30  2015 run
lrwxrwxrwx.  1 root root     8 Sep 10  2015 sbin -> usr/sbin
drwxr-xr-x.  2 root root  4096 Sep 10  2015 srv
drwxr-xr-x.  2 root root  4096 Oct 30  2015 sys
drwxrwxrwt.  7 root root  4096 Oct 30  2015 tmp
drwxr-xr-x. 12 root root  4096 Oct 30  2015 usr
drwxr-xr-x. 18 root root  4096 Oct 30  2015 var

><fs> find / | wc -l
26532
><fs> exit

$ http_proxy= https_proxy= LIBGUESTFS_BACKEND=direct LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 guestfish -a https://download.fedoraproject.org/pub/fedora/linux/releases/24/CloudImages/x86_64/images/Fedora-Cloud-Base-24-1.2.x86_64.qcow2 --ro -i

Welcome to guestfish, the guest filesystem shell for
editing virtual machine filesystems and disk images.

Type: 'help' for help on commands
      'man' to read the manual
      'quit' to quit the shell

Operating system: Fedora 24 (Cloud Edition)
/dev/sda1 mounted on /

><fs> ll /
total 84
dr-xr-xr-x. 18 root root  4096 Jun 14 16:24 .
drwxr-xr-x  19 root root  4096 Oct 26 09:42 ..
lrwxrwxrwx.  1 root root     7 Feb  3  2016 bin -> usr/bin
dr-xr-xr-x.  4 root root  4096 Jun 14 16:24 boot
drwxr-xr-x.  2 root root  4096 Jun 14 16:20 dev
drwxr-xr-x. 67 root root  4096 Jun 14 16:25 etc
drwxr-xr-x.  2 root root  4096 Jun 14 16:24 home
lrwxrwxrwx.  1 root root     7 Feb  3  2016 lib -> usr/lib
lrwxrwxrwx.  1 root root     9 Feb  3  2016 lib64 -> usr/lib64
drwx------.  2 root root 16384 Jun 14 16:20 lost+found
drwxr-xr-x.  2 root root  4096 Feb  3  2016 media
drwxr-xr-x.  2 root root  4096 Feb  3  2016 mnt
drwxr-xr-x.  2 root root  4096 Feb  3  2016 opt
drwxr-xr-x.  2 root root  4096 Jun 14 16:20 proc
dr-xr-x---.  2 root root  4096 Jun 14 16:26 root
drwxr-xr-x.  2 root root  4096 Jun 14 16:20 run
lrwxrwxrwx.  1 root root     8 Feb  3  2016 sbin -> usr/sbin
drwxr-xr-x.  2 root root  4096 Feb  3  2016 srv
drwxr-xr-x.  2 root root  4096 Jun 14 16:20 sys
drwxrwxrwt.  7 root root  4096 Jun 14 16:26 tmp
drwxr-xr-x. 12 root root  4096 Jun 14 16:20 usr
drwxr-xr-x. 19 root root  4096 Jun 14 16:20 var

><fs> find / | wc -l
25817
><fs> 


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH 1/4] block/curl: Use BDRV_SECTOR_SIZE

[Max Reitz]

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

On 25.10.2016 20:31, Eric Blake wrote:
> On 10/24/2016 09:54 PM, Max Reitz wrote:
>> Currently, curl defines its own constant SECTOR_SIZE. There is no
>> advantage over using the global BDRV_SECTOR_SIZE, so drop it.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/curl.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> (Can curl technically be used to access single-byte granularity?  I
> suppose that would be a larger patch, though, to implement the right
> callbacks).

Yes, it definitely can, and yes, we definitely should implement it at
some point.

>> +    DPRINTF("CURL (AIO): Reading %llu at %zd (%s)\n",
>> +            (acb->nb_sectors * BDRV_SECTOR_SIZE), start, state->range);
> 
> Eww. start is seriously limited to size_t rather than off_t?  Doesn't
> that cripple us to a maximum of 4G files on 32-bit hosts?  But unrelated
> to this patch.

Well, yes, it's all kind of yuck...

Thanks for reviewing, though!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Max Reitz]

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

On 25.10.2016 20:37, Eric Blake wrote:
> On 10/24/2016 09:54 PM, Max Reitz wrote:
>> While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in that
>> the callback is supposed to return the number of bytes handled; what it
>> does not mention is that libcurl will throw an error if the callback did
>> not "handle" all of the data passed to it.
>>
>> Therefore, if the callback receives some data that it cannot handle
>> (either because the receive buffer has not been set up yet or because it
>> would not fit into the receive buffer) and we have to ignore it, we
>> still have to report that the data has been handled.
>>
>> Obviously, this should not happen normally. But it does happen at least
>> for FTP connections where some data (that we do not expect) may be
>> generated when the connection is established.
> 
> Just to make sure, we aren't losing data by reporting this value, but
> merely letting curl know that our callback has "dealt" with the data, so
> that we don't error out, in order to get a second chance at the same
> data later on?

I don't know about previous or future versions of libcurl, but I looked
at the current git code and it really just does two things with the
return code:

(1) Check whether it's a special code for pausing the connection, if so,
do what's necessary.

(2) If it is not, check whether it's different from the number of bytes
passed: If it is, report an error and abort the original transfer (which
in the case of establishing the connection means basically dropping the
connection).

Thus, curl will never save the data we didn't handle for later or
something like that. It's just an error if we don't report to have
handled everything.


This is also supported by the man page CURLOPT_WRITEFUNCTION(3):

> Your callback should return the number of bytes actually taken care
> of. If that amount differs from the amount passed to your callback
> function, it'll signal an error condition to the library. This will
> cause the transfer to get aborted and the libcurl function used will
> return CURLE_WRITE_ERROR.

Therefore, I don't think this is going to be changed in the future.

Max

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> But given that it undoes 38bbc0a, I'd rather that it gets reviewed by
> Matthew and/or tested by Richard.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Matthew Booth]

I've long since lost all context on this patch, so I'll trust you :)

Thanks for the ping,

Matt

On Wed, Oct 26, 2016 at 10:17 AM, Kevin Wolf <kwolf@redhat.com> wrote:

> Am 25.10.2016 um 20:37 hat Eric Blake geschrieben:
> > On 10/24/2016 09:54 PM, Max Reitz wrote:
> > > While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in
> that
> > > the callback is supposed to return the number of bytes handled; what it
> > > does not mention is that libcurl will throw an error if the callback
> did
> > > not "handle" all of the data passed to it.
> > >
> > > Therefore, if the callback receives some data that it cannot handle
> > > (either because the receive buffer has not been set up yet or because
> it
> > > would not fit into the receive buffer) and we have to ignore it, we
> > > still have to report that the data has been handled.
> > >
> > > Obviously, this should not happen normally. But it does happen at least
> > > for FTP connections where some data (that we do not expect) may be
> > > generated when the connection is established.
> >
> > Just to make sure, we aren't losing data by reporting this value, but
> > merely letting curl know that our callback has "dealt" with the data, so
> > that we don't error out, in order to get a second chance at the same
> > data later on?
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> > But given that it undoes 38bbc0a, I'd rather that it gets reviewed by
> > Matthew and/or tested by Richard.
>
> In that case, I guess we should CC them. (Hereby done.)
>
> Kevin
>



-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)

Re: [Qemu-devel] [PATCH 0/4] block/curl: Fix FTP

[Jeff Cody]

On Wed, Oct 26, 2016 at 10:44:02AM +0100, Richard W.M. Jones wrote:
> As well as testing patch 2 on its own, I also tested all 4 patches
> together on top of current qemu from git.
> 
> In summary, it seems to work fine and doesn't break http or https as
> far as I can tell.
> 
> Rich.
> 
> $ http_proxy= LIBGUESTFS_BACKEND=direct LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 guestfish -a http://onuma.home.annexia.org/media/installers/Fedora-23-Cloud-x86_64/Fedora-Cloud-Base-23-20151030.x86_64.qcow2 --ro -i
> 
> Welcome to guestfish, the guest filesystem shell for
> editing virtual machine filesystems and disk images.
> 
> Type: 'help' for help on commands
>       'man' to read the manual
>       'quit' to quit the shell
> 
> Operating system: Fedora 23 (Cloud Edition)
> /dev/sda1 mounted on /
> 
> ><fs> ll /
> total 84
> dr-xr-xr-x. 18 root root  4096 Oct 30  2015 .
> drwxr-xr-x  19 root root  4096 Oct 26 09:42 ..
> lrwxrwxrwx.  1 root root     7 Sep 10  2015 bin -> usr/bin
> dr-xr-xr-x.  5 root root  4096 Oct 30  2015 boot
> drwxr-xr-x.  2 root root  4096 Oct 30  2015 dev
> drwxr-xr-x. 68 root root  4096 Oct 30  2015 etc
> drwxr-xr-x.  2 root root  4096 Oct 30  2015 home
> lrwxrwxrwx.  1 root root     7 Sep 10  2015 lib -> usr/lib
> lrwxrwxrwx.  1 root root     9 Sep 10  2015 lib64 -> usr/lib64
> drwx------.  2 root root 16384 Oct 30  2015 lost+found
> drwxr-xr-x.  2 root root  4096 Sep 10  2015 media
> drwxr-xr-x.  2 root root  4096 Sep 10  2015 mnt
> drwxr-xr-x.  2 root root  4096 Sep 10  2015 opt
> drwxr-xr-x.  2 root root  4096 Oct 30  2015 proc
> dr-xr-x---.  2 root root  4096 Oct 30  2015 root
> drwxr-xr-x.  2 root root  4096 Oct 30  2015 run
> lrwxrwxrwx.  1 root root     8 Sep 10  2015 sbin -> usr/sbin
> drwxr-xr-x.  2 root root  4096 Sep 10  2015 srv
> drwxr-xr-x.  2 root root  4096 Oct 30  2015 sys
> drwxrwxrwt.  7 root root  4096 Oct 30  2015 tmp
> drwxr-xr-x. 12 root root  4096 Oct 30  2015 usr
> drwxr-xr-x. 18 root root  4096 Oct 30  2015 var
> 
> ><fs> find / | wc -l
> 26532
> ><fs> exit
> 
> $ http_proxy= https_proxy= LIBGUESTFS_BACKEND=direct LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 guestfish -a https://download.fedoraproject.org/pub/fedora/linux/releases/24/CloudImages/x86_64/images/Fedora-Cloud-Base-24-1.2.x86_64.qcow2 --ro -i
> 
> Welcome to guestfish, the guest filesystem shell for
> editing virtual machine filesystems and disk images.
> 
> Type: 'help' for help on commands
>       'man' to read the manual
>       'quit' to quit the shell
> 
> Operating system: Fedora 24 (Cloud Edition)
> /dev/sda1 mounted on /
> 
> ><fs> ll /
> total 84
> dr-xr-xr-x. 18 root root  4096 Jun 14 16:24 .
> drwxr-xr-x  19 root root  4096 Oct 26 09:42 ..
> lrwxrwxrwx.  1 root root     7 Feb  3  2016 bin -> usr/bin
> dr-xr-xr-x.  4 root root  4096 Jun 14 16:24 boot
> drwxr-xr-x.  2 root root  4096 Jun 14 16:20 dev
> drwxr-xr-x. 67 root root  4096 Jun 14 16:25 etc
> drwxr-xr-x.  2 root root  4096 Jun 14 16:24 home
> lrwxrwxrwx.  1 root root     7 Feb  3  2016 lib -> usr/lib
> lrwxrwxrwx.  1 root root     9 Feb  3  2016 lib64 -> usr/lib64
> drwx------.  2 root root 16384 Jun 14 16:20 lost+found
> drwxr-xr-x.  2 root root  4096 Feb  3  2016 media
> drwxr-xr-x.  2 root root  4096 Feb  3  2016 mnt
> drwxr-xr-x.  2 root root  4096 Feb  3  2016 opt
> drwxr-xr-x.  2 root root  4096 Jun 14 16:20 proc
> dr-xr-x---.  2 root root  4096 Jun 14 16:26 root
> drwxr-xr-x.  2 root root  4096 Jun 14 16:20 run
> lrwxrwxrwx.  1 root root     8 Feb  3  2016 sbin -> usr/sbin
> drwxr-xr-x.  2 root root  4096 Feb  3  2016 srv
> drwxr-xr-x.  2 root root  4096 Jun 14 16:20 sys
> drwxrwxrwt.  7 root root  4096 Jun 14 16:26 tmp
> drwxr-xr-x. 12 root root  4096 Jun 14 16:20 usr
> drwxr-xr-x. 19 root root  4096 Jun 14 16:20 var
> 
> ><fs> find / | wc -l
> 25817
> ><fs> 
> 
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org
> 

Thanks,

Applied to my block branch:

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

-Jeff