| Message ID | 41ed8543e005205118438328c7e162532729d9c7.1510093478.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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) {