[v3,7/7] block/curl: code cleanup to comply with coding style

This addresses non-functional changes to help curl.c better comply
with the coding styles (comments, indentation, brackets, etc.).

One minor code change is the combination of two if statements into
a single if statement.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/curl.c | 100 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 52 insertions(+), 48 deletions(-)

Hi Jeff,

While I'm relatively new to this community, I do have some comments
about the styling in this file.

I don't see anything in the CODING_STYLE file that tells me I'm
wrong here, but it's certainly possible...

More inline.

On Tue, Nov 07, 2017 at 05:27:24PM -0500, Jeff Cody wrote:
>This addresses non-functional changes to help curl.c better comply
>with the coding styles (comments, indentation, brackets, etc.).
>
>One minor code change is the combination of two if statements into
>a single if statement.
>
>Signed-off-by: Jeff Cody <jcody@redhat.com>
>Reviewed-by: Eric Blake <eblake@redhat.com>
>---
> block/curl.c | 100 +++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 52 insertions(+), 48 deletions(-)
>
>diff --git a/block/curl.c b/block/curl.c
>index 35cf417..7a6dd44 100644
>--- a/block/curl.c
>+++ b/block/curl.c
>@@ -32,8 +32,10 @@
> #include <curl/curl.h>
> #include "qemu/cutils.h"
>
>-// #define DEBUG_CURL
>-// #define DEBUG_VERBOSE
>+/*
>+ #define DEBUG_CURL
>+ #define DEBUG_VERBOSE
>+*/

Is it not more common to use the style:

    /*
     * text
     */

This is visible in almost every file at the top, where the Copyright
and License is.

>
> #ifdef DEBUG_CURL
> #define DEBUG_CURL_PRINT 1
>@@ -76,15 +78,15 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
> #define CURL_TIMEOUT_DEFAULT 5
> #define CURL_TIMEOUT_MAX 10000
>
>-#define CURL_BLOCK_OPT_URL       "url"
>-#define CURL_BLOCK_OPT_READAHEAD "readahead"
>-#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
>-#define CURL_BLOCK_OPT_TIMEOUT "timeout"
>-#define CURL_BLOCK_OPT_COOKIE    "cookie"
>-#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
>-#define CURL_BLOCK_OPT_USERNAME "username"
>-#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
>-#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
>+#define CURL_BLOCK_OPT_URL                   "url"
>+#define CURL_BLOCK_OPT_READAHEAD             "readahead"
>+#define CURL_BLOCK_OPT_SSLVERIFY             "sslverify"
>+#define CURL_BLOCK_OPT_TIMEOUT               "timeout"
>+#define CURL_BLOCK_OPT_COOKIE                "cookie"
>+#define CURL_BLOCK_OPT_COOKIE_SECRET         "cookie-secret"
>+#define CURL_BLOCK_OPT_USERNAME              "username"
>+#define CURL_BLOCK_OPT_PASSWORD_SECRET       "password-secret"
>+#define CURL_BLOCK_OPT_PROXY_USERNAME        "proxy-username"
> #define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
>
> struct BDRVCURLState;
>@@ -110,8 +112,7 @@ typedef struct CURLSocket {
>     QLIST_ENTRY(CURLSocket) next;
> } CURLSocket;
>
>-typedef struct CURLState
>-{
>+typedef struct CURLState {
>     struct BDRVCURLState *s;
>     CURLAIOCB *acb[CURL_NUM_ACB];
>     CURL *curl;
>@@ -196,22 +197,22 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>
>     DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, (int)fd);
>     switch (action) {
>-        case CURL_POLL_IN:
>-            aio_set_fd_handler(s->aio_context, fd, false,
>-                               curl_multi_read, NULL, NULL, state);
>-            break;
>-        case CURL_POLL_OUT:
>-            aio_set_fd_handler(s->aio_context, fd, false,
>-                               NULL, curl_multi_do, NULL, state);
>-            break;
>-        case CURL_POLL_INOUT:
>-            aio_set_fd_handler(s->aio_context, fd, false,
>-                               curl_multi_read, curl_multi_do, NULL, state);
>-            break;
>-        case CURL_POLL_REMOVE:
>-            aio_set_fd_handler(s->aio_context, fd, false,
>-                               NULL, NULL, NULL, NULL);
>-            break;
>+    case CURL_POLL_IN:
>+        aio_set_fd_handler(s->aio_context, fd, false,
>+                           curl_multi_read, NULL, NULL, state);
>+        break;
>+    case CURL_POLL_OUT:
>+        aio_set_fd_handler(s->aio_context, fd, false,
>+                           NULL, curl_multi_do, NULL, state);
>+        break;
>+    case CURL_POLL_INOUT:
>+        aio_set_fd_handler(s->aio_context, fd, false,
>+                           curl_multi_read, curl_multi_do, NULL, state);
>+        break;
>+    case CURL_POLL_REMOVE:
>+        aio_set_fd_handler(s->aio_context, fd, false,
>+                           NULL, NULL, NULL, NULL);
>+        break;
>     }
>
>     return 0;
>@@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> /* Called from curl_multi_do_locked, with s->mutex held.  */
> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
> {
>-    CURLState *s = ((CURLState*)opaque);
>+    CURLState *s = opaque;

Not sure about this one - while it may not be strictly necessary to
do the casting, from a readability point of view it is helpful to
see the cast - possibly with the outer brackets removed:

      CURLState *s = (CURLState*)opaque;

>     size_t realsize = size * nmemb;
>     int i;
>
>@@ -253,11 +254,12 @@ 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<CURL_NUM_ACB; i++) {
>+    for (i = 0; i < CURL_NUM_ACB; i++) {
>         CURLAIOCB *acb = s->acb[i];
>
>-        if (!acb)
>+        if (!acb) {
>             continue;
>+        }
>
>         if ((s->buf_off >= acb->end)) {
>             size_t request_length = acb->bytes;
>@@ -293,17 +295,16 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
>     uint64_t clamped_end = MIN(end, s->len);
>     uint64_t clamped_len = clamped_end - start;
>
>-    for (i=0; i<CURL_NUM_STATES; i++) {
>+    for (i = 0; i < CURL_NUM_STATES; i++) {
>         CURLState *state = &s->states[i];
>         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;
>-        if (!state->buf_off)
>+        if (!state->orig_buf || !state->buf_off) {
>             continue;
>+        }
>
>-        // Does the existing buffer cover our section?
>+        /* Does the existing buffer cover our section? */
>         if ((start >= state->buf_start) &&
>             (start <= buf_end) &&
>             (clamped_end >= state->buf_start) &&
>@@ -319,7 +320,7 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
>             return true;
>         }
>
>-        // Wait for unfinished chunks
>+        /* Wait for unfinished chunks */
>         if (state->in_use &&
>             (start >= state->buf_start) &&
>             (start <= buf_fend) &&
>@@ -331,7 +332,7 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
>             acb->start = start - state->buf_start;
>             acb->end = acb->start + clamped_len;
>
>-            for (j=0; j<CURL_NUM_ACB; j++) {
>+            for (j = 0; j < CURL_NUM_ACB; j++) {
>                 if (!state->acb[j]) {
>                     state->acb[j] = acb;
>                     return true;
>@@ -355,8 +356,9 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>         msg = curl_multi_info_read(s->multi, &msgs_in_queue);
>
>         /* Quit when there are no more completions */
>-        if (!msg)
>+        if (!msg) {
>             break;
>+        }
>
>         if (msg->msg == CURLMSG_DONE) {
>             CURLState *state = NULL;
>@@ -540,12 +542,14 @@ static void curl_clean_state(CURLState *s)
> {
>     CURLAIOCB *next;
>     int j;
>+
>     for (j = 0; j < CURL_NUM_ACB; j++) {
>         assert(!s->acb[j]);
>     }
>
>-    if (s->s->multi)
>+    if (s->s->multi) {
>         curl_multi_remove_handle(s->s->multi, s->curl);
>+    }
>
>     while (!QLIST_EMPTY(&s->sockets)) {
>         CURLSocket *socket = QLIST_FIRST(&s->sockets);
>@@ -794,7 +798,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>         goto out_noclean;
>     }
>
>-    // Get file size
>+    /* Get file size */
>
>     if (curl_init_state(s, state) < 0) {
>         goto out;
>@@ -802,11 +806,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>
>     s->accept_range = false;
>     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
>-    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
>-                     curl_header_cb);
>+    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb);
>     curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
>-    if (curl_easy_perform(state->curl))
>+    if (curl_easy_perform(state->curl)) {
>         goto out;
>+    }
>     if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
>         goto out;
>     }
>@@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
>
>     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.
>+    /* In case we have the requested data already (e.g. read-ahead),
>+       we can just call the callback and be done. */

Please don't do a multi-line comment like this, it is much more
obvious that the second line is a comment if you use the more normal
format of as outlined earlier by me.

Thanks,

Darren.

On Tue, Nov 07, 2017 at 05:27:24PM -0500, Jeff Cody wrote:
> This addresses non-functional changes to help curl.c better comply
> with the coding styles (comments, indentation, brackets, etc.).
> 
> One minor code change is the combination of two if statements into
> a single if statement.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

As you say, just simple coding style fixes except for the single
combined if statement.  Therefore:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

>  block/curl.c | 100 +++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 52 insertions(+), 48 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 35cf417..7a6dd44 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -32,8 +32,10 @@
>  #include <curl/curl.h>
>  #include "qemu/cutils.h"
>  
> -// #define DEBUG_CURL
> -// #define DEBUG_VERBOSE
> +/*
> + #define DEBUG_CURL
> + #define DEBUG_VERBOSE
> +*/
>  
>  #ifdef DEBUG_CURL
>  #define DEBUG_CURL_PRINT 1
> @@ -76,15 +78,15 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
>  #define CURL_TIMEOUT_DEFAULT 5
>  #define CURL_TIMEOUT_MAX 10000
>  
> -#define CURL_BLOCK_OPT_URL       "url"
> -#define CURL_BLOCK_OPT_READAHEAD "readahead"
> -#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
> -#define CURL_BLOCK_OPT_TIMEOUT "timeout"
> -#define CURL_BLOCK_OPT_COOKIE    "cookie"
> -#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
> -#define CURL_BLOCK_OPT_USERNAME "username"
> -#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
> -#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
> +#define CURL_BLOCK_OPT_URL                   "url"
> +#define CURL_BLOCK_OPT_READAHEAD             "readahead"
> +#define CURL_BLOCK_OPT_SSLVERIFY             "sslverify"
> +#define CURL_BLOCK_OPT_TIMEOUT               "timeout"
> +#define CURL_BLOCK_OPT_COOKIE                "cookie"
> +#define CURL_BLOCK_OPT_COOKIE_SECRET         "cookie-secret"
> +#define CURL_BLOCK_OPT_USERNAME              "username"
> +#define CURL_BLOCK_OPT_PASSWORD_SECRET       "password-secret"
> +#define CURL_BLOCK_OPT_PROXY_USERNAME        "proxy-username"
>  #define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
>  
>  struct BDRVCURLState;
> @@ -110,8 +112,7 @@ typedef struct CURLSocket {
>      QLIST_ENTRY(CURLSocket) next;
>  } CURLSocket;
>  
> -typedef struct CURLState
> -{
> +typedef struct CURLState {
>      struct BDRVCURLState *s;
>      CURLAIOCB *acb[CURL_NUM_ACB];
>      CURL *curl;
> @@ -196,22 +197,22 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
>  
>      DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, (int)fd);
>      switch (action) {
> -        case CURL_POLL_IN:
> -            aio_set_fd_handler(s->aio_context, fd, false,
> -                               curl_multi_read, NULL, NULL, state);
> -            break;
> -        case CURL_POLL_OUT:
> -            aio_set_fd_handler(s->aio_context, fd, false,
> -                               NULL, curl_multi_do, NULL, state);
> -            break;
> -        case CURL_POLL_INOUT:
> -            aio_set_fd_handler(s->aio_context, fd, false,
> -                               curl_multi_read, curl_multi_do, NULL, state);
> -            break;
> -        case CURL_POLL_REMOVE:
> -            aio_set_fd_handler(s->aio_context, fd, false,
> -                               NULL, NULL, NULL, NULL);
> -            break;
> +    case CURL_POLL_IN:
> +        aio_set_fd_handler(s->aio_context, fd, false,
> +                           curl_multi_read, NULL, NULL, state);
> +        break;
> +    case CURL_POLL_OUT:
> +        aio_set_fd_handler(s->aio_context, fd, false,
> +                           NULL, curl_multi_do, NULL, state);
> +        break;
> +    case CURL_POLL_INOUT:
> +        aio_set_fd_handler(s->aio_context, fd, false,
> +                           curl_multi_read, curl_multi_do, NULL, state);
> +        break;
> +    case CURL_POLL_REMOVE:
> +        aio_set_fd_handler(s->aio_context, fd, false,
> +                           NULL, NULL, NULL, NULL);
> +        break;
>      }
>  
>      return 0;
> @@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  /* Called from curl_multi_do_locked, with s->mutex held.  */
>  static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>  {
> -    CURLState *s = ((CURLState*)opaque);
> +    CURLState *s = opaque;
>      size_t realsize = size * nmemb;
>      int i;
>  
> @@ -253,11 +254,12 @@ 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<CURL_NUM_ACB; i++) {
> +    for (i = 0; i < CURL_NUM_ACB; i++) {
>          CURLAIOCB *acb = s->acb[i];
>  
> -        if (!acb)
> +        if (!acb) {
>              continue;
> +        }
>  
>          if ((s->buf_off >= acb->end)) {
>              size_t request_length = acb->bytes;
> @@ -293,17 +295,16 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
>      uint64_t clamped_end = MIN(end, s->len);
>      uint64_t clamped_len = clamped_end - start;
>  
> -    for (i=0; i<CURL_NUM_STATES; i++) {
> +    for (i = 0; i < CURL_NUM_STATES; i++) {
>          CURLState *state = &s->states[i];
>          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;
> -        if (!state->buf_off)
> +        if (!state->orig_buf || !state->buf_off) {
>              continue;
> +        }
>  
> -        // Does the existing buffer cover our section?
> +        /* Does the existing buffer cover our section? */
>          if ((start >= state->buf_start) &&
>              (start <= buf_end) &&
>              (clamped_end >= state->buf_start) &&
> @@ -319,7 +320,7 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
>              return true;
>          }
>  
> -        // Wait for unfinished chunks
> +        /* Wait for unfinished chunks */
>          if (state->in_use &&
>              (start >= state->buf_start) &&
>              (start <= buf_fend) &&
> @@ -331,7 +332,7 @@ static bool curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len,
>              acb->start = start - state->buf_start;
>              acb->end = acb->start + clamped_len;
>  
> -            for (j=0; j<CURL_NUM_ACB; j++) {
> +            for (j = 0; j < CURL_NUM_ACB; j++) {
>                  if (!state->acb[j]) {
>                      state->acb[j] = acb;
>                      return true;
> @@ -355,8 +356,9 @@ static void curl_multi_check_completion(BDRVCURLState *s)
>          msg = curl_multi_info_read(s->multi, &msgs_in_queue);
>  
>          /* Quit when there are no more completions */
> -        if (!msg)
> +        if (!msg) {
>              break;
> +        }
>  
>          if (msg->msg == CURLMSG_DONE) {
>              CURLState *state = NULL;
> @@ -540,12 +542,14 @@ static void curl_clean_state(CURLState *s)
>  {
>      CURLAIOCB *next;
>      int j;
> +
>      for (j = 0; j < CURL_NUM_ACB; j++) {
>          assert(!s->acb[j]);
>      }
>  
> -    if (s->s->multi)
> +    if (s->s->multi) {
>          curl_multi_remove_handle(s->s->multi, s->curl);
> +    }
>  
>      while (!QLIST_EMPTY(&s->sockets)) {
>          CURLSocket *socket = QLIST_FIRST(&s->sockets);
> @@ -794,7 +798,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>          goto out_noclean;
>      }
>  
> -    // Get file size
> +    /* Get file size */
>  
>      if (curl_init_state(s, state) < 0) {
>          goto out;
> @@ -802,11 +806,11 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->accept_range = false;
>      curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
> -    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
> -                     curl_header_cb);
> +    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb);
>      curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
> -    if (curl_easy_perform(state->curl))
> +    if (curl_easy_perform(state->curl)) {
>          goto out;
> +    }
>      if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
>          goto out;
>      }
> @@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
>  
>      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.
> +    /* In case we have the requested data already (e.g. read-ahead),
> +       we can just call the callback and be done. */
>      if (curl_find_buf(s, start, acb->bytes, acb)) {
>          goto out;
>      }
>  
> -    // No cache found, so let's start a new request
> +    /* No cache found, so let's start a new request */
>      for (;;) {
>          state = curl_find_state(s);
>          if (state) {
> -- 
> 2.9.5

On 11/08/2017 04:47 AM, Darren Kenny wrote:
> Hi Jeff,
> 
> While I'm relatively new to this community, I do have some comments
> about the styling in this file.
> 
> I don't see anything in the CODING_STYLE file that tells me I'm
> wrong here, but it's certainly possible...
> 
> More inline.
> 

>> -// #define DEBUG_CURL
>> -// #define DEBUG_VERBOSE

This is definitely against style (checkpatch.pl flags it),

>> +/*
>> + #define DEBUG_CURL
>> + #define DEBUG_VERBOSE
>> +*/
> 

while you are correct that this is not quite usual style.

> Is it not more common to use the style:
> 
>    /*
>     * text
>     */

But, since checkpatch.pl doesn't flag it, and since it is easier to
remove the leading and trailing /* and */ to enable the debug #defines
(compared to editing every single line of the comment), I don't see a
problem with the style chosen here.


>> @@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t
>> size, size_t nmemb, void *opaque)
>> /* Called from curl_multi_do_locked, with s->mutex held.  */
>> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void
>> *opaque)
>> {
>> -    CURLState *s = ((CURLState*)opaque);
>> +    CURLState *s = opaque;
> 
> Not sure about this one - while it may not be strictly necessary to
> do the casting, from a readability point of view it is helpful to
> see the cast - possibly with the outer brackets removed:
> 
>      CURLState *s = (CURLState*)opaque;

I _am_ sure about it. The cast from void* is pointless (this is C, not
C++), and this is one of the changes that Jeff specifically made in v3
that was not present in v2, because I requested that we lose the cast
(in general, we try to avoid as many casts as possible).

>> @@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState
>> *bs, CURLAIOCB *acb)
>>
>>     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.
>> +    /* In case we have the requested data already (e.g. read-ahead),
>> +       we can just call the callback and be done. */
> 
> Please don't do a multi-line comment like this, it is much more
> obvious that the second line is a comment if you use the more normal
> format of as outlined earlier by me.

Indeed, while I don't mind the style used on the #define DEBUG_*, this
one could be touched up to be more idiomatic.

On Wed, Nov 08, 2017 at 08:26:57AM -0600, Eric Blake wrote:
>On 11/08/2017 04:47 AM, Darren Kenny wrote:
>> Hi Jeff,
>>
>> While I'm relatively new to this community, I do have some comments
>> about the styling in this file.
>>
>> I don't see anything in the CODING_STYLE file that tells me I'm
>> wrong here, but it's certainly possible...
>>
>> More inline.
>>
>
>>> -// #define DEBUG_CURL
>>> -// #define DEBUG_VERBOSE
>
>This is definitely against style (checkpatch.pl flags it),
>
>>> +/*
>>> + #define DEBUG_CURL
>>> + #define DEBUG_VERBOSE
>>> +*/
>>
>
>while you are correct that this is not quite usual style.
>
>> Is it not more common to use the style:
>>
>>    /*
>>     * text
>>     */
>
>But, since checkpatch.pl doesn't flag it, and since it is easier to
>remove the leading and trailing /* and */ to enable the debug #defines
>(compared to editing every single line of the comment), I don't see a
>problem with the style chosen here.
>

If that is the purpose, maybe an #if 0 is more appropriate or #ifdef
DEBUG, or similar.

Isn't the purpose of styling to be consistent? As such should we not
be trying to use the multi-line style set out at the top of the file?

>
>>> @@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t
>>> size, size_t nmemb, void *opaque)
>>> /* Called from curl_multi_do_locked, with s->mutex held.  */
>>> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void
>>> *opaque)
>>> {
>>> -    CURLState *s = ((CURLState*)opaque);
>>> +    CURLState *s = opaque;
>>
>> Not sure about this one - while it may not be strictly necessary to
>> do the casting, from a readability point of view it is helpful to
>> see the cast - possibly with the outer brackets removed:
>>
>>      CURLState *s = (CURLState*)opaque;
>
>I _am_ sure about it. The cast from void* is pointless (this is C, not
>C++), and this is one of the changes that Jeff specifically made in v3
>that was not present in v2, because I requested that we lose the cast
>(in general, we try to avoid as many casts as possible).

Fair enough.

>
>>> @@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState
>>> *bs, CURLAIOCB *acb)
>>>
>>>     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.
>>> +    /* In case we have the requested data already (e.g. read-ahead),
>>> +       we can just call the callback and be done. */
>>
>> Please don't do a multi-line comment like this, it is much more
>> obvious that the second line is a comment if you use the more normal
>> format of as outlined earlier by me.
>
>Indeed, while I don't mind the style used on the #define DEBUG_*, this
>one could be touched up to be more idiomatic.
>

Thanks,

Darren.

On 08/11/2017 11:47, Darren Kenny wrote:
>>
>>
>> -// #define DEBUG_CURL
>> -// #define DEBUG_VERBOSE
>> +/*
>> + #define DEBUG_CURL
>> + #define DEBUG_VERBOSE
>> +*/
> 
> Is it not more common to use the style:
> 
>    /*
>     * text
>     */
> 
> This is visible in almost every file at the top, where the Copyright
> and License is.

The most common style in QEMU is probably

   /* text
    * more text
    */

But for DEBUG_* macros I think // are appropriate.  checkpatch should
still warn about them because DEBUG_* macros should be replaced by
tracepoints; but unless we do that we should keep line comments.

>>
>> -    CURLState *s = ((CURLState*)opaque);
>> +    CURLState *s = opaque;
> 
> Not sure about this one - while it may not be strictly necessary to
> do the casting, from a readability point of view it is helpful to
> see the cast - possibly with the outer brackets removed:
> 
>      CURLState *s = (CURLState*)opaque;

I think the most common idiom here is to omit the cast.

>> -    // In case we have the requested data already (e.g. read-ahead),
>> -    // we can just call the callback and be done.
>> +    /* In case we have the requested data already (e.g. read-ahead),
>> +       we can just call the callback and be done. */
> 
> Please don't do a multi-line comment like this, it is much more
> obvious that the second line is a comment if you use the more normal
> format of as outlined earlier by me. 

Agreed.

Paolo

On 08/11/2017 15:50, Darren Kenny wrote:
>> But, since checkpatch.pl doesn't flag it, and since it is easier to
>> remove the leading and trailing /* and */ to enable the debug #defines
>> (compared to editing every single line of the comment), I don't see a
>> problem with the style chosen here.
> 
> If that is the purpose, maybe an #if 0 is more appropriate or #ifdef
> DEBUG, or similar.
> 
> Isn't the purpose of styling to be consistent? As such should we not
> be trying to use the multi-line style set out at the top of the file?

Yes, and you're very welcome to submit a checkpatch.pl patch that warns
about that comment style without * at the beginning of each line.

On the other hand, style should not get in the way, and the version that
gets least in the way for debug #defines is //.

Paolo