[PATCH v2 0/5] block/vvfat: introduce "fat-size" option

[PATCH v2 0/5] block/vvfat: introduce "fat-size" option

[Clément Chigot]

The main goal of this series is to introduce a new option "fat-size"
within the vvfat backend (patch 5).

This series also includes minor patches:
 - patch 1 introduces another option to create unpartitionned disks.
 - patch 2-4 are minor improvements easing the introducing of
   "fat-size" option

This was tested on with a aarch64-linux kernel taken from
functional/aarch64/test-virt and on aarch64-qnx over raspi4b with a
workaround, not included here (the SD bus must be associated to the EMMC2
port instead of through GPIOs).

Changes since v1:
 - patch 1:
   - rename option "partitioned"
   - add qapi entries
   - add option in vvfat_strong_runtime_opts
 - patch 2:
   - make FAT12 1440K the default instead of FAT12 2880K
 - patch 3:
   - introduce rename VVFAT_SECTOR_SIZE
   - replace BRDV_SECTOR_SIZE as those are actually VVFAT_SECTOR_SIZE
   - introduce VVFAT_SECTOR_BITS
 - patch 5
   - rename option "fat-size"
   - add qapi entries
   - add option in vvfat_strong_runtime_opts
   - replace floppy 1M/2M size limitation by 1440K or 2880K
   - correctly round up when "fat-size" cannot be expressed by CHS values.

Clément Chigot (5):
  vvfat: introduce partitioned option
  vvfat: move fat_type check prior to size setup
  vvfat: add a define for VVFAT_SECTOR_BITS and VVFAT_SECTOR_SIZE
  vvfat: move size parameters within driver structure
  vvfat: add support for "fat-size" options

 block/vvfat.c        | 353 +++++++++++++++++++++++++++++++------------
 qapi/block-core.json |  16 +-
 2 files changed, 273 insertions(+), 96 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] vvfat: introduce partitioned option

[Clément Chigot]

This option tells whether a hard disk should be partitioned or not. It
defaults to true and have the prime effect of preventing a master boot
record (MBR) to be initialized.

This is useful as some operating system (QNX, Rtems) don't
recognized FAT mounted disks (especially SD cards) if a MBR is present.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c        | 21 +++++++++++++++++++--
 qapi/block-core.json | 10 +++++++---
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 814796d918..de6031db98 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -306,7 +306,8 @@ typedef struct BDRVVVFATState {
     array_t fat,directory,mapping;
     char volume_label[11];
 
-    uint32_t offset_to_bootsector; /* 0 for floppy, 0x3f for disk */
+    /* 0x3f for partitioned disk, 0x0 otherwise */
+    uint32_t offset_to_bootsector;
 
     unsigned int cluster_size;
     unsigned int sectors_per_cluster;
@@ -1082,6 +1083,12 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Make the image writable",
         },
+        {
+            .name = "partitioned",
+            .type = QEMU_OPT_BOOL,
+            .def_value_str = "true",
+            .help = "Do not add a Master Boot Record on this disk",
+        },
         { /* end of list */ }
     },
 };
@@ -1092,6 +1099,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     int fat_type = 0;
     bool floppy = false;
     bool rw = false;
+    bool partitioned = true;
     int i;
 
     if (!strstart(filename, "fat:", NULL)) {
@@ -1116,6 +1124,10 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
         rw = true;
     }
 
+    if (strstr(filename, ":unpartitioned:")) {
+        partitioned = false;
+    }
+
     /* Get the directory name without options */
     i = strrchr(filename, ':') - filename;
     assert(i >= 3);
@@ -1131,6 +1143,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put_int(options, "fat-type", fat_type);
     qdict_put_bool(options, "floppy", floppy);
     qdict_put_bool(options, "rw", rw);
+    qdict_put_bool(options, "partitioned", partitioned);
 }
 
 static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
@@ -1196,7 +1209,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (!s->fat_type) {
             s->fat_type = 16;
         }
-        s->offset_to_bootsector = 0x3f;
+        /* Reserver space for MBR */
+        if (qemu_opt_get_bool(opts, "partitioned", true)) {
+            s->offset_to_bootsector = 0x3f;
+        }
         cyls = s->fat_type == 12 ? 64 : 1024;
         heads = 16;
         secs = 63;
@@ -3246,6 +3262,7 @@ static const char *const vvfat_strong_runtime_opts[] = {
     "floppy",
     "label",
     "rw",
+    "partitioned",
 
     NULL
 };
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b82af74256..8a479ba090 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3464,8 +3464,8 @@
 #
 # @fat-type: FAT type: 12, 16 or 32
 #
-# @floppy: whether to export a floppy image (true) or partitioned hard
-#     disk (false; default)
+# @floppy: whether to export a floppy image (true) or hard disk
+#     (false; default)
 #
 # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
 #     traditionally have some restrictions on labels, which are
@@ -3474,11 +3474,15 @@
 #
 # @rw: whether to allow write operations (default: false)
 #
+# @partitioned: whether a hard disk will be partitioned
+#     (default: true)
+#     (since 10.2)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsVVFAT',
   'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
-            '*label': 'str', '*rw': 'bool' } }
+            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
 
 ##
 # @BlockdevOptionsGenericFormat:
-- 
2.43.0

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

[Clément Chigot]

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

[PATCH v2 3/5] vvfat: add a define for VVFAT_SECTOR_BITS and VVFAT_SECTOR_SIZE

[Clément Chigot]

This removes some hardcoded 0x200 making them far clearer.

This also renames some BDRV_SECTOR_* constants that were introduced
during the transitions for sector-based to bytes-based block interfaces.
While they have the same values, the BDRV_SECTOR_* constants are
unrelated to the VVFAT sectors.

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

diff --git a/block/vvfat.c b/block/vvfat.c
index d8c8d44f16..1c51dfa561 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -75,6 +75,9 @@ static void checkpoint(void);
  */
 #define BOOTSECTOR_OEM_NAME "MSWIN4.1"
 
+#define VVFAT_SECTOR_BITS 9
+#define VVFAT_SECTOR_SIZE (1ULL << VVFAT_SECTOR_BITS)
+
 #define DIR_DELETED 0xe5
 #define DIR_KANJI DIR_DELETED
 #define DIR_KANJI_FAKE 0x05
@@ -300,7 +303,7 @@ static void print_mapping(const struct mapping_t* mapping);
 typedef struct BDRVVVFATState {
     CoMutex lock;
     BlockDriverState* bs; /* pointer to parent */
-    unsigned char first_sectors[0x40*0x200];
+    unsigned char first_sectors[0x40 * VVFAT_SECTOR_SIZE];
 
     int fat_type; /* 16 or 32 */
     array_t fat,directory,mapping;
@@ -690,11 +693,11 @@ static inline void init_fat(BDRVVVFATState* s)
     if (s->fat_type == 12) {
         array_init(&(s->fat),1);
         array_ensure_allocated(&(s->fat),
-                s->sectors_per_fat * 0x200 * 3 / 2 - 1);
+                s->sectors_per_fat * VVFAT_SECTOR_SIZE * 3 / 2 - 1);
     } else {
         array_init(&(s->fat),(s->fat_type==32?4:2));
         array_ensure_allocated(&(s->fat),
-                s->sectors_per_fat * 0x200 / s->fat.item_size - 1);
+                s->sectors_per_fat * VVFAT_SECTOR_SIZE / s->fat.item_size - 1);
     }
     memset(s->fat.pointer,0,s->fat.size);
 
@@ -902,19 +905,19 @@ static int init_directories(BDRVVVFATState* s,
     unsigned int i;
     unsigned int cluster;
 
-    memset(&(s->first_sectors[0]),0,0x40*0x200);
+    memset(&(s->first_sectors[0]), 0 , 0x40 * VVFAT_SECTOR_SIZE);
 
-    s->cluster_size=s->sectors_per_cluster*0x200;
+    s->cluster_size = s->sectors_per_cluster * VVFAT_SECTOR_SIZE;
     s->cluster_buffer=g_malloc(s->cluster_size);
 
     /*
-     * The formula: sc = spf+1+spf*spc*(512*8/fat_type),
+     * The formula: sc = spf+1+spf*spc*(VVFAT_SECTOR_SIZE*8/fat_type),
      * where sc is sector_count,
      * spf is sectors_per_fat,
      * spc is sectors_per_clusters, and
      * fat_type = 12, 16 or 32.
      */
-    i = 1+s->sectors_per_cluster*0x200*8/s->fat_type;
+    i = 1 + s->sectors_per_cluster * VVFAT_SECTOR_SIZE * 8 / s->fat_type;
     s->sectors_per_fat=(s->sector_count+i)/i; /* round up */
 
     s->offset_to_fat = s->offset_to_bootsector + 1;
@@ -1012,12 +1015,13 @@ static int init_directories(BDRVVVFATState* s,
     s->current_mapping = NULL;
 
     bootsector = (bootsector_t *)(s->first_sectors
-                                  + s->offset_to_bootsector * 0x200);
+                                  + s->offset_to_bootsector
+                                  * VVFAT_SECTOR_SIZE);
     bootsector->jump[0]=0xeb;
     bootsector->jump[1]=0x3e;
     bootsector->jump[2]=0x90;
     memcpy(bootsector->name, BOOTSECTOR_OEM_NAME, 8);
-    bootsector->sector_size=cpu_to_le16(0x200);
+    bootsector->sector_size = cpu_to_le16(VVFAT_SECTOR_SIZE);
     bootsector->sectors_per_cluster=s->sectors_per_cluster;
     bootsector->reserved_sectors=cpu_to_le16(1);
     bootsector->number_of_fats=0x2; /* number of FATs */
@@ -1313,7 +1317,7 @@ fail:
 
 static void vvfat_refresh_limits(BlockDriverState *bs, Error **errp)
 {
-    bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */
+    bs->bl.request_alignment = VVFAT_SECTOR_SIZE; /* No sub-sector I/O */
 }
 
 static inline void vvfat_close_current_file(BDRVVVFATState *s)
@@ -1499,21 +1503,23 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
         if (s->qcow) {
             int64_t n;
             int ret;
-            ret = bdrv_co_is_allocated(s->qcow->bs, sector_num * BDRV_SECTOR_SIZE,
-                                       (nb_sectors - i) * BDRV_SECTOR_SIZE, &n);
+            ret = bdrv_co_is_allocated(s->qcow->bs,
+                                       sector_num * VVFAT_SECTOR_SIZE,
+                                       (nb_sectors - i) * VVFAT_SECTOR_SIZE,
+                                       &n);
             if (ret < 0) {
                 return ret;
             }
             if (ret) {
                 DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
                              " allocated\n", sector_num,
-                             n >> BDRV_SECTOR_BITS));
-                if (bdrv_co_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE, n,
-                                  buf + i * 0x200, 0) < 0) {
+                             n >> VVFAT_SECTOR_BITS));
+                if (bdrv_co_pread(s->qcow, sector_num * VVFAT_SECTOR_SIZE,
+                                  n, buf + i * VVFAT_SECTOR_SIZE, 0) < 0) {
                     return -1;
                 }
-                i += (n >> BDRV_SECTOR_BITS) - 1;
-                sector_num += (n >> BDRV_SECTOR_BITS) - 1;
+                i += (n >> VVFAT_SECTOR_BITS) - 1;
+                sector_num += (n >> VVFAT_SECTOR_BITS) - 1;
                 continue;
             }
             DLOG(fprintf(stderr, "sector %" PRId64 " not allocated\n",
@@ -1521,19 +1527,20 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
         }
         if (sector_num < s->offset_to_root_dir) {
             if (sector_num < s->offset_to_fat) {
-                memcpy(buf + i * 0x200,
-                       &(s->first_sectors[sector_num * 0x200]),
-                       0x200);
+                memcpy(buf + i * VVFAT_SECTOR_SIZE,
+                       &(s->first_sectors[sector_num * VVFAT_SECTOR_SIZE]),
+                       VVFAT_SECTOR_SIZE);
             } else if (sector_num < s->offset_to_fat + s->sectors_per_fat) {
-                memcpy(buf + i * 0x200,
-                       &(s->fat.pointer[(sector_num
-                                       - s->offset_to_fat) * 0x200]),
-                       0x200);
+                memcpy(buf + i * VVFAT_SECTOR_SIZE,
+                       &(s->fat.pointer[(sector_num - s->offset_to_fat)
+                                        * VVFAT_SECTOR_SIZE]),
+                       VVFAT_SECTOR_SIZE);
             } else if (sector_num < s->offset_to_root_dir) {
-                memcpy(buf + i * 0x200,
+                memcpy(buf + i * VVFAT_SECTOR_SIZE,
                        &(s->fat.pointer[(sector_num - s->offset_to_fat
-                                       - s->sectors_per_fat) * 0x200]),
-                       0x200);
+                                         - s->sectors_per_fat)
+                                        * VVFAT_SECTOR_SIZE]),
+                       VVFAT_SECTOR_SIZE);
             }
         } else {
             uint32_t sector = sector_num - s->offset_to_root_dir,
@@ -1541,10 +1548,12 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
             cluster_num=sector/s->sectors_per_cluster;
             if(cluster_num > s->cluster_count || read_cluster(s, cluster_num) != 0) {
                 /* LATER TODO: strict: return -1; */
-                memset(buf+i*0x200,0,0x200);
+                memset(buf + i * VVFAT_SECTOR_SIZE, 0, VVFAT_SECTOR_SIZE);
                 continue;
             }
-            memcpy(buf+i*0x200,s->cluster+sector_offset_in_cluster*0x200,0x200);
+            memcpy(buf + i * VVFAT_SECTOR_SIZE,
+                   s->cluster + sector_offset_in_cluster * VVFAT_SECTOR_SIZE,
+                   VVFAT_SECTOR_SIZE);
         }
     }
     return 0;
@@ -1556,12 +1565,12 @@ vvfat_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
 {
     int ret;
     BDRVVVFATState *s = bs->opaque;
-    uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
-    int nb_sectors = bytes >> BDRV_SECTOR_BITS;
+    uint64_t sector_num = offset >> VVFAT_SECTOR_BITS;
+    int nb_sectors = bytes >> VVFAT_SECTOR_BITS;
     void *buf;
 
-    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
-    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(offset, VVFAT_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, VVFAT_SECTOR_SIZE));
 
     buf = g_try_malloc(bytes);
     if (bytes && buf == NULL) {
@@ -1827,8 +1836,8 @@ cluster_was_modified(BDRVVVFATState *s, uint32_t cluster_num)
     for (i = 0; !was_modified && i < s->sectors_per_cluster; i++) {
         was_modified = bdrv_co_is_allocated(s->qcow->bs,
                                             (cluster2sector(s, cluster_num) +
-                                             i) * BDRV_SECTOR_SIZE,
-                                            BDRV_SECTOR_SIZE, NULL);
+                                             i) * VVFAT_SECTOR_SIZE,
+                                            VVFAT_SECTOR_SIZE, NULL);
     }
 
     /*
@@ -1982,8 +1991,8 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
                     int res;
 
                     res = bdrv_co_is_allocated(s->qcow->bs,
-                                               (offs + i) * BDRV_SECTOR_SIZE,
-                                               BDRV_SECTOR_SIZE, NULL);
+                                               (offs + i) * VVFAT_SECTOR_SIZE,
+                                               VVFAT_SECTOR_SIZE, NULL);
                     if (res < 0) {
                         return -1;
                     }
@@ -1992,9 +2001,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
                         if (res) {
                             return -1;
                         }
-                        res = bdrv_co_pwrite(s->qcow, offs * BDRV_SECTOR_SIZE,
-                                             BDRV_SECTOR_SIZE, s->cluster_buffer,
-                                             0);
+                        res = bdrv_co_pwrite(s->qcow, offs * VVFAT_SECTOR_SIZE,
+                                             VVFAT_SECTOR_SIZE,
+                                             s->cluster_buffer, 0);
                         if (res < 0) {
                             return -2;
                         }
@@ -2172,7 +2181,7 @@ DLOG(checkpoint());
      * - if all is fine, return number of used clusters
      */
     if (s->fat2 == NULL) {
-        int size = 0x200 * s->sectors_per_fat;
+        int size = VVFAT_SECTOR_SIZE * s->sectors_per_fat;
         s->fat2 = g_malloc(size);
         memcpy(s->fat2, s->fat.pointer, size);
     }
@@ -2569,7 +2578,8 @@ commit_one_file(BDRVVVFATState* s, int dir_index, uint32_t offset)
                 (size > offset && c >=2 && !fat_eof(s, c)));
 
         ret = vvfat_read(s->bs, cluster2sector(s, c),
-            (uint8_t*)cluster, DIV_ROUND_UP(rest_size, 0x200));
+                         (uint8_t *)cluster,
+                         DIV_ROUND_UP(rest_size, VVFAT_SECTOR_SIZE));
 
         if (ret < 0) {
             qemu_close(fd);
@@ -2948,7 +2958,7 @@ static int coroutine_fn GRAPH_RDLOCK do_commit(BDRVVVFATState* s)
     }
 
     /* copy FAT (with bdrv_pread) */
-    memcpy(s->fat.pointer, s->fat2, 0x200 * s->sectors_per_fat);
+    memcpy(s->fat.pointer, s->fat2, VVFAT_SECTOR_SIZE * s->sectors_per_fat);
 
     /* recurse direntries from root (using bs->bdrv_pread) */
     ret = commit_direntries(s, 0, -1);
@@ -3012,14 +3022,15 @@ DLOG(checkpoint());
          * used to mark volume dirtiness
          */
         unsigned char *bootsector = s->first_sectors
-                                    + s->offset_to_bootsector * 0x200;
+                                    + s->offset_to_bootsector
+                                    * VVFAT_SECTOR_SIZE;
         /*
          * LATER TODO: if FAT32, this is wrong (see init_directories(),
          * which always creates a FAT16 bootsector)
          */
         const int reserved1_offset = offsetof(bootsector_t, u.fat16.reserved1);
 
-        for (i = 0; i < 0x200; i++) {
+        for (i = 0; i < VVFAT_SECTOR_SIZE; i++) {
             if (i != reserved1_offset && bootsector[i] != buf[i]) {
                 fprintf(stderr, "Tried to write to protected bootsector\n");
                 return -1;
@@ -3074,7 +3085,9 @@ DLOG(checkpoint());
                     end = sector_num + nb_sectors;
                 dir_index  = mapping->dir_index +
                     0x10 * (begin - mapping->begin * s->sectors_per_cluster);
-                direntries = (direntry_t*)(buf + 0x200 * (begin - sector_num));
+                direntries =
+                    (direntry_t *)(buf + VVFAT_SECTOR_SIZE
+                                   * (begin - sector_num));
 
                 for (k = 0; k < (end - begin) * 0x10; k++) {
                     /* no access to the direntry of a read-only file */
@@ -3100,8 +3113,8 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = bdrv_co_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE,
-                         nb_sectors * BDRV_SECTOR_SIZE, buf, 0);
+    ret = bdrv_co_pwrite(s->qcow, sector_num * VVFAT_SECTOR_SIZE,
+                         nb_sectors * VVFAT_SECTOR_SIZE, buf, 0);
     if (ret < 0) {
         fprintf(stderr, "Error writing to qcow backend\n");
         return ret;
@@ -3127,12 +3140,12 @@ vvfat_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
 {
     int ret;
     BDRVVVFATState *s = bs->opaque;
-    uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
-    int nb_sectors = bytes >> BDRV_SECTOR_BITS;
+    uint64_t sector_num = offset >> VVFAT_SECTOR_BITS;
+    int nb_sectors = bytes >> VVFAT_SECTOR_BITS;
     void *buf;
 
-    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
-    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(offset, VVFAT_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, VVFAT_SECTOR_SIZE));
 
     buf = g_try_malloc(bytes);
     if (bytes && buf == NULL) {
@@ -3198,7 +3211,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
 
     opts = qemu_opts_create(bdrv_qcow->create_opts, NULL, 0, &error_abort);
     qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
-                        bs->total_sectors * BDRV_SECTOR_SIZE, &error_abort);
+                        bs->total_sectors * VVFAT_SECTOR_SIZE, &error_abort);
     qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:", &error_abort);
 
     ret = bdrv_create(bdrv_qcow, s->qcow_filename, opts, errp);
-- 
2.43.0

[PATCH v2 4/5] vvfat: move size parameters within driver structure

[Clément Chigot]

At the same time, rename them to match bootsector fields.

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

diff --git a/block/vvfat.c b/block/vvfat.c
index 1c51dfa561..b0e591e35e 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -312,7 +312,10 @@ typedef struct BDRVVVFATState {
     /* 0x3f for partitioned disk, 0x0 otherwise */
     uint32_t offset_to_bootsector;
 
+    unsigned int cylinders;
     unsigned int cluster_size;
+    unsigned int number_of_heads;
+    unsigned int sectors_per_track;
     unsigned int sectors_per_cluster;
     unsigned int sectors_per_fat;
     uint32_t last_cluster_of_root_directory;
@@ -366,7 +369,7 @@ static int sector2CHS(mbr_chs_t *chs, int spos, int cyls, int heads, int secs)
     return 0;
 }
 
-static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
+static void init_mbr(BDRVVVFATState *s)
 {
     /* TODO: if the files mbr.img and bootsect.img exist, use them */
     mbr_t* real_mbr=(mbr_t*)s->first_sectors;
@@ -382,9 +385,9 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
 
     /* LBA is used when partition is outside the CHS geometry */
     lba  = sector2CHS(&partition->start_CHS, s->offset_to_bootsector,
-                     cyls, heads, secs);
+                      s->cylinders, s->number_of_heads, s->sectors_per_track);
     lba |= sector2CHS(&partition->end_CHS,   s->bs->total_sectors - 1,
-                     cyls, heads, secs);
+                      s->cylinders, s->number_of_heads, s->sectors_per_track);
 
     /*LBA partitions are identified only by start/length_sector_long not by CHS*/
     partition->start_sector_long  = cpu_to_le32(s->offset_to_bootsector);
@@ -896,8 +899,7 @@ static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num)
     return s->offset_to_root_dir + s->sectors_per_cluster * cluster_num;
 }
 
-static int init_directories(BDRVVVFATState* s,
-                            const char *dirname, int heads, int secs,
+static int init_directories(BDRVVVFATState *s, const char *dirname,
                             Error **errp)
 {
     bootsector_t* bootsector;
@@ -1031,8 +1033,8 @@ static int init_directories(BDRVVVFATState* s,
     bootsector->media_type = (s->offset_to_bootsector > 0 ? 0xf8 : 0xf0);
     s->fat.pointer[0] = bootsector->media_type;
     bootsector->sectors_per_fat=cpu_to_le16(s->sectors_per_fat);
-    bootsector->sectors_per_track = cpu_to_le16(secs);
-    bootsector->number_of_heads = cpu_to_le16(heads);
+    bootsector->sectors_per_track = cpu_to_le16(s->sectors_per_track);
+    bootsector->number_of_heads = cpu_to_le16(s->number_of_heads);
     bootsector->hidden_sectors = cpu_to_le32(s->offset_to_bootsector);
     bootsector->total_sectors=cpu_to_le32(s->sector_count>0xffff?s->sector_count:0);
 
@@ -1154,7 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     BDRVVVFATState *s = bs->opaque;
-    int cyls, heads, secs;
     bool floppy;
     const char *dirname, *label;
     QemuOpts *opts;
@@ -1221,18 +1222,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     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;
+        s->sectors_per_track = s->fat_type == 12 ? 18 : 36;
+        s->cylinders = 80;
+        s->number_of_heads = 2;
     } else {
         /* 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;
+        s->cylinders = s->fat_type == 12 ? 64 : 1024;
+        s->number_of_heads = 16;
+        s->sectors_per_track = 63;
     }
 
 
@@ -1249,10 +1250,13 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->downcase_short_names = 1;
 
     DLOG(fprintf(stderr, "vvfat %s chs %d,%d,%d\n",
-                 dirname, cyls, heads, secs));
+                 dirname, s->cylinders, s->number_of_heads,
+                 s->sectors_per_track));
 
-    s->sector_count = cyls * heads * secs - s->offset_to_bootsector;
-    bs->total_sectors = cyls * heads * secs;
+    s->sector_count = s->cylinders * s->number_of_heads *
+        s->sectors_per_track - s->offset_to_bootsector;
+    bs->total_sectors = s->cylinders * s->number_of_heads *
+        s->sectors_per_track;
 
     if (qemu_opt_get_bool(opts, "rw", false)) {
         if (!bdrv_is_read_only(bs)) {
@@ -1273,7 +1277,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    if (init_directories(s, dirname, heads, secs, errp)) {
+    if (init_directories(s, dirname, errp)) {
         ret = -EIO;
         goto fail;
     }
@@ -1294,7 +1298,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (s->offset_to_bootsector > 0) {
-        init_mbr(s, cyls, heads, secs);
+        init_mbr(s);
     }
 
     qemu_co_mutex_init(&s->lock);
-- 
2.43.0

[PATCH v2 5/5] vvfat: add support for "fat-size" options

[Clément Chigot]

This allows more flexibility to vvfat backend. The values of "Number of
Heads" and "Sectors per track" are based on SD specifications Part 2.

Due to the FAT architecture, not all sizes are reachable. Therefore, it
could be round up to the closest available size.

FAT32 has not been adjusted and thus still default to 504 Mib.

For floppy, only 1440 Kib and 2880 Kib are supported.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c        | 169 ++++++++++++++++++++++++++++++++++++++-----
 qapi/block-core.json |   8 +-
 2 files changed, 158 insertions(+), 19 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index b0e591e35e..96f5062939 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1095,6 +1095,11 @@ static QemuOptsList runtime_opts = {
             .def_value_str = "true",
             .help = "Do not add a Master Boot Record on this disk",
         },
+        {
+            .name = "fat-size",
+            .type = QEMU_OPT_SIZE,
+            .help = "Virtual disk size"
+        },
         { /* end of list */ }
     },
 };
@@ -1152,10 +1157,150 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put_bool(options, "partitioned", partitioned);
 }
 
+static void vvfat_get_size_parameters(uint64_t size, BDRVVVFATState *s,
+                                      bool floppy, Error **errp)
+{
+    if (floppy) {
+        if (s->fat_type == 16) {
+            /* The default for floppy FAT 16 is 2880 KiB */
+            if (!size) {
+                size = 2880 * 1024;
+            }
+
+            if (size != 2880 * 1024) {
+                error_setg(errp,
+                           "floppy FAT16 unsupported size; only support "
+                           "2880 KiB");
+            }
+
+        } else {
+            /* The default for floppy FAT 12 is 1440 KiB */
+            if (!size) {
+                size = 1440 * 1024;
+            }
+
+            if (size != 2880 * 1024 && size != 1440 * 1024) {
+                error_setg(errp,
+                           "floppy FAT12 unsupported size; only support "
+                           "1440 KiB or 2880 KiB");
+            }
+
+        }
+
+        if (s->fat_type == 12) {
+            if (size == 2880 * 1024) {
+                s->sectors_per_track = 36;
+            } else {
+                s->sectors_per_track = 18;
+            }
+        } else {
+            s->sectors_per_track = 36;
+        }
+
+        s->sectors_per_cluster = 0x10;
+        s->cylinders = 80;
+        s->number_of_heads = 2;
+    } else {
+
+        switch (s->fat_type) {
+        case 32:
+            /* TODO FAT32 adjust  */
+            if (size) {
+                warn_report("size parameters not supported with FAT32;"
+                            "default to 504 MiB.");
+            }
+            s->cylinders = 1024;
+            s->number_of_heads = 16;
+            s->sectors_per_cluster = 0x10;
+            s->sectors_per_track = 63;
+            return;
+
+        case 12:
+
+            /* Default is 32 MB */
+            if (!size) {
+                size = 32 * 1024 * 1024;
+            } else if (size > 32 * 1024 * 1024) {
+                error_setg(errp, "FAT12 unsupported size; higher than 32 MiB");
+            }
+
+            s->sectors_per_cluster = 0x10;
+            s->cylinders = 64;
+
+            /*
+             * Based on CHS Recommandation table:
+             *  Card Capacity | Number of Headers | Sectors per track
+             *      2 MiB     |         4         |       16
+             *      4 MiB     |         8         |       16
+             *      8 MiB     |        16         |       16
+             *      16 MiB    |         2         |       32
+             *      32 MiB    |         4         |       32
+             *
+             * For 2 MiB, the recommendation is heads = 2 and sectors = 16, but
+             * this requires a different number of cylinders. Thus, it was
+             * adjusted to keep this number constant.
+             */
+            if (size <= 8 * 1024 * 1024) {
+                s->sectors_per_track = 16;
+            } else {
+                s->sectors_per_track = 32;
+            }
+
+            break;
+
+        case 16:
+            /* Default is 504 MiB */
+            if (!size) {
+                size = 504 * 1024 * 1024;
+            } else if (size / 1024 > 4 * 1024 * 1024) {
+                error_setg(errp, "FAT16 unsupported size; higher than 4 GiB");
+            }
+
+            s->sectors_per_cluster = 0x10;
+            s->cylinders = 1024;
+
+            /*
+             * Based on CHS Recommandation table:
+             *  Card Capacity | Number of Headers | Sectors per track
+             *      64 MiB    |         4         |       32
+             *     128 MiB    |         8         |       32
+             *     256 MiB    |        16         |       32
+             *     504 MiB    |        16         |       63
+             *    1008 MiB    |        32         |       63
+             *    2016 MiB    |        64         |       63
+             */
+            if (size <= 256 * 1024 * 1024) {
+                s->sectors_per_track = 32;
+            } else {
+                s->sectors_per_track = 63;
+            }
+
+            break;
+        }
+
+        /*
+         * The formula between the size (in bytes) and the parameters are:
+         *  size = VVFAT_SECTOR_SIZE * sectors_per_track * number_of_headers *
+         *         cylinders
+         *
+         */
+        s->number_of_heads = size / s->sectors_per_track /
+            VVFAT_SECTOR_SIZE / s->cylinders;
+
+        /* Round up to the closest size if the given one cannot be reached. */
+        if (size %
+            (s->sectors_per_track * VVFAT_SECTOR_SIZE * s->cylinders) != 0) {
+            s->number_of_heads++;
+        }
+
+    }
+}
+
 static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     BDRVVVFATState *s = bs->opaque;
+    uint64_t size;
     bool floppy;
     const char *dirname, *label;
     QemuOpts *opts;
@@ -1182,6 +1327,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->fat_type = qemu_opt_get_number(opts, "fat-type", 0);
     floppy = qemu_opt_get_bool(opts, "floppy", false);
+    size = qemu_opt_get_size_del(opts, "fat-size", 0);
 
     memset(s->volume_label, ' ', sizeof(s->volume_label));
     label = qemu_opt_get(opts, "label");
@@ -1219,29 +1365,15 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    vvfat_get_size_parameters(size, s, floppy, errp);
 
-    if (floppy) {
-        /* Choose floppy size. 1440 KiB for FAT 12, 2880 KiB for FAT-16 */
-        s->sectors_per_track = s->fat_type == 12 ? 18 : 36;
-        s->cylinders = 80;
-        s->number_of_heads = 2;
-    } else {
-        /* Reserver space for MBR */
-        if (qemu_opt_get_bool(opts, "partitioned", true)) {
-            s->offset_to_bootsector = 0x3f;
-        }
-        /* 32MB or 504MB disk*/
-        s->cylinders = s->fat_type == 12 ? 64 : 1024;
-        s->number_of_heads = 16;
-        s->sectors_per_track = 63;
+    /* Reserver space for MBR */
+    if (!floppy && qemu_opt_get_bool(opts, "partitioned", true)) {
+        s->offset_to_bootsector = 0x3f;
     }
 
-
     s->bs = bs;
 
-    /* LATER TODO: if FAT32, adjust */
-    s->sectors_per_cluster=0x10;
-
     s->current_cluster=0xffffffff;
 
     s->qcow = NULL;
@@ -3280,6 +3412,7 @@ static const char *const vvfat_strong_runtime_opts[] = {
     "label",
     "rw",
     "partitioned",
+    "fat-size",
 
     NULL
 };
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8a479ba090..0bcb360320 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3478,11 +3478,17 @@
 #     (default: true)
 #     (since 10.2)
 #
+# @fat-size: size of the device in bytes.  Due to FAT underlying
+#     architecture, this size can be rounded up to the closest valid
+#     size.
+#     (since 10.2)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsVVFAT',
   'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
-            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
+            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool',
+            'fat-size': 'int' } }
 
 ##
 # @BlockdevOptionsGenericFormat:
-- 
2.43.0

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Markus Armbruster]

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

> This option tells whether a hard disk should be partitioned or not. It
> defaults to true and have the prime effect of preventing a master boot
> record (MBR) to be initialized.
>
> This is useful as some operating system (QNX, Rtems) don't
> recognized FAT mounted disks (especially SD cards) if a MBR is present.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b82af74256..8a479ba090 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3464,8 +3464,8 @@
>  #
>  # @fat-type: FAT type: 12, 16 or 32
>  #
> -# @floppy: whether to export a floppy image (true) or partitioned hard
> -#     disk (false; default)
> +# @floppy: whether to export a floppy image (true) or hard disk
> +#     (false; default)
>  #
>  # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
>  #     traditionally have some restrictions on labels, which are
> @@ -3474,11 +3474,15 @@
>  #
>  # @rw: whether to allow write operations (default: false)
>  #
> +# @partitioned: whether a hard disk will be partitioned

How does "partitioned" combine with "floppy": true?

Is it silently ignored?

Is it an error if present?

Is it an error if true?

Does it add a partition table if true?

> +#     (default: true)

Hmm, this suggests it's silently ignored.

Silently ignoring nonsensical configuration is usually a bad idea.

> +#     (since 10.2)
> +#

Not sure I like "partitioned".  Is a disk with an MBR and a partition
table contraining a single partition partitioned?  Call it "mbr"?

>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsVVFAT',
>    'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> -            '*label': 'str', '*rw': 'bool' } }
> +            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
>  
>  ##
>  # @BlockdevOptionsGenericFormat:

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

[Markus Armbruster]

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?

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Markus Armbruster]

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

> This allows more flexibility to vvfat backend. The values of "Number of
> Heads" and "Sectors per track" are based on SD specifications Part 2.
>
> Due to the FAT architecture, not all sizes are reachable. Therefore, it
> could be round up to the closest available size.
>
> FAT32 has not been adjusted and thus still default to 504 Mib.
>
> For floppy, only 1440 Kib and 2880 Kib are supported.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8a479ba090..0bcb360320 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3478,11 +3478,17 @@
>  #     (default: true)
>  #     (since 10.2)
>  #
> +# @fat-size: size of the device in bytes.  Due to FAT underlying
> +#     architecture, this size can be rounded up to the closest valid
> +#     size.
> +#     (since 10.2)
> +#

Can you explain again why you moved from @size to @fat-size?

I assume you dropped the horrible special floppy sizes because ordinary
sizes suffice.  Correct?

>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsVVFAT',
>    'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> -            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
> +            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool',
> +            'fat-size': 'int' } }
>  
>  ##
>  # @BlockdevOptionsGenericFormat:

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Clément Chigot]

On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Clément Chigot <chigot@adacore.com> writes:
>
> > This option tells whether a hard disk should be partitioned or not. It
> > defaults to true and have the prime effect of preventing a master boot
> > record (MBR) to be initialized.
> >
> > This is useful as some operating system (QNX, Rtems) don't
> > recognized FAT mounted disks (especially SD cards) if a MBR is present.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> [...]
>
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index b82af74256..8a479ba090 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3464,8 +3464,8 @@
> >  #
> >  # @fat-type: FAT type: 12, 16 or 32
> >  #
> > -# @floppy: whether to export a floppy image (true) or partitioned hard
> > -#     disk (false; default)
> > +# @floppy: whether to export a floppy image (true) or hard disk
> > +#     (false; default)
> >  #
> >  # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
> >  #     traditionally have some restrictions on labels, which are
> > @@ -3474,11 +3474,15 @@
> >  #
> >  # @rw: whether to allow write operations (default: false)
> >  #
> > +# @partitioned: whether a hard disk will be partitioned
>
> How does "partitioned" combine with "floppy": true?
>
> Is it silently ignored?
>
> Is it an error if present?
>
> Is it an error if true?
>
> Does it add a partition table if true?
>
> > +#     (default: true)
>
> Hmm, this suggests it's silently ignored.
>
> Silently ignoring nonsensical configuration is usually a bad idea.

True, but that would mean "unpartitioned" must always be passed when
"floppy" is requested. That would make such command lines a bit more
verbose, but otherwise I don't think there is any issue to that.

Note that I didn't add "partition" as a keyword in the command line.
Currently, it's either the default (thus partitioned) or
"unpartitioned" being requested. Do you think it makes sense to add it
as well, even if it's redundant ?

> > +#     (since 10.2)
> > +#
>
> Not sure I like "partitioned".  Is a disk with an MBR and a partition
> table contraining a single partition partitioned?  Call it "mbr"?

It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
V1. Honestly I'm fine with both options:
 - Technically, the option prevents MBR which has a side effect for
preventing partition tables
 - Even it has a single partition, I think it makes sense to call a
disk "partitioned" as long as it has a partition table

But I'm not that familiar with disk formats, etc. I'll let you decide
with Kevin, which one you prefer.

> >  # Since: 2.9
> >  ##
> >  { 'struct': 'BlockdevOptionsVVFAT',
> >    'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> > -            '*label': 'str', '*rw': 'bool' } }
> > +            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
> >
> >  ##
> >  # @BlockdevOptionsGenericFormat:
>

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

[Clément Chigot]

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.

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Clément Chigot]

On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Clément Chigot <chigot@adacore.com> writes:
>
> > This allows more flexibility to vvfat backend. The values of "Number of
> > Heads" and "Sectors per track" are based on SD specifications Part 2.
> >
> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> > could be round up to the closest available size.
> >
> > FAT32 has not been adjusted and thus still default to 504 Mib.
> >
> > For floppy, only 1440 Kib and 2880 Kib are supported.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> [...]
>
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 8a479ba090..0bcb360320 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3478,11 +3478,17 @@
> >  #     (default: true)
> >  #     (since 10.2)
> >  #
> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> > +#     architecture, this size can be rounded up to the closest valid
> > +#     size.
> > +#     (since 10.2)
> > +#
>
> Can you explain again why you moved from @size to @fat-size?

Just to be sure, you mean in the above comment, in the commit message or both ?

> I assume you dropped the horrible special floppy sizes because ordinary
> sizes suffice.  Correct?

Yes. Clearly better that way.

> >  # Since: 2.9
> >  ##
> >  { 'struct': 'BlockdevOptionsVVFAT',
> >    'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> > -            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
> > +            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool',
> > +            'fat-size': 'int' } }
> >
> >  ##
> >  # @BlockdevOptionsGenericFormat:
>

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[BALATON Zoltan]

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

On Mon, 10 Nov 2025, Clément Chigot wrote:
> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Clément Chigot <chigot@adacore.com> writes:
>>
>>> This option tells whether a hard disk should be partitioned or not. It
>>> defaults to true and have the prime effect of preventing a master boot
>>> record (MBR) to be initialized.
>>>
>>> This is useful as some operating system (QNX, Rtems) don't
>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
>>>
>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>
>> [...]
>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index b82af74256..8a479ba090 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3464,8 +3464,8 @@
>>>  #
>>>  # @fat-type: FAT type: 12, 16 or 32
>>>  #
>>> -# @floppy: whether to export a floppy image (true) or partitioned hard
>>> -#     disk (false; default)
>>> +# @floppy: whether to export a floppy image (true) or hard disk
>>> +#     (false; default)
>>>  #
>>>  # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
>>>  #     traditionally have some restrictions on labels, which are
>>> @@ -3474,11 +3474,15 @@
>>>  #
>>>  # @rw: whether to allow write operations (default: false)
>>>  #
>>> +# @partitioned: whether a hard disk will be partitioned
>>
>> How does "partitioned" combine with "floppy": true?
>>
>> Is it silently ignored?
>>
>> Is it an error if present?
>>
>> Is it an error if true?
>>
>> Does it add a partition table if true?
>>
>>> +#     (default: true)
>>
>> Hmm, this suggests it's silently ignored.
>>
>> Silently ignoring nonsensical configuration is usually a bad idea.
>
> True, but that would mean "unpartitioned" must always be passed when
> "floppy" is requested. That would make such command lines a bit more
> verbose, but otherwise I don't think there is any issue to that.
>
> Note that I didn't add "partition" as a keyword in the command line.
> Currently, it's either the default (thus partitioned) or
> "unpartitioned" being requested. Do you think it makes sense to add it
> as well, even if it's redundant ?
>
>>> +#     (since 10.2)
>>> +#
>>
>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
>> table contraining a single partition partitioned?  Call it "mbr"?
>
> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
> V1. Honestly I'm fine with both options:
> - Technically, the option prevents MBR which has a side effect for
> preventing partition tables
> - Even it has a single partition, I think it makes sense to call a
> disk "partitioned" as long as it has a partition table
>
> But I'm not that familiar with disk formats, etc. I'll let you decide
> with Kevin, which one you prefer.

I'd also vote for mbr or similar shorter name; unpartitioned is awkward to 
type out in a command line. Maybe it can default to false for floppy and 
true for disk to preserve current behaviour but allow controlling it.

Regards,
BALATON Zoltan

>>>  # Since: 2.9
>>>  ##
>>>  { 'struct': 'BlockdevOptionsVVFAT',
>>>    'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
>>> -            '*label': 'str', '*rw': 'bool' } }
>>> +            '*label': 'str', '*rw': 'bool', '*partitioned': 'bool' } }
>>>
>>>  ##
>>>  # @BlockdevOptionsGenericFormat:
>>
>
>

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Markus Armbruster]

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

> On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Clément Chigot <chigot@adacore.com> writes:
>>
>> > This allows more flexibility to vvfat backend. The values of "Number of
>> > Heads" and "Sectors per track" are based on SD specifications Part 2.
>> >
>> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
>> > could be round up to the closest available size.
>> >
>> > FAT32 has not been adjusted and thus still default to 504 Mib.
>> >
>> > For floppy, only 1440 Kib and 2880 Kib are supported.
>> >
>> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>>
>> [...]
>>
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 8a479ba090..0bcb360320 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -3478,11 +3478,17 @@
>> >  #     (default: true)
>> >  #     (since 10.2)
>> >  #
>> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
>> > +#     architecture, this size can be rounded up to the closest valid
>> > +#     size.
>> > +#     (since 10.2)
>> > +#
>>
>> Can you explain again why you moved from @size to @fat-size?
>
> Just to be sure, you mean in the above comment, in the commit message or both ?

Just to me, because I'm not sure I like the change, but that may well be
due to a lack of understanding of your reasons.

[...]

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

[Markus Armbruster]

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.

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 10 Nov 2025, Clément Chigot wrote:
>> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Clément Chigot <chigot@adacore.com> writes:
>>>
>>>> This option tells whether a hard disk should be partitioned or not. It
>>>> defaults to true and have the prime effect of preventing a master boot
>>>> record (MBR) to be initialized.
>>>>
>>>> This is useful as some operating system (QNX, Rtems) don't
>>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
>>>>
>>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>>
>>> [...]
>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index b82af74256..8a479ba090 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -3464,8 +3464,8 @@
>>>>  #
>>>>  # @fat-type: FAT type: 12, 16 or 32
>>>>  #
>>>> -# @floppy: whether to export a floppy image (true) or partitioned hard
>>>> -#     disk (false; default)
>>>> +# @floppy: whether to export a floppy image (true) or hard disk
>>>> +#     (false; default)
>>>>  #
>>>>  # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
>>>>  #     traditionally have some restrictions on labels, which are
>>>> @@ -3474,11 +3474,15 @@
>>>>  #
>>>>  # @rw: whether to allow write operations (default: false)
>>>>  #
>>>> +# @partitioned: whether a hard disk will be partitioned
>>>
>>> How does "partitioned" combine with "floppy": true?
>>>
>>> Is it silently ignored?
>>>
>>> Is it an error if present?
>>>
>>> Is it an error if true?
>>>
>>> Does it add a partition table if true?
>>>
>>>> +#     (default: true)
>>>
>>> Hmm, this suggests it's silently ignored.
>>>
>>> Silently ignoring nonsensical configuration is usually a bad idea.
>>
>> True, but that would mean "unpartitioned" must always be passed when
>> "floppy" is requested. That would make such command lines a bit more
>> verbose, but otherwise I don't think there is any issue to that.
>>
>> Note that I didn't add "partition" as a keyword in the command line.
>> Currently, it's either the default (thus partitioned) or
>> "unpartitioned" being requested. Do you think it makes sense to add it
>> as well, even if it's redundant ?
>>
>>>> +#     (since 10.2)
>>>> +#
>>>
>>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
>>> table contraining a single partition partitioned?  Call it "mbr"?
>>
>> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
>> V1. Honestly I'm fine with both options:
>> - Technically, the option prevents MBR which has a side effect for
>> preventing partition tables

Yes, because the partition table is part of the MBR.  I'd rather name
the option after the entire thing it controls, not one of its parts.

>> - Even it has a single partition, I think it makes sense to call a
>> disk "partitioned" as long as it has a partition table
>>
>> But I'm not that familiar with disk formats, etc. I'll let you decide
>> with Kevin, which one you prefer.

Kevin is the maintainer, I just serve as advisor here.

> I'd also vote for mbr or similar shorter name; unpartitioned is awkward to type out in a command line. Maybe it can default to false for floppy and true for disk to preserve current behaviour but allow controlling it.

I'm not a fan of conditional defaults, but I think it's better than a
nonsensical default that gets ignored.

[...]

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Clément Chigot]

On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Clément Chigot <chigot@adacore.com> writes:
>
> > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Clément Chigot <chigot@adacore.com> writes:
> >>
> >> > This allows more flexibility to vvfat backend. The values of "Number of
> >> > Heads" and "Sectors per track" are based on SD specifications Part 2.
> >> >
> >> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> >> > could be round up to the closest available size.
> >> >
> >> > FAT32 has not been adjusted and thus still default to 504 Mib.
> >> >
> >> > For floppy, only 1440 Kib and 2880 Kib are supported.
> >> >
> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> >>
> >> [...]
> >>
> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> > index 8a479ba090..0bcb360320 100644
> >> > --- a/qapi/block-core.json
> >> > +++ b/qapi/block-core.json
> >> > @@ -3478,11 +3478,17 @@
> >> >  #     (default: true)
> >> >  #     (since 10.2)
> >> >  #
> >> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> >> > +#     architecture, this size can be rounded up to the closest valid
> >> > +#     size.
> >> > +#     (since 10.2)
> >> > +#
> >>
> >> Can you explain again why you moved from @size to @fat-size?
> >
> > Just to be sure, you mean in the above comment, in the commit message or both ?
>
> Just to me, because I'm not sure I like the change, but that may well be
> due to a lack of understanding of your reasons.

Naming `fat-size` instead of `size` ensures the parameter is only
recognized by the vvfat backend. In particular, it will be refused by
the default raw format, avoiding confusion:
 "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
truncated to 256M, raw format being implicit.
 "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
unsupported by raw format.
 "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
 "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
unsupported by vvfat format.

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Markus Armbruster]

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

> On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Clément Chigot <chigot@adacore.com> writes:
>>
>> > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Clément Chigot <chigot@adacore.com> writes:
>> >>
>> >> > This allows more flexibility to vvfat backend. The values of "Number of
>> >> > Heads" and "Sectors per track" are based on SD specifications Part 2.
>> >> >
>> >> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
>> >> > could be round up to the closest available size.
>> >> >
>> >> > FAT32 has not been adjusted and thus still default to 504 Mib.
>> >> >
>> >> > For floppy, only 1440 Kib and 2880 Kib are supported.
>> >> >
>> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>> >>
>> >> [...]
>> >>
>> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> > index 8a479ba090..0bcb360320 100644
>> >> > --- a/qapi/block-core.json
>> >> > +++ b/qapi/block-core.json
>> >> > @@ -3478,11 +3478,17 @@
>> >> >  #     (default: true)
>> >> >  #     (since 10.2)
>> >> >  #
>> >> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
>> >> > +#     architecture, this size can be rounded up to the closest valid
>> >> > +#     size.
>> >> > +#     (since 10.2)
>> >> > +#
>> >>
>> >> Can you explain again why you moved from @size to @fat-size?
>> >
>> > Just to be sure, you mean in the above comment, in the commit message or both ?
>>
>> Just to me, because I'm not sure I like the change, but that may well be
>> due to a lack of understanding of your reasons.
>
> Naming `fat-size` instead of `size` ensures the parameter is only
> recognized by the vvfat backend. In particular, it will be refused by
> the default raw format, avoiding confusion:
>  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
> truncated to 256M, raw format being implicit.
>  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
> unsupported by raw format.

I figure throwing in format=raw to make raw format explicit doesn't
change anything.  Correct?

>  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
>  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
> unsupported by vvfat format.

If it was called @size, what behavior would we get?  Just two cases, I
think:

1. With raw format:

    -drive file=fat:<path>,size=256M

2. Without raw format:

    -drive file=fat:<path>,format=vvfat,size=256M

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Clément Chigot]

On Mon, Nov 10, 2025 at 2:42 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Clément Chigot <chigot@adacore.com> writes:
>
> > On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Clément Chigot <chigot@adacore.com> writes:
> >>
> >> > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>
> >> >> Clément Chigot <chigot@adacore.com> writes:
> >> >>
> >> >> > This allows more flexibility to vvfat backend. The values of "Number of
> >> >> > Heads" and "Sectors per track" are based on SD specifications Part 2.
> >> >> >
> >> >> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> >> >> > could be round up to the closest available size.
> >> >> >
> >> >> > FAT32 has not been adjusted and thus still default to 504 Mib.
> >> >> >
> >> >> > For floppy, only 1440 Kib and 2880 Kib are supported.
> >> >> >
> >> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> >> >>
> >> >> [...]
> >> >>
> >> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> > index 8a479ba090..0bcb360320 100644
> >> >> > --- a/qapi/block-core.json
> >> >> > +++ b/qapi/block-core.json
> >> >> > @@ -3478,11 +3478,17 @@
> >> >> >  #     (default: true)
> >> >> >  #     (since 10.2)
> >> >> >  #
> >> >> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> >> >> > +#     architecture, this size can be rounded up to the closest valid
> >> >> > +#     size.
> >> >> > +#     (since 10.2)
> >> >> > +#
> >> >>
> >> >> Can you explain again why you moved from @size to @fat-size?
> >> >
> >> > Just to be sure, you mean in the above comment, in the commit message or both ?
> >>
> >> Just to me, because I'm not sure I like the change, but that may well be
> >> due to a lack of understanding of your reasons.
> >
> > Naming `fat-size` instead of `size` ensures the parameter is only
> > recognized by the vvfat backend. In particular, it will be refused by
> > the default raw format, avoiding confusion:
> >  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
> > truncated to 256M, raw format being implicit.
> >  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
> > unsupported by raw format.
>
> I figure throwing in format=raw to make raw format explicit doesn't
> change anything.  Correct?
>
> >  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
> >  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
> > unsupported by vvfat format.
>
> If it was called @size, what behavior would we get?  Just two cases, I
> think:
>
> 1. With raw format:
>
>     -drive file=fat:<path>,size=256M
>
> 2. Without raw format:
>
>     -drive file=fat:<path>,format=vvfat,size=256M

Yes and both result in a FAT system having different sizes. The only
difference being "format=vvfat". When @size is renamed @fat-size, you
are certain to get an error when forgetting that format=vvfat.
Moreover, one could think that one day,
`format=vvfat,size=256M,fat-size=128M` could coexist, creating a 256M
disk with a 128M FAT partition.

Again, I'm not against renaming @size, but I like Kevin's idea to
avoid confusing errors just because you forgot "format".

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Kevin Wolf]

Am 10.11.2025 um 14:20 hat Markus Armbruster geschrieben:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
> 
> > On Mon, 10 Nov 2025, Clément Chigot wrote:
> >> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>> Clément Chigot <chigot@adacore.com> writes:
> >>>
> >>>> This option tells whether a hard disk should be partitioned or not. It
> >>>> defaults to true and have the prime effect of preventing a master boot
> >>>> record (MBR) to be initialized.
> >>>>
> >>>> This is useful as some operating system (QNX, Rtems) don't
> >>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
> >>>>
> >>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
> >>>
> >>> [...]
> >>>
> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>>> index b82af74256..8a479ba090 100644
> >>>> --- a/qapi/block-core.json
> >>>> +++ b/qapi/block-core.json
> >>>> @@ -3464,8 +3464,8 @@
> >>>>  #
> >>>>  # @fat-type: FAT type: 12, 16 or 32
> >>>>  #
> >>>> -# @floppy: whether to export a floppy image (true) or partitioned hard
> >>>> -#     disk (false; default)
> >>>> +# @floppy: whether to export a floppy image (true) or hard disk
> >>>> +#     (false; default)
> >>>>  #
> >>>>  # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
> >>>>  #     traditionally have some restrictions on labels, which are
> >>>> @@ -3474,11 +3474,15 @@
> >>>>  #
> >>>>  # @rw: whether to allow write operations (default: false)
> >>>>  #
> >>>> +# @partitioned: whether a hard disk will be partitioned
> >>>
> >>> How does "partitioned" combine with "floppy": true?
> >>>
> >>> Is it silently ignored?
> >>>
> >>> Is it an error if present?
> >>>
> >>> Is it an error if true?
> >>>
> >>> Does it add a partition table if true?
> >>>
> >>>> +#     (default: true)
> >>>
> >>> Hmm, this suggests it's silently ignored.
> >>>
> >>> Silently ignoring nonsensical configuration is usually a bad idea.
> >>
> >> True, but that would mean "unpartitioned" must always be passed when
> >> "floppy" is requested. That would make such command lines a bit more
> >> verbose, but otherwise I don't think there is any issue to that.
> >>
> >> Note that I didn't add "partition" as a keyword in the command line.
> >> Currently, it's either the default (thus partitioned) or
> >> "unpartitioned" being requested. Do you think it makes sense to add it
> >> as well, even if it's redundant ?
> >>
> >>>> +#     (since 10.2)
> >>>> +#
> >>>
> >>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
> >>> table contraining a single partition partitioned?  Call it "mbr"?
> >>
> >> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
> >> V1. Honestly I'm fine with both options:
> >> - Technically, the option prevents MBR which has a side effect for
> >> preventing partition tables
> 
> Yes, because the partition table is part of the MBR.  I'd rather name
> the option after the entire thing it controls, not one of its parts.
> 
> >> - Even it has a single partition, I think it makes sense to call a
> >> disk "partitioned" as long as it has a partition table
> >>
> >> But I'm not that familiar with disk formats, etc. I'll let you decide
> >> with Kevin, which one you prefer.
> 
> Kevin is the maintainer, I just serve as advisor here.

I figured that the meaning of "partitioned" is easier to understand for
a casual user than having or not having an MBR ("I don't want to boot
from this disk, why would I care about a boot record?").

But if people think that "mbr" is better, that's fine with me.

The only thing I really didn't want is the negative "no-mbr" and the
double negation in "no-mbr=off" that comes with it.

> > I'd also vote for mbr or similar shorter name; unpartitioned is
> > awkward to type out in a command line. Maybe it can default to false
> > for floppy and true for disk to preserve current behaviour but allow
> > controlling it.
> 
> I'm not a fan of conditional defaults, but I think it's better than a
> nonsensical default that gets ignored.

I think in this case a conditional default makes sense, not only for
compatibility reasons. Hard disks almost always have a partition, floppy
disks with partitions are basically unheard of.

Kevin

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Kevin Wolf]

Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
> Clément Chigot <chigot@adacore.com> writes:
> 
> > On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Clément Chigot <chigot@adacore.com> writes:
> >>
> >> > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>
> >> >> Clément Chigot <chigot@adacore.com> writes:
> >> >>
> >> >> > This allows more flexibility to vvfat backend. The values of "Number of
> >> >> > Heads" and "Sectors per track" are based on SD specifications Part 2.
> >> >> >
> >> >> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> >> >> > could be round up to the closest available size.
> >> >> >
> >> >> > FAT32 has not been adjusted and thus still default to 504 Mib.
> >> >> >
> >> >> > For floppy, only 1440 Kib and 2880 Kib are supported.
> >> >> >
> >> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> >> >>
> >> >> [...]
> >> >>
> >> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> > index 8a479ba090..0bcb360320 100644
> >> >> > --- a/qapi/block-core.json
> >> >> > +++ b/qapi/block-core.json
> >> >> > @@ -3478,11 +3478,17 @@
> >> >> >  #     (default: true)
> >> >> >  #     (since 10.2)
> >> >> >  #
> >> >> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> >> >> > +#     architecture, this size can be rounded up to the closest valid
> >> >> > +#     size.
> >> >> > +#     (since 10.2)
> >> >> > +#
> >> >>
> >> >> Can you explain again why you moved from @size to @fat-size?
> >> >
> >> > Just to be sure, you mean in the above comment, in the commit message or both ?
> >>
> >> Just to me, because I'm not sure I like the change, but that may well be
> >> due to a lack of understanding of your reasons.
> >
> > Naming `fat-size` instead of `size` ensures the parameter is only
> > recognized by the vvfat backend. In particular, it will be refused by
> > the default raw format, avoiding confusion:
> >  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
> > truncated to 256M, raw format being implicit.
> >  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
> > unsupported by raw format.
> 
> I figure throwing in format=raw to make raw format explicit doesn't
> change anything.  Correct?
> 
> >  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
> >  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
> > unsupported by vvfat format.
> 
> If it was called @size, what behavior would we get?  Just two cases, I
> think:
> 
> 1. With raw format:
> 
>     -drive file=fat:<path>,size=256M

You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
corrupted file system). It's quite easy to forget format=vvfat, so
something that initially looks like it might be working is not a great
result for this user error.

> 2. Without raw format:
> 
>     -drive file=fat:<path>,format=vvfat,size=256M

This does the thing that you actually want, a 256 MiB file system.

I suggested to rename the vvfat option in v1 to make accidents at least
a bit less likely. I'm not completely sure if "fat-size" is the best
name, though, as it sounds as if it referred to the FAT itself instead
of the FAT filesystem. Maybe "fs-size"?

Kevin

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[BALATON Zoltan]

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

On Mon, 10 Nov 2025, Kevin Wolf wrote:
> Am 10.11.2025 um 14:20 hat Markus Armbruster geschrieben:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>
>>> On Mon, 10 Nov 2025, Clément Chigot wrote:
>>>> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>>
>>>>>> This option tells whether a hard disk should be partitioned or not. It
>>>>>> defaults to true and have the prime effect of preventing a master boot
>>>>>> record (MBR) to be initialized.
>>>>>>
>>>>>> This is useful as some operating system (QNX, Rtems) don't
>>>>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
>>>>>>
>>>>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>>> index b82af74256..8a479ba090 100644
>>>>>> --- a/qapi/block-core.json
>>>>>> +++ b/qapi/block-core.json
>>>>>> @@ -3464,8 +3464,8 @@
>>>>>>  #
>>>>>>  # @fat-type: FAT type: 12, 16 or 32
>>>>>>  #
>>>>>> -# @floppy: whether to export a floppy image (true) or partitioned hard
>>>>>> -#     disk (false; default)
>>>>>> +# @floppy: whether to export a floppy image (true) or hard disk
>>>>>> +#     (false; default)
>>>>>>  #
>>>>>>  # @label: set the volume label, limited to 11 bytes.  FAT16 and FAT32
>>>>>>  #     traditionally have some restrictions on labels, which are
>>>>>> @@ -3474,11 +3474,15 @@
>>>>>>  #
>>>>>>  # @rw: whether to allow write operations (default: false)
>>>>>>  #
>>>>>> +# @partitioned: whether a hard disk will be partitioned
>>>>>
>>>>> How does "partitioned" combine with "floppy": true?
>>>>>
>>>>> Is it silently ignored?
>>>>>
>>>>> Is it an error if present?
>>>>>
>>>>> Is it an error if true?
>>>>>
>>>>> Does it add a partition table if true?
>>>>>
>>>>>> +#     (default: true)
>>>>>
>>>>> Hmm, this suggests it's silently ignored.
>>>>>
>>>>> Silently ignoring nonsensical configuration is usually a bad idea.
>>>>
>>>> True, but that would mean "unpartitioned" must always be passed when
>>>> "floppy" is requested. That would make such command lines a bit more
>>>> verbose, but otherwise I don't think there is any issue to that.
>>>>
>>>> Note that I didn't add "partition" as a keyword in the command line.
>>>> Currently, it's either the default (thus partitioned) or
>>>> "unpartitioned" being requested. Do you think it makes sense to add it
>>>> as well, even if it's redundant ?
>>>>
>>>>>> +#     (since 10.2)
>>>>>> +#
>>>>>
>>>>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
>>>>> table contraining a single partition partitioned?  Call it "mbr"?
>>>>
>>>> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
>>>> V1. Honestly I'm fine with both options:
>>>> - Technically, the option prevents MBR which has a side effect for
>>>> preventing partition tables
>>
>> Yes, because the partition table is part of the MBR.  I'd rather name
>> the option after the entire thing it controls, not one of its parts.
>>
>>>> - Even it has a single partition, I think it makes sense to call a
>>>> disk "partitioned" as long as it has a partition table
>>>>
>>>> But I'm not that familiar with disk formats, etc. I'll let you decide
>>>> with Kevin, which one you prefer.
>>
>> Kevin is the maintainer, I just serve as advisor here.
>
> I figured that the meaning of "partitioned" is easier to understand for
> a casual user than having or not having an MBR ("I don't want to boot
> from this disk, why would I care about a boot record?").
>
> But if people think that "mbr" is better, that's fine with me.

I think partitioned is both inconvenient and not specific enough as there 
could be other partitioning schemes.

> The only thing I really didn't want is the negative "no-mbr" and the
> double negation in "no-mbr=off" that comes with it.

Having mbr=true|false would be clear enough IMO so no need for the 
negated version.

If we're already bikeshedding this I also thought fat-size may not be the 
best name as I think about 12,16,32 as fat-size so maybe it could be 
called vfat-size or similar. But why can't it just be size and when 
specified for raw vfat would use the same size (so raw truncates image to 
size and only that part is used by vfat like we have a larger disk with an 
MBR set to smaller size with unused space at the end) and when size is 
specified for vvfat it would error out if the underlying raw image does 
not match that size? Then no need for a separate option but I don't know 
if there's a problem with getting raw size from vvfat to check this or if 
that could be solved.

Regards,
BALATON Zoltan

>>> I'd also vote for mbr or similar shorter name; unpartitioned is
>>> awkward to type out in a command line. Maybe it can default to false
>>> for floppy and true for disk to preserve current behaviour but allow
>>> controlling it.
>>
>> I'm not a fan of conditional defaults, but I think it's better than a
>> nonsensical default that gets ignored.
>
> I think in this case a conditional default makes sense, not only for
> compatibility reasons. Hard disks almost always have a partition, floppy
> disks with partitions are basically unheard of.
>
> Kevin

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

[Kevin Wolf]

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

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[BALATON Zoltan]

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

On Mon, 10 Nov 2025, Kevin Wolf wrote:
> Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
>> Clément Chigot <chigot@adacore.com> writes:
>>
>>> On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>
>>>>> On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>
>>>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>>>
>>>>>>> This allows more flexibility to vvfat backend. The values of "Number of
>>>>>>> Heads" and "Sectors per track" are based on SD specifications Part 2.
>>>>>>>
>>>>>>> Due to the FAT architecture, not all sizes are reachable. Therefore, it
>>>>>>> could be round up to the closest available size.
>>>>>>>
>>>>>>> FAT32 has not been adjusted and thus still default to 504 Mib.
>>>>>>>
>>>>>>> For floppy, only 1440 Kib and 2880 Kib are supported.
>>>>>>>
>>>>>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>>>> index 8a479ba090..0bcb360320 100644
>>>>>>> --- a/qapi/block-core.json
>>>>>>> +++ b/qapi/block-core.json
>>>>>>> @@ -3478,11 +3478,17 @@
>>>>>>>  #     (default: true)
>>>>>>>  #     (since 10.2)
>>>>>>>  #
>>>>>>> +# @fat-size: size of the device in bytes.  Due to FAT underlying
>>>>>>> +#     architecture, this size can be rounded up to the closest valid
>>>>>>> +#     size.
>>>>>>> +#     (since 10.2)
>>>>>>> +#
>>>>>>
>>>>>> Can you explain again why you moved from @size to @fat-size?
>>>>>
>>>>> Just to be sure, you mean in the above comment, in the commit message or both ?
>>>>
>>>> Just to me, because I'm not sure I like the change, but that may well be
>>>> due to a lack of understanding of your reasons.
>>>
>>> Naming `fat-size` instead of `size` ensures the parameter is only
>>> recognized by the vvfat backend. In particular, it will be refused by
>>> the default raw format, avoiding confusion:
>>>  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
>>> truncated to 256M, raw format being implicit.
>>>  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
>>> unsupported by raw format.
>>
>> I figure throwing in format=raw to make raw format explicit doesn't
>> change anything.  Correct?
>>
>>>  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
>>>  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
>>> unsupported by vvfat format.
>>
>> If it was called @size, what behavior would we get?  Just two cases, I
>> think:
>>
>> 1. With raw format:
>>
>>     -drive file=fat:<path>,size=256M
>
> You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
> corrupted file system). It's quite easy to forget format=vvfat, so
> something that initially looks like it might be working is not a great
> result for this user error.

Why doesn't file=fat: imply format=vvfat? For what is the fat: part in 
file then? I currently recommend using:

-drive if=none,id=ufat,format=raw,file=fat:rw:/dir/to/export
-device usb-storage,drive=ufat

to my users which I got from somewhere but don't remember where and it 
seems to work but maybe not the best way to specify this. After reading 
this thread I'm confused about how to use this correctly. Is there some 
documentation on this? There only seems to be some mentions in 
docs/system/qemu-block-drivers.rst.inc but all of them using older 
options:

   |qemu_system| linux.img -hdb fat:/my_directory
   |qemu_system| linux.img -fda fat:floppy:/my_directory
   |qemu_system| linux.img -fda fat:floppy:rw:/my_directory

Regards,
BALATON Zoltan

>> 2. Without raw format:
>>
>>     -drive file=fat:<path>,format=vvfat,size=256M
>
> This does the thing that you actually want, a 256 MiB file system.
>
> I suggested to rename the vvfat option in v1 to make accidents at least
> a bit less likely. I'm not completely sure if "fat-size" is the best
> name, though, as it sounds as if it referred to the FAT itself instead
> of the FAT filesystem. Maybe "fs-size"?
>
> Kevin
>
>
>

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Kevin Wolf]

Am 10.11.2025 um 16:36 hat BALATON Zoltan geschrieben:
> On Mon, 10 Nov 2025, Kevin Wolf wrote:
> > Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
> > > Clément Chigot <chigot@adacore.com> writes:
> > > 
> > > > On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
> > > > > 
> > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > > 
> > > > > > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
> > > > > > > 
> > > > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > > > > 
> > > > > > > > This allows more flexibility to vvfat backend. The values of "Number of
> > > > > > > > Heads" and "Sectors per track" are based on SD specifications Part 2.
> > > > > > > > 
> > > > > > > > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> > > > > > > > could be round up to the closest available size.
> > > > > > > > 
> > > > > > > > FAT32 has not been adjusted and thus still default to 504 Mib.
> > > > > > > > 
> > > > > > > > For floppy, only 1440 Kib and 2880 Kib are supported.
> > > > > > > > 
> > > > > > > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > > > > index 8a479ba090..0bcb360320 100644
> > > > > > > > --- a/qapi/block-core.json
> > > > > > > > +++ b/qapi/block-core.json
> > > > > > > > @@ -3478,11 +3478,17 @@
> > > > > > > >  #     (default: true)
> > > > > > > >  #     (since 10.2)
> > > > > > > >  #
> > > > > > > > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> > > > > > > > +#     architecture, this size can be rounded up to the closest valid
> > > > > > > > +#     size.
> > > > > > > > +#     (since 10.2)
> > > > > > > > +#
> > > > > > > 
> > > > > > > Can you explain again why you moved from @size to @fat-size?
> > > > > > 
> > > > > > Just to be sure, you mean in the above comment, in the commit message or both ?
> > > > > 
> > > > > Just to me, because I'm not sure I like the change, but that may well be
> > > > > due to a lack of understanding of your reasons.
> > > > 
> > > > Naming `fat-size` instead of `size` ensures the parameter is only
> > > > recognized by the vvfat backend. In particular, it will be refused by
> > > > the default raw format, avoiding confusion:
> > > >  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
> > > > truncated to 256M, raw format being implicit.
> > > >  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
> > > > unsupported by raw format.
> > > 
> > > I figure throwing in format=raw to make raw format explicit doesn't
> > > change anything.  Correct?
> > > 
> > > >  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
> > > >  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
> > > > unsupported by vvfat format.
> > > 
> > > If it was called @size, what behavior would we get?  Just two cases, I
> > > think:
> > > 
> > > 1. With raw format:
> > > 
> > >     -drive file=fat:<path>,size=256M
> > 
> > You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
> > corrupted file system). It's quite easy to forget format=vvfat, so
> > something that initially looks like it might be working is not a great
> > result for this user error.
> 
> Why doesn't file=fat: imply format=vvfat? For what is the fat: part in
> file then?

-drive is built pretty much on the assumption that you have an image
format that runs on top of a protocol. Format probing probes the image
format, not the protocol, while prefixes like fat: (or nbd:, http: etc.)
specify the protocol.

So if you don't specify the format, we first open the protocol level
(which is vvfat) and then probing will detect that over this protocol,
we access a raw image. So it's mostly like saying format=raw.

I think that format=<protocol driver> works is really more accidental,
but we can't change it now (and probably also don't want to). It results
in opening only the protocol layer and not stacking any format driver on
top of it.

Options that you specify in -drive generally go to the top layer. So the
consequence in our case is that with format=vvfat, the option goes to
vvfat, but with format=raw (or unspecified format), it goes to the raw
forma driver.

> I currently recommend using:
> 
> -drive if=none,id=ufat,format=raw,file=fat:rw:/dir/to/export
> -device usb-storage,drive=ufat
> 
> to my users which I got from somewhere but don't remember where and it
> seems to work but maybe not the best way to specify this.

It's fine, and I might use the same one myself (though you should be
aware that fat:rw: is risky, it's full of bugs).

But if you add an option like size=64M, it goes to the raw driver, which
will take whatever image you access on the protocol level and truncate
it at 64 MiB.

If you want to give the size option on the vvfat level (and create a
filesystem that is actually only 64 MiB instead of truncating a larger
one), then obviously format=vvfat allows you to do that because then
there is no raw format layer to begin with. Or if you do have the raw
format layer, you can access options of the protocol layer by prefixing
"file.". So format=raw,file.size=64M would still pass the size option to
vvfat.

So the command line does allow you to get the option to the right place,
it's just very easy to get confused about this and to specify the option
for the wrong layer.

> After reading this thread I'm confused about how to use this
> correctly. Is there some documentation on this? There only seems to be
> some mentions in docs/system/qemu-block-drivers.rst.inc but all of
> them using older options:
> 
>   |qemu_system| linux.img -hdb fat:/my_directory
>   |qemu_system| linux.img -fda fat:floppy:/my_directory
>   |qemu_system| linux.img -fda fat:floppy:rw:/my_directory

All of those are honestly fine, too, if you're happy with the defaults
and don't want to set more advanced options.

I'll give you this bonus option if you want to be modern:

    -blockdev vvfat,node-name=ufat,dir=/my_directory,rw=on
    -device usb-storage,drive=ufat

But I don't think any of the other options is going away anytime soon.

Kevin

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[BALATON Zoltan]

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

On Mon, 10 Nov 2025, Kevin Wolf wrote:
> Am 10.11.2025 um 16:36 hat BALATON Zoltan geschrieben:
>> On Mon, 10 Nov 2025, Kevin Wolf wrote:
>>> Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>
>>>>> On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>
>>>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>>>
>>>>>>> On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>>>
>>>>>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>>>>>
>>>>>>>>> This allows more flexibility to vvfat backend. The values of "Number of
>>>>>>>>> Heads" and "Sectors per track" are based on SD specifications Part 2.
>>>>>>>>>
>>>>>>>>> Due to the FAT architecture, not all sizes are reachable. Therefore, it
>>>>>>>>> could be round up to the closest available size.
>>>>>>>>>
>>>>>>>>> FAT32 has not been adjusted and thus still default to 504 Mib.
>>>>>>>>>
>>>>>>>>> For floppy, only 1440 Kib and 2880 Kib are supported.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>>>>>> index 8a479ba090..0bcb360320 100644
>>>>>>>>> --- a/qapi/block-core.json
>>>>>>>>> +++ b/qapi/block-core.json
>>>>>>>>> @@ -3478,11 +3478,17 @@
>>>>>>>>>  #     (default: true)
>>>>>>>>>  #     (since 10.2)
>>>>>>>>>  #
>>>>>>>>> +# @fat-size: size of the device in bytes.  Due to FAT underlying
>>>>>>>>> +#     architecture, this size can be rounded up to the closest valid
>>>>>>>>> +#     size.
>>>>>>>>> +#     (since 10.2)
>>>>>>>>> +#
>>>>>>>>
>>>>>>>> Can you explain again why you moved from @size to @fat-size?
>>>>>>>
>>>>>>> Just to be sure, you mean in the above comment, in the commit message or both ?
>>>>>>
>>>>>> Just to me, because I'm not sure I like the change, but that may well be
>>>>>> due to a lack of understanding of your reasons.
>>>>>
>>>>> Naming `fat-size` instead of `size` ensures the parameter is only
>>>>> recognized by the vvfat backend. In particular, it will be refused by
>>>>> the default raw format, avoiding confusion:
>>>>>  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
>>>>> truncated to 256M, raw format being implicit.
>>>>>  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
>>>>> unsupported by raw format.
>>>>
>>>> I figure throwing in format=raw to make raw format explicit doesn't
>>>> change anything.  Correct?
>>>>
>>>>>  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
>>>>>  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
>>>>> unsupported by vvfat format.
>>>>
>>>> If it was called @size, what behavior would we get?  Just two cases, I
>>>> think:
>>>>
>>>> 1. With raw format:
>>>>
>>>>     -drive file=fat:<path>,size=256M
>>>
>>> You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
>>> corrupted file system). It's quite easy to forget format=vvfat, so
>>> something that initially looks like it might be working is not a great
>>> result for this user error.
>>
>> Why doesn't file=fat: imply format=vvfat? For what is the fat: part in
>> file then?
>
> -drive is built pretty much on the assumption that you have an image
> format that runs on top of a protocol. Format probing probes the image
> format, not the protocol, while prefixes like fat: (or nbd:, http: etc.)
> specify the protocol.

Thanks for clarifying. Is this described somewhere in the docs? The file 
formats are mentioned but the protocol and their relation to formats is 
not clearly described. Maybe it's worth clarifying that in the docs.

> So if you don't specify the format, we first open the protocol level
> (which is vvfat) and then probing will detect that over this protocol,
> we access a raw image. So it's mostly like saying format=raw.

I'm still not sure I completely get this but vvfat is not backed by a raw 
image but a host directory so why is the raw file format comes into play 
here at all? Shouldn't this always assume that for fat: the format is 
vvfat (or no format) and not a raw file? Then size property then would 
also work as expected.

> I think that format=<protocol driver> works is really more accidental,
> but we can't change it now (and probably also don't want to). It results
> in opening only the protocol layer and not stacking any format driver on
> top of it.

Does any format make sense for fat: at all considering the above that it 
does not access an image file with a format but a host directory? So maybe 
using and defaulting to format=raw is not quite right here?

> Options that you specify in -drive generally go to the top layer. So the
> consequence in our case is that with format=vvfat, the option goes to
> vvfat, but with format=raw (or unspecified format), it goes to the raw
> forma driver.

It would work if it would not default to format=raw for fat: which seems 
to make more sense to me than the current default but I don't know if it's 
easy or possible to change that.

>> I currently recommend using:
>>
>> -drive if=none,id=ufat,format=raw,file=fat:rw:/dir/to/export
>> -device usb-storage,drive=ufat
>>
>> to my users which I got from somewhere but don't remember where and it
>> seems to work but maybe not the best way to specify this.
>
> It's fine, and I might use the same one myself (though you should be
> aware that fat:rw: is risky, it's full of bugs).
>
> But if you add an option like size=64M, it goes to the raw driver, which
> will take whatever image you access on the protocol level and truncate
> it at 64 MiB.
>
> If you want to give the size option on the vvfat level (and create a
> filesystem that is actually only 64 MiB instead of truncating a larger
> one), then obviously format=vvfat allows you to do that because then
> there is no raw format layer to begin with. Or if you do have the raw
> format layer, you can access options of the protocol layer by prefixing
> "file.". So format=raw,file.size=64M would still pass the size option to
> vvfat.
>
> So the command line does allow you to get the option to the right place,
> it's just very easy to get confused about this and to specify the option
> for the wrong layer.

In that case maybe it's enough to give a warning in case we detect the 
wrong usage and point users to use the right way to specify size instead 
of adding another fat-size, fs-size or mbr-size option for this? 
Considering that format=vvfat,size=256M works I think either changing the 
default format for fat: or giving an error if fat: and size is specified 
without format=vvfat would solve this without a new option.

Regards,
BALATON Zoltan

>> After reading this thread I'm confused about how to use this
>> correctly. Is there some documentation on this? There only seems to be
>> some mentions in docs/system/qemu-block-drivers.rst.inc but all of
>> them using older options:
>>
>>   |qemu_system| linux.img -hdb fat:/my_directory
>>   |qemu_system| linux.img -fda fat:floppy:/my_directory
>>   |qemu_system| linux.img -fda fat:floppy:rw:/my_directory
>
> All of those are honestly fine, too, if you're happy with the defaults
> and don't want to set more advanced options.
>
> I'll give you this bonus option if you want to be modern:
>
>    -blockdev vvfat,node-name=ufat,dir=/my_directory,rw=on
>    -device usb-storage,drive=ufat
>
> But I don't think any of the other options is going away anytime soon.
>
> Kevin
>
>

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.11.2025 um 14:20 hat Markus Armbruster geschrieben:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> 
>> > On Mon, 10 Nov 2025, Clément Chigot wrote:
>> >> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>>
>> >>> Clément Chigot <chigot@adacore.com> writes:
>> >>>
>> >>>> This option tells whether a hard disk should be partitioned or not. It
>> >>>> defaults to true and have the prime effect of preventing a master boot
>> >>>> record (MBR) to be initialized.
>> >>>>
>> >>>> This is useful as some operating system (QNX, Rtems) don't
>> >>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
>> >>>>
>> >>>> Signed-off-by: Clément Chigot <chigot@adacore.com>

[...]

>> >>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
>> >>> table contraining a single partition partitioned?  Call it "mbr"?
>> >>
>> >> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
>> >> V1. Honestly I'm fine with both options:
>> >> - Technically, the option prevents MBR which has a side effect for
>> >> preventing partition tables
>> 
>> Yes, because the partition table is part of the MBR.  I'd rather name
>> the option after the entire thing it controls, not one of its parts.
>> 
>> >> - Even it has a single partition, I think it makes sense to call a
>> >> disk "partitioned" as long as it has a partition table
>> >>
>> >> But I'm not that familiar with disk formats, etc. I'll let you decide
>> >> with Kevin, which one you prefer.
>> 
>> Kevin is the maintainer, I just serve as advisor here.
>
> I figured that the meaning of "partitioned" is easier to understand for
> a casual user than having or not having an MBR ("I don't want to boot
> from this disk, why would I care about a boot record?").

Fair point.

Possible counter-points:

* The default is almost always right for the casual user.  The
  exception, as far as I understand, is certain guest OSes refuse to
  play ball with certain devices when they have an MBR.

* The configuration interface isn't exactly casual-user-friendly to
  begin with.  @fat-type, what's that, and why do I care?  @floppy,
  what's that, and why do I care?

Anyway, you decide.

> But if people think that "mbr" is better, that's fine with me.
>
> The only thing I really didn't want is the negative "no-mbr" and the
> double negation in "no-mbr=off" that comes with it.

Yes, negative names should definitely be avoided for boolean options.

[...]

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
>> Clément Chigot <chigot@adacore.com> writes:
>> 
>> > On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Clément Chigot <chigot@adacore.com> writes:
>> >>
>> >> > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >> >>
>> >> >> Clément Chigot <chigot@adacore.com> writes:
>> >> >>
>> >> >> > This allows more flexibility to vvfat backend. The values of "Number of
>> >> >> > Heads" and "Sectors per track" are based on SD specifications Part 2.
>> >> >> >
>> >> >> > Due to the FAT architecture, not all sizes are reachable. Therefore, it
>> >> >> > could be round up to the closest available size.
>> >> >> >
>> >> >> > FAT32 has not been adjusted and thus still default to 504 Mib.
>> >> >> >
>> >> >> > For floppy, only 1440 Kib and 2880 Kib are supported.
>> >> >> >
>> >> >> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> > index 8a479ba090..0bcb360320 100644
>> >> >> > --- a/qapi/block-core.json
>> >> >> > +++ b/qapi/block-core.json
>> >> >> > @@ -3478,11 +3478,17 @@
>> >> >> >  #     (default: true)
>> >> >> >  #     (since 10.2)
>> >> >> >  #
>> >> >> > +# @fat-size: size of the device in bytes.  Due to FAT underlying
>> >> >> > +#     architecture, this size can be rounded up to the closest valid
>> >> >> > +#     size.
>> >> >> > +#     (since 10.2)
>> >> >> > +#
>> >> >>
>> >> >> Can you explain again why you moved from @size to @fat-size?
>> >> >
>> >> > Just to be sure, you mean in the above comment, in the commit message or both ?
>> >>
>> >> Just to me, because I'm not sure I like the change, but that may well be
>> >> due to a lack of understanding of your reasons.
>> >
>> > Naming `fat-size` instead of `size` ensures the parameter is only
>> > recognized by the vvfat backend. In particular, it will be refused by
>> > the default raw format, avoiding confusion:
>> >  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
>> > truncated to 256M, raw format being implicit.
>> >  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
>> > unsupported by raw format.
>> 
>> I figure throwing in format=raw to make raw format explicit doesn't
>> change anything.  Correct?
>> 
>> >  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
>> >  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
>> > unsupported by vvfat format.
>> 
>> If it was called @size, what behavior would we get?  Just two cases, I
>> think:
>> 
>> 1. With raw format:
>> 
>>     -drive file=fat:<path>,size=256M
>
> You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
> corrupted file system). It's quite easy to forget format=vvfat, so
> something that initially looks like it might be working is not a great
> result for this user error.
>
>> 2. Without raw format:
>> 
>>     -drive file=fat:<path>,format=vvfat,size=256M
>
> This does the thing that you actually want, a 256 MiB file system.
>
> I suggested to rename the vvfat option in v1 to make accidents at least
> a bit less likely.

Valid point.

The "raw" format's slicing feature has dangerous sharp edges.

I'm all for with giving users poweful tools, even if they're dangerous.
However, as we can see here, this one can interact badly with the
implicit use of "raw".  Adding the slicing feature to "raw" may have
been a mistake, and naming one of its configuration options "size"
definitely was a mistake.  Something like @slice-offset and @slize-size
would've been safer.

Anyway, the interface is set in stone now.

>                    I'm not completely sure if "fat-size" is the best
> name, though, as it sounds as if it referred to the FAT itself instead
> of the FAT filesystem. Maybe "fs-size"?

Better.  It's the size of the image, though, not the size of the
filesystem.  They are the same only if there's no MBR.

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

[Markus Armbruster]

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.

[...]

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

[Markus Armbruster]

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>

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Clément Chigot]

On Mon, Nov 10, 2025 at 5:31 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 10.11.2025 um 16:36 hat BALATON Zoltan geschrieben:
> > On Mon, 10 Nov 2025, Kevin Wolf wrote:
> > > Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
> > > > Clément Chigot <chigot@adacore.com> writes:
> > > >
> > > > > On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
> > > > > >
> > > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > > >
> > > > > > > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
> > > > > > > >
> > > > > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > > > > >
> > > > > > > > > This allows more flexibility to vvfat backend. The values of "Number of
> > > > > > > > > Heads" and "Sectors per track" are based on SD specifications Part 2.
> > > > > > > > >
> > > > > > > > > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> > > > > > > > > could be round up to the closest available size.
> > > > > > > > >
> > > > > > > > > FAT32 has not been adjusted and thus still default to 504 Mib.
> > > > > > > > >
> > > > > > > > > For floppy, only 1440 Kib and 2880 Kib are supported.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > > > > > index 8a479ba090..0bcb360320 100644
> > > > > > > > > --- a/qapi/block-core.json
> > > > > > > > > +++ b/qapi/block-core.json
> > > > > > > > > @@ -3478,11 +3478,17 @@
> > > > > > > > >  #     (default: true)
> > > > > > > > >  #     (since 10.2)
> > > > > > > > >  #
> > > > > > > > > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> > > > > > > > > +#     architecture, this size can be rounded up to the closest valid
> > > > > > > > > +#     size.
> > > > > > > > > +#     (since 10.2)
> > > > > > > > > +#
> > > > > > > >
> > > > > > > > Can you explain again why you moved from @size to @fat-size?
> > > > > > >
> > > > > > > Just to be sure, you mean in the above comment, in the commit message or both ?
> > > > > >
> > > > > > Just to me, because I'm not sure I like the change, but that may well be
> > > > > > due to a lack of understanding of your reasons.
> > > > >
> > > > > Naming `fat-size` instead of `size` ensures the parameter is only
> > > > > recognized by the vvfat backend. In particular, it will be refused by
> > > > > the default raw format, avoiding confusion:
> > > > >  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
> > > > > truncated to 256M, raw format being implicit.
> > > > >  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
> > > > > unsupported by raw format.
> > > >
> > > > I figure throwing in format=raw to make raw format explicit doesn't
> > > > change anything.  Correct?
> > > >
> > > > >  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
> > > > >  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
> > > > > unsupported by vvfat format.
> > > >
> > > > If it was called @size, what behavior would we get?  Just two cases, I
> > > > think:
> > > >
> > > > 1. With raw format:
> > > >
> > > >     -drive file=fat:<path>,size=256M
> > >
> > > You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
> > > corrupted file system). It's quite easy to forget format=vvfat, so
> > > something that initially looks like it might be working is not a great
> > > result for this user error.
> >
> > Why doesn't file=fat: imply format=vvfat? For what is the fat: part in
> > file then?
>
> -drive is built pretty much on the assumption that you have an image
> format that runs on top of a protocol. Format probing probes the image
> format, not the protocol, while prefixes like fat: (or nbd:, http: etc.)
> specify the protocol.
>
> So if you don't specify the format, we first open the protocol level
> (which is vvfat) and then probing will detect that over this protocol,
> we access a raw image. So it's mostly like saying format=raw.
>
> I think that format=<protocol driver> works is really more accidental,
> but we can't change it now (and probably also don't want to). It results
> in opening only the protocol layer and not stacking any format driver on
> top of it.
>
> Options that you specify in -drive generally go to the top layer. So the
> consequence in our case is that with format=vvfat, the option goes to
> vvfat, but with format=raw (or unspecified format), it goes to the raw
> forma driver.
>
> > I currently recommend using:
> >
> > -drive if=none,id=ufat,format=raw,file=fat:rw:/dir/to/export
> > -device usb-storage,drive=ufat
> >
> > to my users which I got from somewhere but don't remember where and it
> > seems to work but maybe not the best way to specify this.
>
> It's fine, and I might use the same one myself (though you should be
> aware that fat:rw: is risky, it's full of bugs).
>
> But if you add an option like size=64M, it goes to the raw driver, which
> will take whatever image you access on the protocol level and truncate
> it at 64 MiB.
>
> If you want to give the size option on the vvfat level (and create a
> filesystem that is actually only 64 MiB instead of truncating a larger
> one), then obviously format=vvfat allows you to do that because then
> there is no raw format layer to begin with. Or if you do have the raw
> format layer, you can access options of the protocol layer by prefixing
> "file.". So format=raw,file.size=64M would still pass the size option to
> vvfat.

How is `file.size` working ? I've tried a similar syntax for other
vvfat options (e.g `file.floppy` or the new `file.partitioned`) but
those have no effect. Is this because there are fetched within the
"filename"
Wondering because I'mn ot a fan of the new ":unpartitioned:", I've
added in patch 1. If it can simply be replaced by
`format=raw,file.partitioned=false` or
`format=vvfat,partitioned=false`. I think that would be far enough for
its purpose.

> So the command line does allow you to get the option to the right place,
> it's just very easy to get confused about this and to specify the option
> for the wrong layer.
>
> > After reading this thread I'm confused about how to use this
> > correctly. Is there some documentation on this? There only seems to be
> > some mentions in docs/system/qemu-block-drivers.rst.inc but all of
> > them using older options:
> >
> >   |qemu_system| linux.img -hdb fat:/my_directory
> >   |qemu_system| linux.img -fda fat:floppy:/my_directory
> >   |qemu_system| linux.img -fda fat:floppy:rw:/my_directory
>
> All of those are honestly fine, too, if you're happy with the defaults
> and don't want to set more advanced options.
>
> I'll give you this bonus option if you want to be modern:
>
>     -blockdev vvfat,node-name=ufat,dir=/my_directory,rw=on
>     -device usb-storage,drive=ufat
>
> But I don't think any of the other options is going away anytime soon.
>
> Kevin
>

Re: [PATCH v2 5/5] vvfat: add support for "fat-size" options

[Kevin Wolf]

Am 12.11.2025 um 10:50 hat Clément Chigot geschrieben:
> On Mon, Nov 10, 2025 at 5:31 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 10.11.2025 um 16:36 hat BALATON Zoltan geschrieben:
> > > On Mon, 10 Nov 2025, Kevin Wolf wrote:
> > > > Am 10.11.2025 um 14:42 hat Markus Armbruster geschrieben:
> > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > >
> > > > > > On Mon, Nov 10, 2025 at 2:09 PM Markus Armbruster <armbru@redhat.com> wrote:
> > > > > > >
> > > > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > > > >
> > > > > > > > On Mon, Nov 10, 2025 at 11:13 AM Markus Armbruster <armbru@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > Clément Chigot <chigot@adacore.com> writes:
> > > > > > > > >
> > > > > > > > > > This allows more flexibility to vvfat backend. The values of "Number of
> > > > > > > > > > Heads" and "Sectors per track" are based on SD specifications Part 2.
> > > > > > > > > >
> > > > > > > > > > Due to the FAT architecture, not all sizes are reachable. Therefore, it
> > > > > > > > > > could be round up to the closest available size.
> > > > > > > > > >
> > > > > > > > > > FAT32 has not been adjusted and thus still default to 504 Mib.
> > > > > > > > > >
> > > > > > > > > > For floppy, only 1440 Kib and 2880 Kib are supported.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > > > > > > index 8a479ba090..0bcb360320 100644
> > > > > > > > > > --- a/qapi/block-core.json
> > > > > > > > > > +++ b/qapi/block-core.json
> > > > > > > > > > @@ -3478,11 +3478,17 @@
> > > > > > > > > >  #     (default: true)
> > > > > > > > > >  #     (since 10.2)
> > > > > > > > > >  #
> > > > > > > > > > +# @fat-size: size of the device in bytes.  Due to FAT underlying
> > > > > > > > > > +#     architecture, this size can be rounded up to the closest valid
> > > > > > > > > > +#     size.
> > > > > > > > > > +#     (since 10.2)
> > > > > > > > > > +#
> > > > > > > > >
> > > > > > > > > Can you explain again why you moved from @size to @fat-size?
> > > > > > > >
> > > > > > > > Just to be sure, you mean in the above comment, in the commit message or both ?
> > > > > > >
> > > > > > > Just to me, because I'm not sure I like the change, but that may well be
> > > > > > > due to a lack of understanding of your reasons.
> > > > > >
> > > > > > Naming `fat-size` instead of `size` ensures the parameter is only
> > > > > > recognized by the vvfat backend. In particular, it will be refused by
> > > > > > the default raw format, avoiding confusion:
> > > > > >  "-drive file=fat:<path>,size=256M" results in a 504M FAT disk
> > > > > > truncated to 256M, raw format being implicit.
> > > > > >  "-drive file=fat:<path>,fat-size=256M" is refused. "fat-size" is
> > > > > > unsupported by raw format.
> > > > >
> > > > > I figure throwing in format=raw to make raw format explicit doesn't
> > > > > change anything.  Correct?
> > > > >
> > > > > >  "-drive file=fat:<path>,format=vvfat,fat-size=256M" results in a 256M FAT disk.
> > > > > >  "-drive file=fat:<path>,format=vvfat,size=256M" is refused. "size" is
> > > > > > unsupported by vvfat format.
> > > > >
> > > > > If it was called @size, what behavior would we get?  Just two cases, I
> > > > > think:
> > > > >
> > > > > 1. With raw format:
> > > > >
> > > > >     -drive file=fat:<path>,size=256M
> > > >
> > > > You'd silently get a 504 MiB filesystem truncated to 256 MiB (i.e. a
> > > > corrupted file system). It's quite easy to forget format=vvfat, so
> > > > something that initially looks like it might be working is not a great
> > > > result for this user error.
> > >
> > > Why doesn't file=fat: imply format=vvfat? For what is the fat: part in
> > > file then?
> >
> > -drive is built pretty much on the assumption that you have an image
> > format that runs on top of a protocol. Format probing probes the image
> > format, not the protocol, while prefixes like fat: (or nbd:, http: etc.)
> > specify the protocol.
> >
> > So if you don't specify the format, we first open the protocol level
> > (which is vvfat) and then probing will detect that over this protocol,
> > we access a raw image. So it's mostly like saying format=raw.
> >
> > I think that format=<protocol driver> works is really more accidental,
> > but we can't change it now (and probably also don't want to). It results
> > in opening only the protocol layer and not stacking any format driver on
> > top of it.
> >
> > Options that you specify in -drive generally go to the top layer. So the
> > consequence in our case is that with format=vvfat, the option goes to
> > vvfat, but with format=raw (or unspecified format), it goes to the raw
> > forma driver.
> >
> > > I currently recommend using:
> > >
> > > -drive if=none,id=ufat,format=raw,file=fat:rw:/dir/to/export
> > > -device usb-storage,drive=ufat
> > >
> > > to my users which I got from somewhere but don't remember where and it
> > > seems to work but maybe not the best way to specify this.
> >
> > It's fine, and I might use the same one myself (though you should be
> > aware that fat:rw: is risky, it's full of bugs).
> >
> > But if you add an option like size=64M, it goes to the raw driver, which
> > will take whatever image you access on the protocol level and truncate
> > it at 64 MiB.
> >
> > If you want to give the size option on the vvfat level (and create a
> > filesystem that is actually only 64 MiB instead of truncating a larger
> > one), then obviously format=vvfat allows you to do that because then
> > there is no raw format layer to begin with. Or if you do have the raw
> > format layer, you can access options of the protocol layer by prefixing
> > "file.". So format=raw,file.size=64M would still pass the size option to
> > vvfat.
> 
> How is `file.size` working ? I've tried a similar syntax for other
> vvfat options (e.g `file.floppy` or the new `file.partitioned`) but
> those have no effect. Is this because there are fetched within the
> "filename"
> Wondering because I'mn ot a fan of the new ":unpartitioned:", I've
> added in patch 1. If it can simply be replaced by
> `format=raw,file.partitioned=false` or
> `format=vvfat,partitioned=false`. I think that would be far enough for
> its purpose.

Yes, I think it's because vvfat_parse_filename() overwrites them
unconditionally while getting the options from the filename.

Kevin

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Clément Chigot]

On Tue, Nov 11, 2025 at 8:43 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 10.11.2025 um 14:20 hat Markus Armbruster geschrieben:
> >> BALATON Zoltan <balaton@eik.bme.hu> writes:
> >>
> >> > On Mon, 10 Nov 2025, Clément Chigot wrote:
> >> >> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>>
> >> >>> Clément Chigot <chigot@adacore.com> writes:
> >> >>>
> >> >>>> This option tells whether a hard disk should be partitioned or not. It
> >> >>>> defaults to true and have the prime effect of preventing a master boot
> >> >>>> record (MBR) to be initialized.
> >> >>>>
> >> >>>> This is useful as some operating system (QNX, Rtems) don't
> >> >>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
> >> >>>>
> >> >>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> [...]
>
> >> >>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
> >> >>> table contraining a single partition partitioned?  Call it "mbr"?
> >> >>
> >> >> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
> >> >> V1. Honestly I'm fine with both options:
> >> >> - Technically, the option prevents MBR which has a side effect for
> >> >> preventing partition tables
> >>
> >> Yes, because the partition table is part of the MBR.  I'd rather name
> >> the option after the entire thing it controls, not one of its parts.
> >>
> >> >> - Even it has a single partition, I think it makes sense to call a
> >> >> disk "partitioned" as long as it has a partition table
> >> >>
> >> >> But I'm not that familiar with disk formats, etc. I'll let you decide
> >> >> with Kevin, which one you prefer.
> >>
> >> Kevin is the maintainer, I just serve as advisor here.
> >
> > I figured that the meaning of "partitioned" is easier to understand for
> > a casual user than having or not having an MBR ("I don't want to boot
> > from this disk, why would I care about a boot record?").
>
> Fair point.
>
> Possible counter-points:
>
> * The default is almost always right for the casual user.  The
>   exception, as far as I understand, is certain guest OSes refuse to
>   play ball with certain devices when they have an MBR.
>
> * The configuration interface isn't exactly casual-user-friendly to
>   begin with.  @fat-type, what's that, and why do I care?  @floppy,
>   what's that, and why do I care?
>
> Anyway, you decide.

AFAICT, there are two open questions for that patch:

1. "mbr" vs "partitioned".
I do think "partitioned" is clearer, a bit more casual friendly. "mbr"
requires knowledge about FAT format, while what's a partition should
be known by a wider audience.
Side note, in V3, I'll remove the "unpartitioned" keyword to simply
replace it by "partitoned=false" (I wasn't aware such an obvious
possibility was working...). So we might even call it
"partition/partitions=true|false".

2. The default value. Should it be "false" for @floppy ?
IMO, having a default value independent of other arguments is always
better. Hence, I'll push for keeping "partitioned=true" as the
default, and having users forcing "partitioned=false" for floppy (an
error being raised otherwise). As we'll probably change the default
behavior with floppy anyway (cf patch 2), I don't think it will hurt a
lot to make users passing a new flag.

> > But if people think that "mbr" is better, that's fine with me.
> >
> > The only thing I really didn't want is the negative "no-mbr" and the
> > double negation in "no-mbr=off" that comes with it.
>
> Yes, negative names should definitely be avoided for boolean options.
>
> [...]
>

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[BALATON Zoltan]

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

On Fri, 14 Nov 2025, Clément Chigot wrote:
> On Tue, Nov 11, 2025 at 8:43 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 10.11.2025 um 14:20 hat Markus Armbruster geschrieben:
>>>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>>>
>>>>> On Mon, 10 Nov 2025, Clément Chigot wrote:
>>>>>> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>>
>>>>>>> Clément Chigot <chigot@adacore.com> writes:
>>>>>>>
>>>>>>>> This option tells whether a hard disk should be partitioned or not. It
>>>>>>>> defaults to true and have the prime effect of preventing a master boot
>>>>>>>> record (MBR) to be initialized.
>>>>>>>>
>>>>>>>> This is useful as some operating system (QNX, Rtems) don't
>>>>>>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
>>>>>>>>
>>>>>>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>
>> [...]
>>
>>>>>>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
>>>>>>> table contraining a single partition partitioned?  Call it "mbr"?
>>>>>>
>>>>>> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
>>>>>> V1. Honestly I'm fine with both options:
>>>>>> - Technically, the option prevents MBR which has a side effect for
>>>>>> preventing partition tables
>>>>
>>>> Yes, because the partition table is part of the MBR.  I'd rather name
>>>> the option after the entire thing it controls, not one of its parts.
>>>>
>>>>>> - Even it has a single partition, I think it makes sense to call a
>>>>>> disk "partitioned" as long as it has a partition table
>>>>>>
>>>>>> But I'm not that familiar with disk formats, etc. I'll let you decide
>>>>>> with Kevin, which one you prefer.
>>>>
>>>> Kevin is the maintainer, I just serve as advisor here.
>>>
>>> I figured that the meaning of "partitioned" is easier to understand for
>>> a casual user than having or not having an MBR ("I don't want to boot
>>> from this disk, why would I care about a boot record?").
>>
>> Fair point.
>>
>> Possible counter-points:
>>
>> * The default is almost always right for the casual user.  The
>>   exception, as far as I understand, is certain guest OSes refuse to
>>   play ball with certain devices when they have an MBR.
>>
>> * The configuration interface isn't exactly casual-user-friendly to
>>   begin with.  @fat-type, what's that, and why do I care?  @floppy,
>>   what's that, and why do I care?
>>
>> Anyway, you decide.
>
> AFAICT, there are two open questions for that patch:
>
> 1. "mbr" vs "partitioned".
> I do think "partitioned" is clearer, a bit more casual friendly. "mbr"
> requires knowledge about FAT format, while what's a partition should
> be known by a wider audience.
> Side note, in V3, I'll remove the "unpartitioned" keyword to simply
> replace it by "partitoned=false" (I wasn't aware such an obvious
> possibility was working...). So we might even call it
> "partition/partitions=true|false".
>
> 2. The default value. Should it be "false" for @floppy ?
> IMO, having a default value independent of other arguments is always
> better. Hence, I'll push for keeping "partitioned=true" as the
> default, and having users forcing "partitioned=false" for floppy (an
> error being raised otherwise). As we'll probably change the default
> behavior with floppy anyway (cf patch 2), I don't think it will hurt a
> lot to make users passing a new flag.

Combined with the option called partinioned=false that's quite unfriendly 
for users trying to type a command line. Maybe not many do but those who 
don't also don't care about what are the defaults or if it's called mbr or 
partitioned as whatever generates the command line for them takes care 
of that. So I'm still for user friendly CLI but I also don't care enough 
to insist more if others don't think it's worth to keep this user friendly 
for command line users.

There was another question if the fat-size option is really needed or it 
could just use size if the default format=raw was changed to behave like 
format=vvfat if file=fat: is given which I think would make more sense 
than only truncating the underlying raw format that's not even needed to 
be there but I don't know how difficult it is to implement this or the 
default format=raw is hard coded and hard to change for fat: protocol.

So in summary:

1. format=vvfat,size=xMB was said to work so could file=fat:/dir,size=xMB 
imply format=vvfat so it would also work? Then no other size option is 
needed.

2. Having different defaults for floppy or disk would keep existing 
command lines working. Otherwise why not make partitioned=false the 
default and let users who need it set explicitly. That would also work for 
most cases without having to type out this option.

Regards,
BALATON Zoltan

Re: [PATCH v2 1/5] vvfat: introduce partitioned option

[Clément Chigot]

On Fri, Nov 14, 2025 at 2:25 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Fri, 14 Nov 2025, Clément Chigot wrote:
> > On Tue, Nov 11, 2025 at 8:43 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >>> Am 10.11.2025 um 14:20 hat Markus Armbruster geschrieben:
> >>>> BALATON Zoltan <balaton@eik.bme.hu> writes:
> >>>>
> >>>>> On Mon, 10 Nov 2025, Clément Chigot wrote:
> >>>>>> On Mon, Nov 10, 2025 at 11:07 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>>>>>>
> >>>>>>> Clément Chigot <chigot@adacore.com> writes:
> >>>>>>>
> >>>>>>>> This option tells whether a hard disk should be partitioned or not. It
> >>>>>>>> defaults to true and have the prime effect of preventing a master boot
> >>>>>>>> record (MBR) to be initialized.
> >>>>>>>>
> >>>>>>>> This is useful as some operating system (QNX, Rtems) don't
> >>>>>>>> recognized FAT mounted disks (especially SD cards) if a MBR is present.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
> >>
> >> [...]
> >>
> >>>>>>> Not sure I like "partitioned".  Is a disk with an MBR and a partition
> >>>>>>> table contraining a single partition partitioned?  Call it "mbr"?
> >>>>>>
> >>>>>> It used to be called "mbr/no-mbr" but Kevin suggested renaming it in
> >>>>>> V1. Honestly I'm fine with both options:
> >>>>>> - Technically, the option prevents MBR which has a side effect for
> >>>>>> preventing partition tables
> >>>>
> >>>> Yes, because the partition table is part of the MBR.  I'd rather name
> >>>> the option after the entire thing it controls, not one of its parts.
> >>>>
> >>>>>> - Even it has a single partition, I think it makes sense to call a
> >>>>>> disk "partitioned" as long as it has a partition table
> >>>>>>
> >>>>>> But I'm not that familiar with disk formats, etc. I'll let you decide
> >>>>>> with Kevin, which one you prefer.
> >>>>
> >>>> Kevin is the maintainer, I just serve as advisor here.
> >>>
> >>> I figured that the meaning of "partitioned" is easier to understand for
> >>> a casual user than having or not having an MBR ("I don't want to boot
> >>> from this disk, why would I care about a boot record?").
> >>
> >> Fair point.
> >>
> >> Possible counter-points:
> >>
> >> * The default is almost always right for the casual user.  The
> >>   exception, as far as I understand, is certain guest OSes refuse to
> >>   play ball with certain devices when they have an MBR.
> >>
> >> * The configuration interface isn't exactly casual-user-friendly to
> >>   begin with.  @fat-type, what's that, and why do I care?  @floppy,
> >>   what's that, and why do I care?
> >>
> >> Anyway, you decide.
> >
> > AFAICT, there are two open questions for that patch:
> >
> > 1. "mbr" vs "partitioned".
> > I do think "partitioned" is clearer, a bit more casual friendly. "mbr"
> > requires knowledge about FAT format, while what's a partition should
> > be known by a wider audience.
> > Side note, in V3, I'll remove the "unpartitioned" keyword to simply
> > replace it by "partitoned=false" (I wasn't aware such an obvious
> > possibility was working...). So we might even call it
> > "partition/partitions=true|false".
> >
> > 2. The default value. Should it be "false" for @floppy ?
> > IMO, having a default value independent of other arguments is always
> > better. Hence, I'll push for keeping "partitioned=true" as the
> > default, and having users forcing "partitioned=false" for floppy (an
> > error being raised otherwise). As we'll probably change the default
> > behavior with floppy anyway (cf patch 2), I don't think it will hurt a
> > lot to make users passing a new flag.
>
> Combined with the option called partinioned=false that's quite unfriendly
> for users trying to type a command line. Maybe not many do but those who
> don't also don't care about what are the defaults or if it's called mbr or
> partitioned as whatever generates the command line for them takes care
> of that. So I'm still for user friendly CLI but I also don't care enough
> to insist more if others don't think it's worth to keep this user friendly
> for command line users.
>
> There was another question if the fat-size option is really needed or it
> could just use size if the default format=raw was changed to behave like
> format=vvfat if file=fat: is given which I think would make more sense
> than only truncating the underlying raw format that's not even needed to
> be there but I don't know how difficult it is to implement this or the
> default format=raw is hard coded and hard to change for fat: protocol.
>
>
> So in summary:
>
> 1. format=vvfat,size=xMB was said to work so could file=fat:/dir,size=xMB
> imply format=vvfat so it would also work? Then no other size option is
> needed.

Well, that discussion was related to patch 5 and my understanding is that:
 1. Having @format=raw,size=xMB forwarding the size to the underlying
VVFAT is not easily doable with our current block architecture.
 2. The @size option for format="raw" is misleading. It should have
been @sliced-size or something close to it. However, it's too late to
change it (or we need to deprecate it in a few releases, but then
outside the scope of this patch).
 3. We want to avoid confusing mistakes, such as forgetting
@format=vvfat and having @size then recognized by @format=raw (the
default). Naming the new option differently ensures a clear error.

Side note, I agree that @fat-size is confusing so I'll rename it @fs-size in V3.

> 2. Having different defaults for floppy or disk would keep existing
> command lines working. Otherwise why not make partitioned=false the
> default and let users who need it set explicitly. That would also work for
> most cases without having to type out this option.

Yes, I forgot about that one (though linked to patch 2). If we don't
change the default size of floppy, the existing command lines will
stay as is, hence introducing a new mandatory option is a bad idea.
Overall the tradeoff is "simple default CLI" vs "non-conditional
defaults". Both have pros and cons and I don't have a strong feeling
about which ones should be prefered. So, I'll let you, the
maintainers, decide which one is the best for QEMU, its block devices
and vvfat future ;)

> Regards,
> BALATON Zoltan