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.

The selection of floppy size was a bit unusual:
 - fat-type undefined: a FAT 12 2880 Kib disk (default)
 - fat-type=16: a FAT 16 2880 Kib disk
 - fat-type=12: a FAT 12 1440 Kib disk

Now, that fat-type undefined means fat-type=12, it's no longer possible
to make that size distinction. Therefore, it's being changed for the
following:
 - fat-type=12: a FAT 12 1440 Kib disk (default)
 - fat-type=16: a FAT 16 2880 Kib dis

This has been choosen for two reasons: keep fat-type=12 the default and
creates a more usual size for it: 1440 Kib.

The possibility to create a FAT 12 2880 Kib floppy will be added back
later, through the fat-size parameter.

Side note to mention that s->sectors_per_cluster assignments are
removed because they are overidden a few line further.

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

diff --git a/block/vvfat.c b/block/vvfat.c
index de6031db98..d8c8d44f16 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1192,45 +1192,45 @@ 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;
-            secs = 36;
-            s->sectors_per_cluster = 2;
         } else {
-            secs = s->fat_type == 12 ? 18 : 36;
-            s->sectors_per_cluster = 1;
+            s->fat_type = 16;
         }
+        break;
+    default:
+        error_setg(errp, "Valid FAT types are only 12, 16 and 32");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+
+    if (floppy) {
+        /* Choose floppy size. 1440 KiB for FAT 12, 2880 KiB for FAT-16 */
+        secs = s->fat_type == 12 ? 18 : 36;
         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, "partitioned", true)) {
             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.43.0

Clément Chigot <chigot@adacore.com> writes:

> 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.
>
> The selection of floppy size was a bit unusual:
>  - fat-type undefined: a FAT 12 2880 Kib disk (default)
>  - fat-type=16: a FAT 16 2880 Kib disk
>  - fat-type=12: a FAT 12 1440 Kib disk
>
> Now, that fat-type undefined means fat-type=12, it's no longer possible
> to make that size distinction. Therefore, it's being changed for the
> following:
>  - fat-type=12: a FAT 12 1440 Kib disk (default)
>  - fat-type=16: a FAT 16 2880 Kib dis
>
> This has been choosen for two reasons: keep fat-type=12 the default and
> creates a more usual size for it: 1440 Kib.
>
> The possibility to create a FAT 12 2880 Kib floppy will be added back
> later, through the fat-size parameter.
>
> Side note to mention that s->sectors_per_cluster assignments are
> removed because they are overidden a few line further.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Is this a user-visible change?

On Mon, Nov 10, 2025 at 11:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Clément Chigot <chigot@adacore.com> writes:
>
> > 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.
> >
> > The selection of floppy size was a bit unusual:
> >  - fat-type undefined: a FAT 12 2880 Kib disk (default)
> >  - fat-type=16: a FAT 16 2880 Kib disk
> >  - fat-type=12: a FAT 12 1440 Kib disk
> >
> > Now, that fat-type undefined means fat-type=12, it's no longer possible
> > to make that size distinction. Therefore, it's being changed for the
> > following:
> >  - fat-type=12: a FAT 12 1440 Kib disk (default)
> >  - fat-type=16: a FAT 16 2880 Kib dis
> >
> > This has been choosen for two reasons: keep fat-type=12 the default and
> > creates a more usual size for it: 1440 Kib.
> >
> > The possibility to create a FAT 12 2880 Kib floppy will be added back
> > later, through the fat-size parameter.
> >
> > Side note to mention that s->sectors_per_cluster assignments are
> > removed because they are overidden a few line further.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> Is this a user-visible change?

Yes, just "floppy" will now result in a 1440 KiB instead of the
previous 2880 KiB. However, Kevin mentions in V1 that it would make
more sense and vvfat being known to be unstable, this would be fine.
FTR, here is the complete comment:

> On Wed, Oct 29, 2025 at 5:06 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > 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.

Clément Chigot <chigot@adacore.com> writes:

> On Mon, Nov 10, 2025 at 11:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Clément Chigot <chigot@adacore.com> writes:
>>
>> > 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.
>> >
>> > The selection of floppy size was a bit unusual:
>> >  - fat-type undefined: a FAT 12 2880 Kib disk (default)
>> >  - fat-type=16: a FAT 16 2880 Kib disk
>> >  - fat-type=12: a FAT 12 1440 Kib disk
>> >
>> > Now, that fat-type undefined means fat-type=12, it's no longer possible
>> > to make that size distinction. Therefore, it's being changed for the
>> > following:
>> >  - fat-type=12: a FAT 12 1440 Kib disk (default)
>> >  - fat-type=16: a FAT 16 2880 Kib dis
>> >
>> > This has been choosen for two reasons: keep fat-type=12 the default and
>> > creates a more usual size for it: 1440 Kib.
>> >
>> > The possibility to create a FAT 12 2880 Kib floppy will be added back
>> > later, through the fat-size parameter.
>> >
>> > Side note to mention that s->sectors_per_cluster assignments are
>> > removed because they are overidden a few line further.
>> >
>> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>>
>> Is this a user-visible change?
>
> Yes, just "floppy" will now result in a 1440 KiB instead of the
> previous 2880 KiB. However, Kevin mentions in V1 that it would make
> more sense and vvfat being known to be unstable, this would be fine.
> FTR, here is the complete comment:
>
>> On Wed, Oct 29, 2025 at 5:06 PM Kevin Wolf <kwolf@redhat.com> wrote:
>> > 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.

Hmm, where is this stance on defaults documented?  Question for Kevin,
of course.

>> >                                   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.

Awkward.

>> > 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.

I'm fine to treat vvfat as unstable.  But it's not marked as such in the
QAPI schema!  Is that a bug?  Again, for Kevin.

Am 10.11.2025 um 14:13 hat Markus Armbruster geschrieben:
> Clément Chigot <chigot@adacore.com> writes:
> 
> > On Mon, Nov 10, 2025 at 11:09 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Clément Chigot <chigot@adacore.com> writes:
> >>
> >> > 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.
> >> >
> >> > The selection of floppy size was a bit unusual:
> >> >  - fat-type undefined: a FAT 12 2880 Kib disk (default)
> >> >  - fat-type=16: a FAT 16 2880 Kib disk
> >> >  - fat-type=12: a FAT 12 1440 Kib disk
> >> >
> >> > Now, that fat-type undefined means fat-type=12, it's no longer possible
> >> > to make that size distinction. Therefore, it's being changed for the
> >> > following:
> >> >  - fat-type=12: a FAT 12 1440 Kib disk (default)
> >> >  - fat-type=16: a FAT 16 2880 Kib dis
> >> >
> >> > This has been choosen for two reasons: keep fat-type=12 the default and
> >> > creates a more usual size for it: 1440 Kib.
> >> >
> >> > The possibility to create a FAT 12 2880 Kib floppy will be added back
> >> > later, through the fat-size parameter.
> >> >
> >> > Side note to mention that s->sectors_per_cluster assignments are
> >> > removed because they are overidden a few line further.
> >> >
> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> >>
> >> Is this a user-visible change?
> >
> > Yes, just "floppy" will now result in a 1440 KiB instead of the
> > previous 2880 KiB. However, Kevin mentions in V1 that it would make
> > more sense and vvfat being known to be unstable, this would be fine.
> > FTR, here is the complete comment:
> >
> >> On Wed, Oct 29, 2025 at 5:06 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >> > 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.
> 
> Hmm, where is this stance on defaults documented?  Question for Kevin,
> of course.

Probably nowhere. More importantly, I don't think a compatibility
promise that says otherwise is documented either. And we know that
defaults have changed before, and that libvirt tries to be as explicit
as possible to avoid being impacted by changed defaults.

Do you disagree? If so, is there any way to change defaults or do we
have to stick to the existing defaults forever? To me not specifying an
option means "just pick anything that makes sense", without any promise
that this stays the same across versions.

> >> >                                   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.
> 
> Awkward.
> 
> >> > 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.
> 
> I'm fine to treat vvfat as unstable.  But it's not marked as such in the
> QAPI schema!  Is that a bug?  Again, for Kevin.

Maybe? Though the kind of unstable I think of with vvfat is more than
just API instability that the QAPI feature is about. vvfat is more a
dirty (and clever) hack that sometimes works and can be useful enough,
but if it breaks, you get to keep both pieces. Good for one-off uses on
your personal toy VM, but keep it far away from production. We never
seriously tried to get it to a properly supportable level.

(And yes, probably none of this is documented as clearly as it should
be.)

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.11.2025 um 14:13 hat Markus Armbruster geschrieben:
>> Clément Chigot <chigot@adacore.com> writes:
>> 
>> > On Mon, Nov 10, 2025 at 11:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Clément Chigot <chigot@adacore.com> writes:
>> >>
>> >> > 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.
>> >> >
>> >> > The selection of floppy size was a bit unusual:
>> >> >  - fat-type undefined: a FAT 12 2880 Kib disk (default)
>> >> >  - fat-type=16: a FAT 16 2880 Kib disk
>> >> >  - fat-type=12: a FAT 12 1440 Kib disk
>> >> >
>> >> > Now, that fat-type undefined means fat-type=12, it's no longer possible
>> >> > to make that size distinction. Therefore, it's being changed for the
>> >> > following:
>> >> >  - fat-type=12: a FAT 12 1440 Kib disk (default)
>> >> >  - fat-type=16: a FAT 16 2880 Kib dis
>> >> >
>> >> > This has been choosen for two reasons: keep fat-type=12 the default and
>> >> > creates a more usual size for it: 1440 Kib.
>> >> >
>> >> > The possibility to create a FAT 12 2880 Kib floppy will be added back
>> >> > later, through the fat-size parameter.
>> >> >
>> >> > Side note to mention that s->sectors_per_cluster assignments are
>> >> > removed because they are overidden a few line further.
>> >> >
>> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>> >>
>> >> Is this a user-visible change?
>> >
>> > Yes, just "floppy" will now result in a 1440 KiB instead of the
>> > previous 2880 KiB. However, Kevin mentions in V1 that it would make
>> > more sense and vvfat being known to be unstable, this would be fine.
>> > FTR, here is the complete comment:
>> >
>> >> On Wed, Oct 29, 2025 at 5:06 PM Kevin Wolf <kwolf@redhat.com> wrote:
>> >> > 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.
>> 
>> Hmm, where is this stance on defaults documented?  Question for Kevin,
>> of course.
>
> Probably nowhere. More importantly, I don't think a compatibility
> promise that says otherwise is documented either. And we know that
> defaults have changed before, and that libvirt tries to be as explicit
> as possible to avoid being impacted by changed defaults.
>
> Do you disagree? If so, is there any way to change defaults or do we
> have to stick to the existing defaults forever? To me not specifying an
> option means "just pick anything that makes sense", without any promise
> that this stays the same across versions.

I'd love to be able to change defaults.  Defaults can become bad over
time.  I remember arguing for changing such defaults unsuccessfully.

Looks like there's differing opinions / uncertainty on whether our
compatibility promise covers defaults.  That's bad, we need clarity
there.  I'll start a separate thread.

[...]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.11.2025 um 14:13 hat Markus Armbruster geschrieben:
>> Clément Chigot <chigot@adacore.com> writes:
>> 
>> > On Mon, Nov 10, 2025 at 11:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Clément Chigot <chigot@adacore.com> writes:

[...]

>> >> > 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.
>> 
>> I'm fine to treat vvfat as unstable.  But it's not marked as such in the
>> QAPI schema!  Is that a bug?  Again, for Kevin.
>
> Maybe? Though the kind of unstable I think of with vvfat is more than
> just API instability that the QAPI feature is about. vvfat is more a
> dirty (and clever) hack that sometimes works and can be useful enough,
> but if it breaks, you get to keep both pieces. Good for one-off uses on
> your personal toy VM, but keep it far away from production. We never
> seriously tried to get it to a properly supportable level.
>
> (And yes, probably none of this is documented as clearly as it should
> be.)

Do we need to differentiate between "unstable interface, may change
incompatibly or be withdrawn in future releases, stay away if you don't
want your software to break when this happens" and "known-wobbly
feature, do not use in production"?

Related ot Daniel's work on marking insecure objects, I think:

    Subject: [PATCH v2 00/32] Encode object type security status in code
    Date: Fri, 26 Sep 2025 15:01:11 +0100
    Message-ID: <20250926140144.1998694-1-berrange@redhat.com>