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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vvfat.c | 210 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 158 insertions(+), 52 deletions(-)
> 

> +    if (!strstart(filename, "fat:", NULL)) {
> +        error_setg(errp, "File name string must start with 'fat:'");
> +        return;
> +    }
> +
> +    /* Parse options */
> +    if (strstr(filename, ":32:")) {
> +        fat_type = 32;
> +    } else if (strstr(filename, ":16:")) {
> +        fat_type = 16;
> +    } else if (strstr(filename, ":12:")) {
> +        fat_type = 12;
> +    }
> +
> +    if (strstr(filename, ":floppy:")) {
> +        floppy = true;
> +    }
> +
> +    if (strstr(filename, ":rw:")) {
> +        rw = true;
> +    }
> +
> +    /* Get the directory name without options */
> +    i = strrchr(filename, ':') - filename;

This parser is rather loose, in that it ignores unknown options (for
example, if I did fat:1:file, it would happily ignore option "1").  But
that's no worse than the old code.

> +    switch (s->fat_type) {
> +    case 32:
> +	    fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. "
> +                "You are welcome to do so!\n");

Should we s/greek/Greek/ in the message, or otherwise change its
contents?  But you just moved the pre-existing choice of spelling, so it
doesn't reflect on you.

> @@ -2868,8 +2973,9 @@ static void vvfat_close(BlockDriverState *bs)
>  static BlockDriver bdrv_vvfat = {
>      .format_name	= "vvfat",
>      .instance_size	= sizeof(BDRVVVFATState),
> -    .bdrv_file_open	= vvfat_open,
> -    .bdrv_rebind	= vvfat_rebind,
> +    .bdrv_parse_filename    = vvfat_parse_filename,
> +    .bdrv_file_open         = vvfat_open,
> +    .bdrv_rebind            = vvfat_rebind,
>      .bdrv_read          = vvfat_co_read,
>      .bdrv_write         = vvfat_co_write,
>      .bdrv_close		= vvfat_close,

Back to my comments on 9/15 on indenting the callbacks consistently.

No major errors, so I'm fine if you use:

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]