[1/4] block/curl: Use BDRV_SECTOR_SIZE
diff mbox

Message ID 20161025025431.24714-2-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Oct. 25, 2016, 2:54 a.m. UTC
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(-)

Comments

Eric Blake Oct. 25, 2016, 6:31 p.m. UTC | #1
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.
Richard W.M. Jones Oct. 26, 2016, 9:31 a.m. UTC | #2
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.
Max Reitz Oct. 26, 2016, 2:39 p.m. UTC | #3
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

Patch
diff mbox

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);