Message ID | 20161025025431.24714-2-mreitz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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.
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.
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
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);
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(-)