On Fri, Mar 06, 2026 at 12:08:11PM -0700, Simon Glass wrote: > From: Simon Glass <[email protected]> > > Create a class around mk_fs() (and later setup_image()) to handle the > common tasks of image creation. Many callers of fs_helper.mk_fs() > create their own scratch directories while users of > fs_helper.setup_image() rely on one being returned. Unify this by > adding 'srcdir' as a field while converting to a class. > > The class delegates to the existing mk_fs() function for the actual > filesystem creation, adding lifecycle management for scratch > directories and the image file. > > Signed-off-by: Simon Glass <[email protected]> > --- > > Changes in v3: > - Rewrite the commit message as suggested by Tom Rini
Thanks.
> - Make the class a thin wrapper that delegates to mk_fs()
Oh, this does make it much easier to review, thanks!
[snip]
> + Args:
> + config (u_boot_config): U-Boot configuration
> + fs_type (str): File system type: one of ext2, ext3, ext4, vfat,
> + fat12, fat16, fat32, exfat, fs_generic (which means vfat)
> + size_mb (int): Size of file system in MB
> + prefix (str): Prefix string of volume's file name
> + """
> + if ('fat' not in fs_type and 'ext' not in fs_type and
> + fs_type not in ['exfat', 'fs_generic']):
> + raise ValueError(f"Unsupported filesystem type '{fs_type}'")
I don't think this works as expected. The first thing that caught my eye
is that shouldn't '"fat' not in fs_type" see exfat as valid? But we now
also pass along a bunch of nonsense like ext5 or fat128 or fatnotreal to
mkfs.notreal to fail on. We did zero sanity checks before, which is bad.
But maybe we should just add this as an explicit:
if (fs_type not in ['fat12', 'fat16', ...]):
raise ...
?
Otherwise looks good.
--
Tom
signature.asc
Description: PGP signature
