1. Patchew
  2. QEMU
  3. [PATCH 0/5] block/vvfat: introduce "size" option
  4. View patch

[PATCH 2/5] vvfat: move fat_type check prior to size setup

Clément Chigot posted 5 patches 5 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
[PATCH 2/5] vvfat: move fat_type check prior to size setup
Posted by Clément Chigot 5 months ago 
This allows to handle the default FAT size in a single place and make the
following part taking care only about size parameters. It will be later
moved away in a specific function.

Setting a floppy disk of 1MB is no longer possible as it was a side
effect of passing "fat-type=12". To be precise there were three cases:
 - fat-type undefined (aka default): a fat12 2MB disk
 - fat-type=16: a fat16 2Mb disk
 - fat-type=12: a fat12 1Mb disk

Now, that fat-type undefined means fat-type=2, it's no longer possible
to make that size distinction. It will be added back a bit later,
through the size parameter.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 0220dd828b..91e4ad3158 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1190,45 +1190,51 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         memcpy(s->volume_label, "QEMU VVFAT", 10);
     }
 
-    if (floppy) {
-        /* 1.44MB or 2.88MB floppy.  2.88MB can be FAT12 (default) or FAT16. */
-        if (!s->fat_type) {
+    /* Verify FAT type  */
+    switch (s->fat_type) {
+    case 32:
+        warn_report("FAT32 has not been tested. You are welcome to do so!");
+        break;
+    case 16:
+    case 12:
+        break;
+    case 0:
+        /* Set a default type */
+        if (floppy) {
             s->fat_type = 12;
+        } else {
+            s->fat_type = 16;
+        }
+        break;
+    default:
+        error_setg(errp, "Valid FAT types are only 12, 16 and 32");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+
+    if (floppy) {
+        /* 2.88MB floppy */
+        if (s->fat_type == 12) {
             secs = 36;
             s->sectors_per_cluster = 2;
         } else {
-            secs = s->fat_type == 12 ? 18 : 36;
+            secs = 36;
             s->sectors_per_cluster = 1;
         }
         cyls = 80;
         heads = 2;
     } else {
-        /* 32MB or 504MB disk*/
-        if (!s->fat_type) {
-            s->fat_type = 16;
-        }
         /* Reserver space for MBR */
         if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
             s->offset_to_bootsector = 0x3f;
         }
+        /* 32MB or 504MB disk*/
         cyls = s->fat_type == 12 ? 64 : 1024;
         heads = 16;
         secs = 63;
     }
 
-    switch (s->fat_type) {
-    case 32:
-        warn_report("FAT32 has not been tested. You are welcome to do so!");
-        break;
-    case 16:
-    case 12:
-        break;
-    default:
-        error_setg(errp, "Valid FAT types are only 12, 16 and 32");
-        ret = -EINVAL;
-        goto fail;
-    }
-
 
     s->bs = bs;
 
-- 
2.34.1
Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup
Posted by Kevin Wolf 3 months, 2 weeks ago 
Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> This allows to handle the default FAT size in a single place and make the
> following part taking care only about size parameters. It will be later
> moved away in a specific function.
> 
> Setting a floppy disk of 1MB is no longer possible as it was a side
> effect of passing "fat-type=12". To be precise there were three cases:
>  - fat-type undefined (aka default): a fat12 2MB disk
>  - fat-type=16: a fat16 2Mb disk
>  - fat-type=12: a fat12 1Mb disk

That's quite a strange interface!

If we're touching it anyway, I would change it to make the more common
format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
temporarily unavailable and later require an explicit size. This way
both sizes would still be available using the fat-type.

Please say 1.44 MB and 2.88 MB in the commit message rather than 1MB (or
even 1Mb, which might mean megabit). There were other sizes like 1.2 MB
that are closer to 1 MB, so it's better to avoid that confusion.

> Now, that fat-type undefined means fat-type=2, it's no longer possible

s/2/12/

> to make that size distinction. It will be added back a bit later,
> through the size parameter.
> 
> Signed-off-by: Clément Chigot <chigot@adacore.com>

The code looks good, apart from the change I suggested above (making
1.44 MB the default for FAT12).

Kevin
Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup
Posted by Clément Chigot 3 months, 1 week ago 
On Thu, Oct 23, 2025 at 8:40 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > This allows to handle the default FAT size in a single place and make the
> > following part taking care only about size parameters. It will be later
> > moved away in a specific function.
> >
> > Setting a floppy disk of 1MB is no longer possible as it was a side
> > effect of passing "fat-type=12". To be precise there were three cases:
> >  - fat-type undefined (aka default): a fat12 2MB disk
> >  - fat-type=16: a fat16 2Mb disk
> >  - fat-type=12: a fat12 1Mb disk
>
> That's quite a strange interface!
>
> If we're touching it anyway, I would change it to make the more common
> format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
> temporarily unavailable and later require an explicit size. This way
> both sizes would still be available using the fat-type.

I'm a bit hesitant to change the default behavior as people might be
using it without clear knowledge of it. True, "floppy" is probably not
a widely used feature but still.
Do QEMU have some specific guidelines when changing such default
behavior ? Adding a warning ? Or just a comment in the changelog would
be enough ?

> Please say 1.44 MB and 2.88 MB in the commit message rather than 1MB (or
> even 1Mb, which might mean megabit). There were other sizes like 1.2 MB
> that are closer to 1 MB, so it's better to avoid that confusion.
>
> > Now, that fat-type undefined means fat-type=2, it's no longer possible
>
> s/2/12/
>
> > to make that size distinction. It will be added back a bit later,
> > through the size parameter.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> The code looks good, apart from the change I suggested above (making
> 1.44 MB the default for FAT12).
>
> Kevin
>
Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup
Posted by BALATON Zoltan 3 months, 1 week ago 
On Wed, 29 Oct 2025, Clément Chigot wrote:
> On Thu, Oct 23, 2025 at 8:40 PM Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
>>> This allows to handle the default FAT size in a single place and make the
>>> following part taking care only about size parameters. It will be later
>>> moved away in a specific function.
>>>
>>> Setting a floppy disk of 1MB is no longer possible as it was a side
>>> effect of passing "fat-type=12". To be precise there were three cases:
>>>  - fat-type undefined (aka default): a fat12 2MB disk
>>>  - fat-type=16: a fat16 2Mb disk
>>>  - fat-type=12: a fat12 1Mb disk
>>
>> That's quite a strange interface!
>>
>> If we're touching it anyway, I would change it to make the more common
>> format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
>> temporarily unavailable and later require an explicit size. This way
>> both sizes would still be available using the fat-type.
>
> I'm a bit hesitant to change the default behavior as people might be
> using it without clear knowledge of it. True, "floppy" is probably not
> a widely used feature but still.
> Do QEMU have some specific guidelines when changing such default
> behavior ? Adding a warning ? Or just a comment in the changelog would
> be enough ?

https://www.qemu.org/docs/master/about/deprecated.html

Usually you'd add a warning and list it in the docs as deprecated then can 
make the change after it was deperecated for two releases.

Regards,
BALATON Zoltan
Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup
Posted by Kevin Wolf 3 months, 1 week ago 
Am 29.10.2025 um 14:58 hat BALATON Zoltan geschrieben:
> On Wed, 29 Oct 2025, Clément Chigot wrote:
> > On Thu, Oct 23, 2025 at 8:40 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > > 
> > > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > > > This allows to handle the default FAT size in a single place and make the
> > > > following part taking care only about size parameters. It will be later
> > > > moved away in a specific function.
> > > > 
> > > > Setting a floppy disk of 1MB is no longer possible as it was a side
> > > > effect of passing "fat-type=12". To be precise there were three cases:
> > > >  - fat-type undefined (aka default): a fat12 2MB disk
> > > >  - fat-type=16: a fat16 2Mb disk
> > > >  - fat-type=12: a fat12 1Mb disk
> > > 
> > > That's quite a strange interface!
> > > 
> > > If we're touching it anyway, I would change it to make the more common
> > > format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
> > > temporarily unavailable and later require an explicit size. This way
> > > both sizes would still be available using the fat-type.
> > 
> > I'm a bit hesitant to change the default behavior as people might be
> > using it without clear knowledge of it. True, "floppy" is probably not
> > a widely used feature but still.
> > Do QEMU have some specific guidelines when changing such default
> > behavior ? Adding a warning ? Or just a comment in the changelog would
> > be enough ?
> 
> https://www.qemu.org/docs/master/about/deprecated.html
> 
> Usually you'd add a warning and list it in the docs as deprecated then can
> make the change after it was deperecated for two releases.

We're not removing any functionality, just changing the default. So I
don't think the deprecation period applies.

In general, our stance is that we can change defaults whenever we want
to, and if you don't want to be surprised by changing defaults, you need
to specify the option explicitly. What's a bit strange about the vvfat
interface is that the default actually represents a configuration that
can't even be expressed explicitly at the moment.

So it is a special case in a way, but given that this is vvfat, which is
known to be unstable, not widely used outside of the occasional manual
use and not supported by libvirt, I'm willing to just make the change.

Kevin

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.