Re: [PATCH v3 2/4] fs: split rootargs into root and options

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 11/27/25 3:25 PM, Fabian Pflug wrote:
> The rootargs argument (root=/dev/etc rootwait) can be split into the
> root and options parts, which makes it easier to manipulate them both
> independently. This changes tries to be as backward compatible as
> possible and does not change the behaviour of the get functions, but
> introduces new ones to get both items seperatly.
> 
> Signed-off-by: Fabian Pflug <f.pflug@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

with below syntax error fixed. Pointed out some nitpicks while at it.

> ---
>  .../migration-guides/migration-master.rst     | 15 +++++
>  common/block.c                                | 44 ++++++++------
>  fs/9p/vfs_super.c                             |  6 +-
>  fs/fs.c                                       | 58 ++++++++++++++-----
>  fs/nfs.c                                      |  4 +-
>  fs/squashfs/squashfs.c                        | 13 +++--
>  fs/ubifs/ubifs.c                              | 12 ++--
>  include/block.h                               |  5 ++
>  include/bootargs.h                            |  2 +
>  include/fs.h                                  |  6 +-
>  10 files changed, 117 insertions(+), 48 deletions(-)
>  create mode 100644 Documentation/migration-guides/migration-master.rst
> 
> diff --git a/Documentation/migration-guides/migration-master.rst b/Documentation/migration-guides/migration-master.rst
> new file mode 100644
> index 0000000000..0d6cfcad57
> --- /dev/null
> +++ b/Documentation/migration-guides/migration-master.rst
> @@ -0,0 +1,15 @@
> +Filesystems
> +-----------
> +
> +{linux.bootargs} is replaced with "root={linux.bootargs.root} {linux.bootargs.rootopts}"
> +
> +The variable linux.bootargs has been replaced by the two variables
> +linux.bootargs.root and linux.bootargs.rootopts, splitting the previous bootargs
> +into three parts. A nonexistent fixed "root=", then the root filesystem and then
> +additional optional params for this particular filesystem.
> +
> +for example the previous
> +    linux.bootargs="root=/dev/nfs nfsroot=192.168.1.1:/rootfs"
> +becomes
> +    linux.bootargs.root="/dev/nfs"
> +    linux.bootargs.rootopts="nfsroot=192.168.1.1:/rootfs"

That will look odd rendered to HTML without extra ReST formatting.
For inline literal code (e.g., ``{linux.booargs}``) and for the code
blocks, use ::, e.g.:

for example the previous::

    linux.bootargs="root=/dev/nfs nfsroot=192.168.1.1:/rootfs"


> +char *cdev_get_linux_rootarg(const struct cdev *partcdev)
> +{
> +	int err;
> +	char* root = NULL;
> +	char* rootopts = NULL;

Asterisks next to identifier.

> +char *format_root_bootarg(const char *root_arg, char *root, char *rootopts) {

Newline before brace.

> -	str = dev_get_param(&fsdev->dev, "linux.bootargs");
> -	if (!str)
> -		return ERR_PTR(-ENOSYS);
> +	fsdev_get_linux_root_options(fsdev, &root, &rootopts);
>  
> -	return xstrdup(str);
> +	if (!root)
> +		return ERR_PTR(-EINVAL);

-ENOSYS would have been the better return type here, which is also what
we had before.

> -		str = cdev_get_linux_rootarg(fsdev->cdev);
> -		if (str)
> -			fsdev_set_linux_rootarg(fsdev, str);
> +		cdev_get_linux_root_and_opts(fsdev->cdev, &root, &rootopts);
> +		if (root) {
> +			fsdev_set_linux_root_options(fsdev, root, rootopts);
> +			free(root);
> +			if(rootopts)
> +				free(rootopts);

Coding style is spaces after if, but you can drop the check anyway, as
mentioned previously.

> +		}
>  	}

>  static int ubifs_probe(struct device *dev)
> diff --git a/include/block.h b/include/block.h
> index b277f590e4..ff18dcded0 100644
> --- a/include/block.h
> +++ b/include/block.h
> @@ -85,6 +85,7 @@ static inline int block_flush(struct block_device *blk)
>  #ifdef CONFIG_BLOCK
>  unsigned file_list_add_blockdevs(struct file_list *files);
>  char *cdev_get_linux_rootarg(const struct cdev *partcdev);
> +int cdev_get_linux_root_and_opts(const struct cdev *partcdev, char** root, char** rootopts);
>  #else
>  static inline unsigned file_list_add_blockdevs(struct file_list *files)
>  {
> @@ -94,6 +95,10 @@ static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev)
>  {
>  	return NULL;
>  }
> +static inline void cdev_get_linux_root_and_opts(const struct cdev *partcdev, char** root, char** rootopts)
> +{
> +	return -ENOSYS;
> +}

Here's the actual issue: a void function can't return a value. Would
break in CI for platforms without CONFIG_BLOCK.

Cheers,
Ahmad

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |