diff mbox

[08/15] curl: Use bdrv_open options instead of filename

Message ID 1365799688-19918-9-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf April 12, 2013, 8:48 p.m. UTC 
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/curl.c | 153 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 102 insertions(+), 51 deletions(-)

Comments

Eric Blake April 15, 2013, 7:57 p.m. UTC | #1 
On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/curl.c | 153 +++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 102 insertions(+), 51 deletions(-)
> 

> @@ -369,29 +364,78 @@ static int curl_open(BlockDriverState *bs, const char *filename,
>                      ra_val = ra + 1;
>                      ra -= strlen(RA_OPTSTR) - 1;
>                      *ra = '\0';
> -                    s->readahead_size = atoi(ra_val);

Good riddance - atoi() is evil because it can't detect overflow.

> -                    break;
> -                } else {
> -                    break;
> +                    qdict_put(options, "readahead", qstring_from_str(ra_val));

So now we just pass the string on to the option parser,...

> +static QemuOptsList runtime_opts = {
> +    .name = "curl",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "url",
> +            .type = QEMU_OPT_STRING,
> +            .help = "URL to open",
> +        },
> +        {
> +            .name = "readahead",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Readahead size",

...and this new option lets QEMU_OPT_SIZE detect crazy strings that were
previously treated as '0' by the old atoi().  You should probably
mention this bug fix in the commit message as being intentional.

> @@ -568,63 +614,68 @@ static int64_t curl_getlength(BlockDriverState *bs)
>  }
>  
>  static BlockDriver bdrv_http = {
> -    .format_name     = "http",
> -    .protocol_name   = "http",
> +    .format_name            = "http",
> +    .protocol_name          = "http",
>  
> -    .instance_size   = sizeof(BDRVCURLState),
> -    .bdrv_file_open  = curl_open,
> -    .bdrv_close      = curl_close,
> -    .bdrv_getlength  = curl_getlength,
> +    .instance_size          = sizeof(BDRVCURLState),
> +    .bdrv_parse_filename    = curl_parse_filename,
> +    .bdrv_file_open         = curl_open,
> +    .bdrv_close             = curl_close,
> +    .bdrv_getlength         = curl_getlength,
>  
> -    .bdrv_aio_readv  = curl_aio_readv,
> +    .bdrv_aio_readv         = curl_aio_readv,

On this patch, you reindented ALL callbacks to the same width.  But in
6/15 and 7/15, you reindented only the callbacks in the
.bdrv_parse_filename section.  You should probably be consistent between
patches (I actually prefer indenting the entire BlockDriver
initialization consistently, as done in this patch, but didn't ding
patch 6 or 7 at the time because they looked innocuous enough in isolation).

Since I only called out whitespace issues (and even then, only for
commits other than this) and a weakness in the commit message, the code
itself deserves:

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/block/curl.c b/block/curl.c
index 186e3b0..61bc3db 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -335,12 +335,9 @@  static void curl_clean_state(CURLState *s)
     s->in_use = 0;
 }
 
-static int curl_open(BlockDriverState *bs, const char *filename,
-                     QDict *options, int flags)
+static void curl_parse_filename(const char *filename, QDict *options,
+                                Error **errp)
 {
-    BDRVCURLState *s = bs->opaque;
-    CURLState *state = NULL;
-    double d;
 
     #define RA_OPTSTR ":readahead="
     char *file;
@@ -348,19 +345,17 @@  static int curl_open(BlockDriverState *bs, const char *filename,
     const char *ra_val;
     int parse_state = 0;
 
-    static int inited = 0;
-
     file = g_strdup(filename);
-    s->readahead_size = READ_AHEAD_SIZE;
 
     /* Parse a trailing ":readahead=#:" param, if present. */
     ra = file + strlen(file) - 1;
     while (ra >= file) {
         if (parse_state == 0) {
-            if (*ra == ':')
+            if (*ra == ':') {
                 parse_state++;
-            else
+            } else {
                 break;
+            }
         } else if (parse_state == 1) {
             if (*ra > '9' || *ra < '0') {
                 char *opt_start = ra - strlen(RA_OPTSTR) + 1;
@@ -369,29 +364,78 @@  static int curl_open(BlockDriverState *bs, const char *filename,
                     ra_val = ra + 1;
                     ra -= strlen(RA_OPTSTR) - 1;
                     *ra = '\0';
-                    s->readahead_size = atoi(ra_val);
-                    break;
-                } else {
-                    break;
+                    qdict_put(options, "readahead", qstring_from_str(ra_val));
                 }
+                break;
             }
         }
         ra--;
     }
 
+    qdict_put(options, "url", qstring_from_str(file));
+
+    g_free(file);
+}
+
+static QemuOptsList runtime_opts = {
+    .name = "curl",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "url",
+            .type = QEMU_OPT_STRING,
+            .help = "URL to open",
+        },
+        {
+            .name = "readahead",
+            .type = QEMU_OPT_SIZE,
+            .help = "Readahead size",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int curl_open(BlockDriverState *bs, const char *dummy,
+                     QDict *options, int flags)
+{
+    BDRVCURLState *s = bs->opaque;
+    CURLState *state = NULL;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *file;
+    double d;
+
+    static int inited = 0;
+
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        goto out_noclean;
+    }
+
+    s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE);
     if ((s->readahead_size & 0x1ff) != 0) {
         fprintf(stderr, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
                 s->readahead_size);
         goto out_noclean;
     }
 
+    file = qemu_opt_get(opts, "url");
+    if (file == NULL) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "curl block driver requires "
+                      "an 'url' option");
+        goto out_noclean;
+    }
+
     if (!inited) {
         curl_global_init(CURL_GLOBAL_ALL);
         inited = 1;
     }
 
     DPRINTF("CURL: Opening %s\n", file);
-    s->url = file;
+    s->url = g_strdup(file);
     state = curl_init_state(s);
     if (!state)
         goto out_noclean;
@@ -423,6 +467,7 @@  static int curl_open(BlockDriverState *bs, const char *filename,
     curl_multi_setopt( s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb ); 
     curl_multi_do(s);
 
+    qemu_opts_del(opts);
     return 0;
 
 out:
@@ -430,7 +475,8 @@  out:
     curl_easy_cleanup(state->curl);
     state->curl = NULL;
 out_noclean:
-    g_free(file);
+    g_free(s->url);
+    qemu_opts_del(opts);
     return -EINVAL;
 }
 
@@ -568,63 +614,68 @@  static int64_t curl_getlength(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_http = {
-    .format_name     = "http",
-    .protocol_name   = "http",
+    .format_name            = "http",
+    .protocol_name          = "http",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_https = {
-    .format_name     = "https",
-    .protocol_name   = "https",
+    .format_name            = "https",
+    .protocol_name          = "https",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_ftp = {
-    .format_name     = "ftp",
-    .protocol_name   = "ftp",
+    .format_name            = "ftp",
+    .protocol_name          = "ftp",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_ftps = {
-    .format_name     = "ftps",
-    .protocol_name   = "ftps",
+    .format_name            = "ftps",
+    .protocol_name          = "ftps",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_tftp = {
-    .format_name     = "tftp",
-    .protocol_name   = "tftp",
+    .format_name            = "tftp",
+    .protocol_name          = "tftp",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static void curl_block_init(void)