[PATCH 6/7] fs/uimagefs: Use is_tftp_fs() and cache_file() to ease TFTP workaround

[Stefan Lengfeld]

Hi Sascha,

On Wed, Jan 24, 2018 at 08:45:33AM +0100, Sascha Hauer wrote:
> We have helper functions now to ease file caching when a file
> is on TFTP. Use them.
> 
> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> ---
>  fs/uimagefs.c      | 33 ++++++++++-----------------------
>  include/uimagefs.h |  2 +-
>  2 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/uimagefs.c b/fs/uimagefs.c
> index c0c5750c2c..2607d285f1 100644
> --- a/fs/uimagefs.c
> +++ b/fs/uimagefs.c
> @@ -196,7 +196,6 @@ static void uimagefs_remove(struct device_d *dev)
>  {
>  	struct uimagefs_handle *priv = dev->priv;
>  	struct uimagefs_handle_data *d, *tmp;
> -	struct stat s;
>  
>  	list_for_each_entry_safe(d, tmp, &priv->list, list) {
>  		free(d->name);
> @@ -204,10 +203,11 @@ static void uimagefs_remove(struct device_d *dev)
>  		free(d);
>  	}
>  
> -	if (IS_BUILTIN(CONFIG_FS_TFTP) && !stat(priv->tmp, &s))
> -		unlink(priv->tmp);
> +	if (priv->copy) {
> +		unlink(priv->copy);
> +		free(priv->copy);
> +	}
>  
> -	free(priv->tmp);
>  	free(priv);
>  }
>  
> @@ -364,27 +364,18 @@ static int __uimage_open(struct uimagefs_handle *priv)
>  	size_t offset = 0;
>  	size_t data_offset = 0;
>  
> -again:
> +	if (is_tftp_fs(priv->filename)) {
> +		ret = cache_file(priv->filename, &priv->copy);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	fd = open(priv->filename, O_RDONLY);

Same pattern as in the previous patch, but slightly different code :-)

The open() call must use the newly created file in 'priv->copy' and not
open the non-seekable file in 'priv-filename' again (if the file was not
seekable of course). Maybe copy the contents of priv->filename to
priv->copy in the seekable case.

+1 for removing the backwards jumping goto-statement that hides this
   magic.

>  	if (fd < 0) {
>  		printf("could not open: %s\n", errno_str());
>  		return fd;

Just a note: Since the struct uimagefs_handle is caller allocated, the
caller takes care of freeing the priv->copy string with function
uimagefs_remove() in case of an error. So no leaking string here.


All in all. Nice cleanup work.

Kind regards,
Stefan

>  	}
>  
> -	/*
> -	 * Hack around tftp fs. We need lseek for uImage support, but
> -	 * this cannot be implemented in tftp fs, so we detect this
> -	 * and copy the file to ram if it fails
> -	 */
> -	if (IS_BUILTIN(CONFIG_FS_TFTP) && !can_lseek_backward(fd)) {
> -		close(fd);
> -		ret = copy_file(priv->filename, priv->tmp, 0);
> -		if (ret)
> -			return ret;
> -		priv->filename = priv->tmp;
> -		goto again;
> -	}
> -
>  	header = &priv->header;
>  
>  	ret = read(fd, header, sizeof(*header));
> @@ -514,10 +505,6 @@ static int uimagefs_probe(struct device_d *dev)
>  	priv->filename = fsdev->backingstore;
>  	dev_dbg(dev, "mount: %s\n", fsdev->backingstore);
>  
> -	if (IS_BUILTIN(CONFIG_FS_TFTP))
> -		priv->tmp = basprintf("/.uImage_tmp_%08x",
> -					crc32(0, fsdev->path, strlen(fsdev->path)));
> -
>  	ret = __uimage_open(priv);
>  	if (ret)
>  		goto err;
> diff --git a/include/uimagefs.h b/include/uimagefs.h
> index 81b32310ad..3f58589b73 100644
> --- a/include/uimagefs.h
> +++ b/include/uimagefs.h
> @@ -45,7 +45,7 @@ struct uimagefs_handle {
>  	struct image_header header;
>  	int nb_data_entries;
>  	char *filename;
> -	char *tmp;
> +	char *copy;
>  
>  	struct list_head list;
>  };
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
>