Re: [LTP] [PATCH 1/2] lib: tst_test: Add per filesystem mkfs and mount opts

[Petr Vorel <pvorel@suse.cz>]

Hi Cyril,

> This commit does:

> * Group the filesystem type, mkfs and mount options into a separate
>   structure

> * Add an array of these structures to be able to define per filesystem
>   mkfs and mount options

> The details on the usage should be hopefully clear from the
> documentation comments for the struct tst_test.

Nice cleanup!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW is this effort somehow related to Martin's suggestion to "combine
.all_filesystems + .needs_rofs together"? [1] Or you're still not convinced it'd
be useful?

Also, this reminds me our goal to implement minimal filesystem size required per
filesystem.

[1] https://lore.kernel.org/ltp/ad6558e0-e834-4b35-923a-7b519384f436@suse.cz/

...
> +/**
> + * struct tst_fs - A file system type, mkfs and mount options
> + *
> + * @fs_type A filesystem type to use.
nit: @type.

Unfortunately sphinx does not complain when building docs, but you can see
"undescribed" in the resulted html.

> + *
> + * @mkfs_opts: A NULL terminated array of options passed to mkfs in the case
> + *             of 'tst_test.format_device'. These options are passed to mkfs
> + *             before the device path.
> + *
> + * @mkfs_size_opt: An option passed to mkfs in the case of
> + *                 'tst_test.format_device'. The device size in blocks is
> + *                 passed to mkfs after the device path and can be used to
> + *                 limit the file system not to use the whole block device.
> + *
> + * @mnt_flags: MS_* flags passed to mount(2) when the test library mounts a
> + *             device in the case of 'tst_test.mount_device'.
> + *
> + * @mnt_data: The data passed to mount(2) when the test library mounts a device
> + *            in the case of 'tst_test.mount_device'.
> + */
> +struct tst_fs {
> +	const char *type;
> +
> +	const char *const *mkfs_opts;
> +	const char *mkfs_size_opt;
> +
> +	const unsigned int mnt_flags;
> +	const void *mnt_data;
> +};
...

> - * @dev_fs_type: If set overrides the default file system type for the device and
> - *               sets the tst_device.fs_type.
> + * @fs: If fs.type is set it overrides the default file system type for the
> + *      device. The rest of the parameters describe default parameters for
> + *      mkfs and mount.
>   *
> - * @dev_fs_opts: A NULL terminated array of options passed to mkfs in the case
> - *               of 'tst_test.format_device'. These options are passed to mkfs
> - *               before the device path.
> - *
> - * @dev_extra_opts: A NULL terminated array of extra options passed to mkfs in
> - *                  the case of 'tst_test.format_device'. Extra options are
> - *                  passed to mkfs after the device path. Commonly the option
> - *                  after mkfs is the number of blocks and can be used to limit
> - *                  the file system not to use the whole block device.
> + * @fss: A NULL type terminated array of per file system type options. If

Could this be fs_all (although it can be used also without all_filesystems just to describe more fs)?
Because fss looks confusing a bit (it looks as some abbreviation, thus I would
not match it to fs member).

> + *       tst_test.all_filesystems is not set the array describes a list of
> + *       file systems to test along with parameters to pass to mkfs and mount.

So that one can decide if use .fss instead of .all_filesystems and
.skip_filesystems? I like the flexibility, but it can leads to confusion.

Also it'd be like to 1) use fss in some test 2) have at least trivial library
test.

Also nit: writing it as @all_filesystems formats it red (I'd prefer to be a HTML
link to the member in docs, but no idea how to do it. @Andrea, any idea?)
> + *       If tst_test.all_filesystems is set the mkfs and mount options are
> + *       taken from tst_test.fs unless there is an override for a given
> + *       file system type defined in this array.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp