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

[Kevin Wolf <kwolf@redhat.com>]

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