Message ID | 20170510143205.32013-6-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On 10.05.2017 16:32, Paolo Bonzini wrote: > 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. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/curl.c | 44 ++++++++++++++++++++++---------------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index 4b4d5a2389..3e288f2bc7 100644 > --- a/block/curl.c > +++ b/block/curl.c [...] > @@ -788,7 +788,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, > } > #endif > > - s->len = (size_t)d; > + s->len = d; On a very personal level, I'd rather have an explicit cast when converting floating point values to integers. Anyway: Reviewed-by: Max Reitz <mreitz@redhat.com> On a very surprised level: Why does curl return the content length as a double?! I can't think of a reason where that might be a good idea. If your integer no longer fits into a uin64_t, the double will be inexact, so it pretty much is useless, too... > > if ((!strncasecmp(s->url, "http://", strlen("http://")) > || !strncasecmp(s->url, "https://", strlen("https://")))
On 05/10/2017 12:36 PM, Max Reitz wrote: > On a very surprised level: Why does curl return the content length as a > double?! I can't think of a reason where that might be a good idea. If > your integer no longer fits into a uin64_t, the double will be inexact, > so it pretty much is useless, too... Not to mention: NO ONE has off_t larger than 63 bits (there physically is not that much storage around!), so a signed 64-bit integer should always be sufficient, rather than artificially limiting exact answers to 53 bits of precision via a double.
On Wed, May 10, 2017 at 04:32:03PM +0200, Paolo Bonzini wrote: > 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. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/curl.c | 44 ++++++++++++++++++++++---------------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index 4b4d5a2389..3e288f2bc7 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -96,8 +96,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; > @@ -115,7 +115,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]; > @@ -126,7 +126,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; > @@ -257,7 +257,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); > @@ -282,18 +282,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; > @@ -788,7 +788,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://"))) > @@ -797,7 +797,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); > @@ -836,14 +836,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; > @@ -871,7 +871,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); > @@ -886,9 +886,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); > @@ -913,8 +913,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.12.2 > > Reviewed-by: Jeff Cody <jcody@redhat.com>
diff --git a/block/curl.c b/block/curl.c index 4b4d5a2389..3e288f2bc7 100644 --- a/block/curl.c +++ b/block/curl.c @@ -96,8 +96,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; @@ -115,7 +115,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]; @@ -126,7 +126,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; @@ -257,7 +257,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); @@ -282,18 +282,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; @@ -788,7 +788,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://"))) @@ -797,7 +797,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); @@ -836,14 +836,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; @@ -871,7 +871,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); @@ -886,9 +886,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); @@ -913,8 +913,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;
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. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/curl.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)