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 <jc...@redhat.com>
Reviewed-by: Eric Blake <ebl...@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.
