Re: [Qemu-devel] [PATCH 08/15] curl: Use bdrv_open options instead of filename

[Eric Blake]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  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 <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature