mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Stefan Lengfeld <contact@stefanchrist.eu>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Barebox List <barebox@lists.infradead.org>
Subject: Re: [PATCH 6/7] fs/uimagefs: Use is_tftp_fs() and cache_file() to ease TFTP workaround
Date: Wed, 24 Jan 2018 20:56:17 +0100	[thread overview]
Message-ID: <20180124195617.GD521@porty> (raw)
In-Reply-To: <20180124074534.7966-7-s.hauer@pengutronix.de>

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@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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2018-01-24 19:56 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-24  7:45 [PATCH 0/7] Make TFTP detection more convenient Sascha Hauer
2018-01-24  7:45 ` [PATCH 1/7] startup: create /tmp Sascha Hauer
2018-01-24  7:45 ` [PATCH 2/7] fs: implement is_tftp_fs() Sascha Hauer
2018-01-24 19:12   ` Stefan Lengfeld
2018-01-25  7:46     ` Sascha Hauer
2018-01-24  7:45 ` [PATCH 3/7] libfile: implement make_temp Sascha Hauer
2018-01-24  7:45 ` [PATCH 4/7] libfile: implement a function to cache a file Sascha Hauer
2018-01-24 19:23   ` Stefan Lengfeld
2018-01-24  7:45 ` [PATCH 5/7] uimage: Use is_tftp_fs() and cache_file() to ease TFTP workaround Sascha Hauer
2018-01-24 19:39   ` Stefan Lengfeld
2018-01-24  7:45 ` [PATCH 6/7] fs/uimagefs: " Sascha Hauer
2018-01-24 19:56   ` Stefan Lengfeld [this message]
2018-01-24  7:45 ` [PATCH 7/7] fs: remove now unused function can_lseek_backward() Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180124195617.GD521@porty \
    --to=contact@stefanchrist.eu \
    --cc=barebox@lists.infradead.org \
    --cc=s.hauer@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox