Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Kevin Wolf <kwolf@redhat.com>]

Am 29.06.2012 17:34, schrieb Markus Armbruster:
> vvfat creates a virtual VFAT filesystem with a certain logical
> geometry that depends on its options.  It sets the "geometry hint" to
> this geometry.  It is the only block driver to do this.
> 
> The geometry hint is about about *physical* geometry, and used only by
> certain hard disk device models.
> 
> vvfat's hint is normally invisible for device models, because
> bdrv_open() puts a raw format on top of vvfat's fat protocol.  That
> raw format is where drive_init() puts the user's geometry (if any),
> and where the device model gets it from.
> 
> Nobody complained, because the default physical geometry is the same
> as vvfat's logical geometry:
> 
>     opts        LCHS        def. PCHS
>                 1024,16,63  same
>     :32:        1024,16,63  same
>     :16:        1024,16,63  same
>     :12:          64,16,63  same
> 
> Except when you specify :floppy:
> 
>     opts        LCHS        def. PCHS
>        :floppy:   80, 2,36  5,16,63
>     :32:floppy:   80, 2,36  5,16,63
>     :16:floppy:   80, 2,36  5,16,63
>     :12:floppy:   80, 2,18  2,16,63
> 
> Silly thing to do for use with a hard disk.
> 
> However, the "raw" format can be suppressed by adding an
> redundant-looking "format=vvfat" to "file=fat:FOO".  Then, vvfat's
> hint clobbers the user's geometry, i.e. -drive options cyls, heads,
> secs get silently ignored.  Don't do that.
> 
> No change without format=vvfat.  With it, the user's hard disk
> geometry (-drive options cyls, heads, secs) is now obeyed, and the
> default hard disk geometry with :floppy: now matches the one without
> format=vvfat.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

For some values of "obeyed". If I understand correctly, the user defined
geometry will indeed by visible for the device emulation now, but this
still doesn't mean that vvfat also provides an image that matches this
geometry. Not sure if it is a good idea to allow such mismatches.

> @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) {
>      else
>  	dirname += i+1;
>  
> -    bs->total_sectors=bs->cyls*bs->heads*bs->secs;
> +    bs->total_sectors = cyls * heads * secs;
>  
> -    if(init_directories(s, dirname))
> +    if (init_directories(s, dirname, heads, secs)) {
>  	return -1;
> +    }
>  
>      s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>  
>      if(s->first_sectors_number==0x40)
> -	init_mbr(s);
> -    else {
> -        /* MS-DOS does not like to know about CHS (?). */
> -	bs->heads = bs->cyls = bs->secs = 0;
> -    }
> +        init_mbr(s, cyls, heads, secs);

You can add braces here while touching the code.

Kevin