Re: [Qemu-block] [PATCH 5/6] curl: Handle success in multi_check_completion
On 09.09.19 22:30, John Snow wrote:
>
>
> On 8/27/19 12:34 PM, Max Reitz wrote:
>> Background: As of cURL 7.59.0, it verifies that several functions are
>> not called from within a callback. Among these functions is
>> curl_multi_add_handle().
>>
>> curl_read_cb() is a callback from cURL and not a coroutine. Waking up
>> acb->co will lead to entering it then and there, which means the current
>> request will settle and the caller (if it runs in the same coroutine)
>> may then issue the next request. In such a case, we will enter
>> curl_setup_preadv() effectively from within curl_read_cb().
>>
>> Calling curl_multi_add_handle() will then fail and the new request will
>> not be processed.
>>
>> Fix this by not letting curl_read_cb() wake up acb->co. Instead, leave
>> the whole business of settling the AIOCB objects to
>> curl_multi_check_completion() (which is called from our timer callback
>> and our FD read handler, so not from any cURL callbacks).
>>
>> Reported-by: Natalie Gavrielov
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
>> Cc: qemu-sta...@nongnu.org
>> Signed-off-by: Max Reitz
>> ---
>> block/curl.c | 69 ++--
>> 1 file changed, 29 insertions(+), 40 deletions(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index bc70f39fcb..5e0cca601d 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -231,7 +231,6 @@ static size_t curl_read_cb(void *ptr, size_t size,
>> size_t nmemb, void *opaque)
>> {
>> CURLState *s = ((CURLState*)opaque);
>> size_t realsize = size * nmemb;
>> -int i;
>>
>> trace_curl_read_cb(realsize);
>>
>> @@ -247,32 +246,6 @@ static size_t curl_read_cb(void *ptr, size_t size,
>> size_t nmemb, void *opaque)
>> memcpy(s->orig_buf + s->buf_off, ptr, realsize);
>> s->buf_off += realsize;
>>
>> -for(i=0; i> -CURLAIOCB *acb = s->acb[i];
>> -
>> -if (!acb)
>> -continue;
>> -
>> -if ((s->buf_off >= acb->end)) {
>
> This changes from a conditional,
It doesn’t really change. This code here is the read_cb. We may have
incomplete data still.
>> -size_t request_length = acb->bytes;
>> -
>> -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->ret = 0;
>> -s->acb[i] = NULL;
>> -qemu_mutex_unlock(>s->mutex);
>> -aio_co_wake(acb->co);
>> -qemu_mutex_lock(>s->mutex);
>> -}
>> -}
>> -
>> read_end:
>> /* curl will error out if we do not return this value */
>> return size * nmemb;
>> @@ -353,13 +326,14 @@ static void curl_multi_check_completion(BDRVCURLState
>> *s)
>> break;
>>
>> if (msg->msg == CURLMSG_DONE) {
>> +int i;
>> CURLState *state = NULL;
>> +bool error = msg->data.result != CURLE_OK;
>> +
>> curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE,
>>(char **));
>>
>> -/* ACBs for successful messages get completed in curl_read_cb */
>> -if (msg->data.result != CURLE_OK) {
>> -int i;
>> +if (error) {
>> static int errcount = 100;
>>
>> /* Don't lose the original error message from curl, since
>> @@ -371,20 +345,35 @@ static void curl_multi_check_completion(BDRVCURLState
>> *s)
>> error_report("curl: further errors suppressed");
>> }
>> }
>> +}
>>
>> -for (i = 0; i < CURL_NUM_ACB; i++) {
>> -CURLAIOCB *acb = state->acb[i];
>> +for (i = 0; i < CURL_NUM_ACB; i++) {
>> +CURLAIOCB *acb = state->acb[i];
>>
>> -if (acb == NULL) {
>> -continue;
>> -}
>> +if (acb == NULL) {
>> +continue;
>> +}
>> +
>> +if (!error) {
>> +/* Assert that we have read all data */
>> +assert(state->buf_off >= acb->end);
>
> To an assertion. This makes me feel better (What happens if that was
> ever false in the old code, we just drop the callback?) but is it
> definitely always gonna be true?
No, in the old code, the read_cb may have been called multiple times per
request. Of course, it would only really complete the last time, and
then we’d invoke the callback.
But here we just look at all requests that are done (CURLMSG_DONE from
curl_multi_info_read()). So we can now assert that we’ve actually read
all
Re: [Qemu-block] [PATCH 5/6] curl: Handle success in multi_check_completion
On 8/27/19 12:34 PM, Max Reitz wrote:
> Background: As of cURL 7.59.0, it verifies that several functions are
> not called from within a callback. Among these functions is
> curl_multi_add_handle().
>
> curl_read_cb() is a callback from cURL and not a coroutine. Waking up
> acb->co will lead to entering it then and there, which means the current
> request will settle and the caller (if it runs in the same coroutine)
> may then issue the next request. In such a case, we will enter
> curl_setup_preadv() effectively from within curl_read_cb().
>
> Calling curl_multi_add_handle() will then fail and the new request will
> not be processed.
>
> Fix this by not letting curl_read_cb() wake up acb->co. Instead, leave
> the whole business of settling the AIOCB objects to
> curl_multi_check_completion() (which is called from our timer callback
> and our FD read handler, so not from any cURL callbacks).
>
> Reported-by: Natalie Gavrielov
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
> ---
> block/curl.c | 69 ++--
> 1 file changed, 29 insertions(+), 40 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index bc70f39fcb..5e0cca601d 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -231,7 +231,6 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t
> nmemb, void *opaque)
> {
> CURLState *s = ((CURLState*)opaque);
> size_t realsize = size * nmemb;
> -int i;
>
> trace_curl_read_cb(realsize);
>
> @@ -247,32 +246,6 @@ static size_t curl_read_cb(void *ptr, size_t size,
> size_t nmemb, void *opaque)
> memcpy(s->orig_buf + s->buf_off, ptr, realsize);
> s->buf_off += realsize;
>
> -for(i=0; i -CURLAIOCB *acb = s->acb[i];
> -
> -if (!acb)
> -continue;
> -
> -if ((s->buf_off >= acb->end)) {
This changes from a conditional,
> -size_t request_length = acb->bytes;
> -
> -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->ret = 0;
> -s->acb[i] = NULL;
> -qemu_mutex_unlock(>s->mutex);
> -aio_co_wake(acb->co);
> -qemu_mutex_lock(>s->mutex);
> -}
> -}
> -
> read_end:
> /* curl will error out if we do not return this value */
> return size * nmemb;
> @@ -353,13 +326,14 @@ static void curl_multi_check_completion(BDRVCURLState
> *s)
> break;
>
> if (msg->msg == CURLMSG_DONE) {
> +int i;
> CURLState *state = NULL;
> +bool error = msg->data.result != CURLE_OK;
> +
> curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE,
>(char **));
>
> -/* ACBs for successful messages get completed in curl_read_cb */
> -if (msg->data.result != CURLE_OK) {
> -int i;
> +if (error) {
> static int errcount = 100;
>
> /* Don't lose the original error message from curl, since
> @@ -371,20 +345,35 @@ static void curl_multi_check_completion(BDRVCURLState
> *s)
> error_report("curl: further errors suppressed");
> }
> }
> +}
>
> -for (i = 0; i < CURL_NUM_ACB; i++) {
> -CURLAIOCB *acb = state->acb[i];
> +for (i = 0; i < CURL_NUM_ACB; i++) {
> +CURLAIOCB *acb = state->acb[i];
>
> -if (acb == NULL) {
> -continue;
> -}
> +if (acb == NULL) {
> +continue;
> +}
> +
> +if (!error) {
> +/* Assert that we have read all data */
> +assert(state->buf_off >= acb->end);
To an assertion. This makes me feel better (What happens if that was
ever false in the old code, we just drop the callback?) but is it
definitely always gonna be true?
Well, you are asserting it is, so I will believe you.
> +
> +qemu_iovec_from_buf(acb->qiov, 0,
> +state->orig_buf + acb->start,
> +acb->end - acb->start);
>
> -acb->ret = -EIO;
> -state->acb[i] = NULL;
> -qemu_mutex_unlock(>mutex);
> -aio_co_wake(acb->co);
> -qemu_mutex_lock(>mutex);
> +if (acb->end - acb->start < acb->bytes) {
> +
