Re: [PATCH v2 10/11] sandbox: Find disk images in the persistent-data directory

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Simon,

Thank you for the patch.

On Sat, May 23, 2026 at 02:54, Simon Glass <sjg@chromium.org> wrote:

> The sandbox mmc, scsi and usb-flash drivers open their backing files
> from plat->pathname as given. Test images are about to move out of
> the source tree into the persistent-data directory, so the drivers
> need to find them there.
>
> Try the persistent-data directory first via os_persistent_file() and
> fall back to the original pathname when the lookup fails, so absolute
> paths and files already in the current directory keep working.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>  drivers/mmc/sandbox_mmc.c        | 13 +++++++++++--
>  drivers/scsi/sandbox_scsi.c      | 15 ++++++++++++---
>  drivers/usb/emul/sandbox_flash.c | 14 +++++++++++---
>  3 files changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c
> index a24520f2e78..42a80c7499c 100644
> --- a/drivers/mmc/sandbox_mmc.c
> +++ b/drivers/mmc/sandbox_mmc.c
> @@ -170,11 +170,20 @@ static int sandbox_mmc_probe(struct udevice *dev)
>  	int ret;
>  
>  	if (plat->fname) {
> -		ret = os_map_file(plat->fname, OS_O_RDWR | OS_O_CREAT,
> +		const char *fname = plat->fname;
> +		char buf[256];

Why 256? Can't we use a define/constant for this?
I see that fname ends up coming from ofnode_read_string() but it's
unclear to me if there is a build-time constant size we could use instead.

> +
> +		/*
> +		 * Try persistent data directory first, then fall back to the
> +		 * filename as given (for absolute paths or current directory)
> +		 */
> +		if (!os_persistent_file(buf, sizeof(buf), plat->fname))
> +			fname = buf;
> +		ret = os_map_file(fname, OS_O_RDWR | OS_O_CREAT,
>  				  (void **)&priv->buf, &priv->size);
>  		if (ret) {
>  			log_err("%s: Unable to map file '%s'\n", dev->name,
> -				plat->fname);
> +				fname);
>  			return ret;
>  		}
>  		priv->csize = priv->size / SIZE_MULTIPLE - 1;
> diff --git a/drivers/scsi/sandbox_scsi.c b/drivers/scsi/sandbox_scsi.c
> index 544a0247083..97afeddc2e9 100644
> --- a/drivers/scsi/sandbox_scsi.c
> +++ b/drivers/scsi/sandbox_scsi.c
> @@ -104,9 +104,18 @@ static int sandbox_scsi_probe(struct udevice *dev)
>  	info->block_size = SANDBOX_SCSI_BLOCK_LEN;
>  
>  	if (priv->pathname) {
> -		priv->fd = os_open(priv->pathname, OS_O_RDONLY);
> -		if (priv->fd != -1) {
> -			ret = os_get_filesize(priv->pathname, &info->file_size);
> +		const char *pathname = priv->pathname;
> +		char buf[256];
> +
> +		/*
> +		 * Try persistent data directory first, then fall back to the
> +		 * pathname as given (for absolute paths or current directory)
> +		 */
> +		if (!os_persistent_file(buf, sizeof(buf), priv->pathname))
> +			pathname = buf;
> +		priv->fd = os_open(pathname, OS_O_RDONLY);
> +		if (priv->fd >= 0) {

I've noticed that the error handling gets updated here (from != -1 to >=
0).
Any reason for doing that? If so, can you please mention it in
the commit message?

os_open()'s docstring seems to mention:

 * Return:	file descriptor, or -1 on error

> +			ret = os_get_filesize(pathname, &info->file_size);
>  			if (ret)
>  				return log_msg_ret("sz", ret);
>  		}
> diff --git a/drivers/usb/emul/sandbox_flash.c b/drivers/usb/emul/sandbox_flash.c
> index b5176bb30ce..82aa7062865 100644
> --- a/drivers/usb/emul/sandbox_flash.c
> +++ b/drivers/usb/emul/sandbox_flash.c
> @@ -339,11 +339,19 @@ static int sandbox_flash_probe(struct udevice *dev)
>  	struct sandbox_flash_plat *plat = dev_get_plat(dev);
>  	struct sandbox_flash_priv *priv = dev_get_priv(dev);
>  	struct scsi_emul_info *info = &priv->eminfo;
> +	const char *pathname = plat->pathname;
> +	char buf[256];
>  	int ret;
>  
> -	priv->fd = os_open(plat->pathname, OS_O_RDWR);
> -	if (priv->fd != -1) {
> -		ret = os_get_filesize(plat->pathname, &info->file_size);
> +	/*
> +	 * Try persistent data directory first, then fall back to the
> +	 * pathname as given (for absolute paths or current directory)
> +	 */
> +	if (!os_persistent_file(buf, sizeof(buf), plat->pathname))
> +		pathname = buf;
> +	priv->fd = os_open(pathname, OS_O_RDWR);
> +	if (priv->fd >= 0) {

Same here.

> +		ret = os_get_filesize(pathname, &info->file_size);
>  		if (ret)
>  			return log_msg_ret("sz", ret);
>  	}
> -- 
> 2.43.0