[PATCH 0/7] Make TFTP detection more convenient

[PATCH 0/7] Make TFTP detection more convenient

[Sascha Hauer]

We can't lseek on files provided by TFTP, so some code copies the file
to operate on from TFTP to RAM before continuing. Make this a bit easier
by providing some helper code. We now create /tmp/, provide make_temp()
to create temporary files and also a single function to create a copy
of a file in /tmp/.

Sascha Hauer (7):
  startup: create /tmp
  fs: implement is_tftp_fs()
  libfile: implement make_temp
  libfile: implement a function to cache a file
  uimage: Use is_tftp_fs() and cache_file() to ease TFTP workaround
  fs/uimagefs: Use is_tftp_fs() and cache_file() to ease TFTP workaround
  fs: remove now unused function can_lseek_backward()

 common/startup.c   |  1 +
 common/uimage.c    | 47 ++++++++++++++++++++++-------------------------
 fs/fs.c            | 22 ++++++++++++++++++++++
 fs/uimagefs.c      | 33 ++++++++++-----------------------
 include/fs.h       | 20 +++++---------------
 include/image.h    |  1 +
 include/libfile.h  |  4 ++++
 include/uimagefs.h |  2 +-
 lib/libfile.c      | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 120 insertions(+), 64 deletions(-)

-- 
2.11.0


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

[PATCH 1/7] startup: create /tmp

[Sascha Hauer]

Some commands create temporary files in /. Create /tmp to offer
these commands an appropriate place for storing temporary files.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/startup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/startup.c b/common/startup.c
index 432be67cd6..8553849cb3 100644
--- a/common/startup.c
+++ b/common/startup.c
@@ -54,6 +54,7 @@ static int mount_root(void)
 {
 	mount("none", "ramfs", "/", NULL);
 	mkdir("/dev", 0);
+	mkdir("/tmp", 0);
 	mount("none", "devfs", "/dev", NULL);
 
 	if (IS_ENABLED(CONFIG_FS_EFIVARFS)) {
-- 
2.11.0


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

[PATCH 2/7] fs: implement is_tftp_fs()

[Sascha Hauer]

Some commands need files in which they can lseek backwards which
is particularly not possible on TFTP. Instead of hiding this
behind can_lseek_backward() create a function for it which tests
if the file is on TFTP directly rather than using different
lseek operations.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/fs.c      | 22 ++++++++++++++++++++++
 include/fs.h | 10 ++++++++++
 2 files changed, 32 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index 6f15e93ba9..e073e3577d 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -1906,3 +1906,25 @@ char *path_get_linux_rootarg(const char *path)
 
 	return xstrdup(str);
 }
+
+/**
+ * __is_tftp_fs() - return true when path is mounted on TFTP
+ * @path: The path
+ *
+ * Do not use directly, use is_tftp_fs instead.
+ *
+ * Return: true when @path is on TFTP, false otherwise
+ */
+bool __is_tftp_fs(const char *path)
+{
+	struct fs_device_d *fsdev;
+
+	fsdev = get_fsdevice_by_path(path);
+	if (!fsdev)
+		return false;
+
+	if (strcmp(fsdev->driver->drv.name, "tftp"))
+		return false;
+
+	return true;
+}
diff --git a/include/fs.h b/include/fs.h
index 3d88bfad4a..5a50d9b9e4 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -121,6 +121,16 @@ static inline int can_lseek_backward(int fd)
 	return 1;
 }
 
+bool __is_tftp_fs(const char *path);
+
+static inline bool is_tftp_fs(const char *path)
+{
+	if (!IS_ENABLED(CONFIG_FS_TFTP))
+		return 0;
+
+	return __is_tftp_fs(path);
+}
+
 #define drv_to_fs_driver(d) container_of(d, struct fs_driver_d, drv)
 
 int flush(int fd);
-- 
2.11.0


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

[PATCH 3/7] libfile: implement make_temp

[Sascha Hauer]

Create a make_temp() function which creates a name for a temporary file.
Since we do not have any concurrency in barebox we do not need to create
the file right away and can leave that to the caller. Unlike unix
mktemp the resulting filename is dynamically allocated and must be
freed by the caller.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/libfile.h |  2 ++
 lib/libfile.c     | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/libfile.h b/include/libfile.h
index dd0b00f988..6dbb81a241 100644
--- a/include/libfile.h
+++ b/include/libfile.h
@@ -26,4 +26,6 @@ int make_directory(const char *pathname);
 
 int unlink_recursive(const char *path, char **failedpath);
 
+char *make_temp(const char *template);
+
 #endif /* __LIBFILE_H */
diff --git a/lib/libfile.c b/lib/libfile.c
index 6b70306dbd..79054eb5ac 100644
--- a/lib/libfile.c
+++ b/lib/libfile.c
@@ -20,6 +20,7 @@
 #include <malloc.h>
 #include <libfile.h>
 #include <progress.h>
+#include <stdlib.h>
 #include <linux/stat.h>
 
 /*
@@ -485,3 +486,29 @@ int open_and_lseek(const char *filename, int mode, loff_t pos)
 
 	return fd;
 }
+
+/**
+ * make_temp - create a name for a temporary file
+ * @template:	The filename prefix
+ *
+ * This function creates a name for a temporary file. @template is used as a
+ * template for the name which gets appended a 8-digit hexadecimal number to
+ * create a unique filename.
+ *
+ * Return: This function returns a filename which can be used as a temporary
+ *         file lateron. The returned filename must be freed by the caller.
+ */
+char *make_temp(const char *template)
+{
+	char *name = NULL;
+	struct stat s;
+	int ret;
+
+	do {
+		free(name);
+		name = basprintf("/tmp/%s-%08x", template, random32());
+		ret = stat(name, &s);
+	} while (!ret);
+
+	return name;
+}
-- 
2.11.0


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

[PATCH 4/7] libfile: implement a function to cache a file

[Sascha Hauer]

Due to the nature of TFTP which can't lseek and due to the silliness
of our filesystem implementation which can't cache accesses we have to
manually cache files on TFTP filesystems sometimes. Make it easier
for them by providing a cache_file() function which copies the file
from TFTP to RAM.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/libfile.h |  2 ++
 lib/libfile.c     | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/libfile.h b/include/libfile.h
index 6dbb81a241..beec7cff79 100644
--- a/include/libfile.h
+++ b/include/libfile.h
@@ -28,4 +28,6 @@ int unlink_recursive(const char *path, char **failedpath);
 
 char *make_temp(const char *template);
 
+int cache_file(const char *path, char **newpath);
+
 #endif /* __LIBFILE_H */
diff --git a/lib/libfile.c b/lib/libfile.c
index 79054eb5ac..738ff1287d 100644
--- a/lib/libfile.c
+++ b/lib/libfile.c
@@ -512,3 +512,30 @@ char *make_temp(const char *template)
 
 	return name;
 }
+
+/**
+ * cache_file - Cache a file in /tmp
+ * @path:	The file to cache
+ * @newpath:	The return path where the file is copied to
+ *
+ * This function copies a given file to /tmp and returns its name in @newpath.
+ *
+ * Return: 0 for success, negative error code otherwise.
+ */
+int cache_file(const char *path, char **newpath)
+{
+	char *npath;
+	int ret;
+
+	npath = make_temp("filecache-");
+
+	ret = copy_file(path, npath, 0);
+	if (ret) {
+		free(npath);
+		return ret;
+	}
+
+	*newpath = npath;
+
+	return 0;
+}
-- 
2.11.0


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

[PATCH 5/7] uimage: Use is_tftp_fs() and cache_file() to ease TFTP workaround

[Sascha Hauer]

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>
---
 common/uimage.c | 47 ++++++++++++++++++++++-------------------------
 include/image.h |  1 +
 2 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/common/uimage.c b/common/uimage.c
index b6f0f109ca..18bc2e5d7b 100644
--- a/common/uimage.c
+++ b/common/uimage.c
@@ -85,8 +85,6 @@ ssize_t uimage_get_size(struct uimage_handle *handle, unsigned int image_no)
 }
 EXPORT_SYMBOL(uimage_get_size);
 
-static const char uimage_tmp[] = "/.uImage_tmp";
-
 /*
  * open a uimage. This will check the header contents and
  * return a handle to the uImage
@@ -99,32 +97,26 @@ struct uimage_handle *uimage_open(const char *filename)
 	struct image_header *header;
 	int i;
 	int ret;
-	struct stat s;
+	char *copy = NULL;
+
+	if (is_tftp_fs(filename)) {
+		ret = cache_file(filename, &copy);
+		if (ret)
+			return NULL;
+		filename = copy;
+	}
 
-again:
 	fd = open(filename, O_RDONLY);
 	if (fd < 0) {
 		printf("could not open: %s\n", errno_str());
 		return NULL;
 	}
 
-	/*
-	 * 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(filename, uimage_tmp, 0);
-		if (ret)
-			return NULL;
-		filename = uimage_tmp;
-		goto again;
-	}
-
 	handle = xzalloc(sizeof(struct uimage_handle));
 	header = &handle->header;
 
+	handle->copy = copy;
+
 	if (read(fd, header, sizeof(*header)) < 0) {
 		printf("could not read: %s\n", errno_str());
 		goto err_out;
@@ -202,9 +194,13 @@ again:
 	return handle;
 err_out:
 	close(fd);
+
+	if (handle->copy) {
+		unlink(handle->copy);
+		free(handle->copy);
+	}
 	free(handle);
-	if (IS_BUILTIN(CONFIG_FS_TFTP) && !stat(uimage_tmp, &s))
-		unlink(uimage_tmp);
+
 	return NULL;
 }
 EXPORT_SYMBOL(uimage_open);
@@ -214,14 +210,15 @@ EXPORT_SYMBOL(uimage_open);
  */
 void uimage_close(struct uimage_handle *handle)
 {
-	struct stat s;
-
 	close(handle->fd);
+
+	if (handle->copy) {
+		unlink(handle->copy);
+		free(handle->copy);
+	}
+
 	free(handle->name);
 	free(handle);
-
-	if (IS_BUILTIN(CONFIG_FS_TFTP) && !stat(uimage_tmp, &s))
-		unlink(uimage_tmp);
 }
 EXPORT_SYMBOL(uimage_close);
 
diff --git a/include/image.h b/include/image.h
index 3e75d49b88..add9c85874 100644
--- a/include/image.h
+++ b/include/image.h
@@ -246,6 +246,7 @@ struct resource *file_to_sdram(const char *filename, unsigned long adr);
 struct uimage_handle {
 	struct image_header header;
 	char *name;
+	char *copy;
 	struct uimage_handle_data ihd[MAX_MULTI_IMAGE_COUNT];
 	int nb_data_entries;
 	size_t data_offset;
-- 
2.11.0


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

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

[Sascha Hauer]

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);
 	if (fd < 0) {
 		printf("could not open: %s\n", errno_str());
 		return fd;
 	}
 
-	/*
-	 * 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

[PATCH 7/7] fs: remove now unused function can_lseek_backward()

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/fs.h | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/include/fs.h b/include/fs.h
index 5a50d9b9e4..2d08add01a 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -101,26 +101,6 @@ struct fs_device_d {
 	char *linux_rootarg;
 };
 
-/*
- * Some filesystems i.e. tftpfs only support lseek into one direction.
- * To detect this limited functionality we add this extra function.
- * Additionaly we also return 0 if we even can not seek forward.
- */
-static inline int can_lseek_backward(int fd)
-{
-	int ret;
-
-	ret = lseek(fd, 1, SEEK_SET);
-	if (ret < 0)
-		return 0;
-
-	ret = lseek(fd, 0, SEEK_SET);
-	if (ret < 0)
-		return 0;
-
-	return 1;
-}
-
 bool __is_tftp_fs(const char *path);
 
 static inline bool is_tftp_fs(const char *path)
-- 
2.11.0


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

Re: [PATCH 2/7] fs: implement is_tftp_fs()

[Stefan Lengfeld]

Hi Sascha,

On Wed, Jan 24, 2018 at 08:45:29AM +0100, Sascha Hauer wrote:
> Some commands need files in which they can lseek backwards which
> is particularly not possible on TFTP. Instead of hiding this
> behind can_lseek_backward() create a function for it which tests
> if the file is on TFTP directly rather than using different
> lseek operations.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  fs/fs.c      | 22 ++++++++++++++++++++++
>  include/fs.h | 10 ++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/fs/fs.c b/fs/fs.c
> index 6f15e93ba9..e073e3577d 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -1906,3 +1906,25 @@ char *path_get_linux_rootarg(const char *path)
>  
>  	return xstrdup(str);
>  }
> +
> +/**
> + * __is_tftp_fs() - return true when path is mounted on TFTP
> + * @path: The path
> + *
> + * Do not use directly, use is_tftp_fs instead.
> + *
> + * Return: true when @path is on TFTP, false otherwise
> + */
> +bool __is_tftp_fs(const char *path)
> +{
> +	struct fs_device_d *fsdev;
> +
> +	fsdev = get_fsdevice_by_path(path);
> +	if (!fsdev)
> +		return false;
> +
> +	if (strcmp(fsdev->driver->drv.name, "tftp"))
> +		return false;
> +
> +	return true;
> +}
> diff --git a/include/fs.h b/include/fs.h
> index 3d88bfad4a..5a50d9b9e4 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -121,6 +121,16 @@ static inline int can_lseek_backward(int fd)
>  	return 1;
>  }
>  
> +bool __is_tftp_fs(const char *path);
> +
> +static inline bool is_tftp_fs(const char *path)
> +{
> +	if (!IS_ENABLED(CONFIG_FS_TFTP))
> +		return 0;

Nitpick: Maybe use 'false' instead of '0', because return value is a
boolean.

Kind regards,
Stefan

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

Re: [PATCH 4/7] libfile: implement a function to cache a file

[Stefan Lengfeld]

Hi Sascha,

On Wed, Jan 24, 2018 at 08:45:31AM +0100, Sascha Hauer wrote:
> Due to the nature of TFTP which can't lseek and due to the silliness
> of our filesystem implementation which can't cache accesses we have to
> manually cache files on TFTP filesystems sometimes. Make it easier
> for them by providing a cache_file() function which copies the file
> from TFTP to RAM.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  include/libfile.h |  2 ++
>  lib/libfile.c     | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/include/libfile.h b/include/libfile.h
> index 6dbb81a241..beec7cff79 100644
> --- a/include/libfile.h
> +++ b/include/libfile.h
> @@ -28,4 +28,6 @@ int unlink_recursive(const char *path, char **failedpath);
>  
>  char *make_temp(const char *template);
>  
> +int cache_file(const char *path, char **newpath);
> +
>  #endif /* __LIBFILE_H */
> diff --git a/lib/libfile.c b/lib/libfile.c
> index 79054eb5ac..738ff1287d 100644
> --- a/lib/libfile.c
> +++ b/lib/libfile.c
> @@ -512,3 +512,30 @@ char *make_temp(const char *template)
>  
>  	return name;
>  }
> +
> +/**
> + * cache_file - Cache a file in /tmp
> + * @path:	The file to cache
> + * @newpath:	The return path where the file is copied to
> + *
> + * This function copies a given file to /tmp and returns its name in @newpath.

Maybe add an additional note here that the caller is responsible for
freeing the string returned in @newpath when the function exists
successfully. The string is allocated by the make_temp() function.

> + *
> + * Return: 0 for success, negative error code otherwise.
> + */
> +int cache_file(const char *path, char **newpath)
> +{
> +	char *npath;
> +	int ret;
> +
> +	npath = make_temp("filecache-");

No dash suffix needed in "filecache-". The make_temp() function already
adds a dash separator.

Kind regards,
Stefan

> +
> +	ret = copy_file(path, npath, 0);
> +	if (ret) {
> +		free(npath);
> +		return ret;
> +	}
> +
> +	*newpath = npath;
> +
> +	return 0;
> +}
> -- 
> 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

Re: [PATCH 5/7] uimage: Use is_tftp_fs() and cache_file() to ease TFTP workaround

[Stefan Lengfeld]

Hi Sascha,

On Wed, Jan 24, 2018 at 08:45:32AM +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>
> ---
>  common/uimage.c | 47 ++++++++++++++++++++++-------------------------
>  include/image.h |  1 +
>  2 files changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/common/uimage.c b/common/uimage.c
> index b6f0f109ca..18bc2e5d7b 100644
> --- a/common/uimage.c
> +++ b/common/uimage.c
> @@ -85,8 +85,6 @@ ssize_t uimage_get_size(struct uimage_handle *handle, unsigned int image_no)
>  }
>  EXPORT_SYMBOL(uimage_get_size);
>  
> -static const char uimage_tmp[] = "/.uImage_tmp";
> -
>  /*
>   * open a uimage. This will check the header contents and
>   * return a handle to the uImage
> @@ -99,32 +97,26 @@ struct uimage_handle *uimage_open(const char *filename)
>  	struct image_header *header;
>  	int i;
>  	int ret;
> -	struct stat s;
> +	char *copy = NULL;
> +
> +	if (is_tftp_fs(filename)) {
> +		ret = cache_file(filename, &copy);
> +		if (ret)
> +			return NULL;
> +		filename = copy;
> +	}
>  
> -again:
>  	fd = open(filename, O_RDONLY);
>  	if (fd < 0) {
>  		printf("could not open: %s\n", errno_str());

Leaking the allocated string in 'copy' here. Missing code:

                free(copy);

>  		return NULL;
>  	}
>  
> -	/*
> -	 * 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(filename, uimage_tmp, 0);
> -		if (ret)
> -			return NULL;
> -		filename = uimage_tmp;
> -		goto again;
> -	}
> -
>  	handle = xzalloc(sizeof(struct uimage_handle));
>  	header = &handle->header;
>  
> +	handle->copy = copy;
> +
>  	if (read(fd, header, sizeof(*header)) < 0) {
>  		printf("could not read: %s\n", errno_str());
>  		goto err_out;
> @@ -202,9 +194,13 @@ again:
>  	return handle;
>  err_out:
>  	close(fd);
> +
> +	if (handle->copy) {
> +		unlink(handle->copy);
> +		free(handle->copy);
> +	}

Not introduced by this patch, but here the line 

        free(handle->name);

is missing. The function uimage_close() below frees the string in 'name'
correctly, but the free call is absent in this error path.

Kind regards,
Stefan

>  	free(handle);
> -	if (IS_BUILTIN(CONFIG_FS_TFTP) && !stat(uimage_tmp, &s))
> -		unlink(uimage_tmp);
> +
>  	return NULL;
>  }
>  EXPORT_SYMBOL(uimage_open);
> @@ -214,14 +210,15 @@ EXPORT_SYMBOL(uimage_open);
>   */
>  void uimage_close(struct uimage_handle *handle)
>  {
> -	struct stat s;
> -
>  	close(handle->fd);
> +
> +	if (handle->copy) {
> +		unlink(handle->copy);
> +		free(handle->copy);
> +	}
> +
>  	free(handle->name);
>  	free(handle);
> -
> -	if (IS_BUILTIN(CONFIG_FS_TFTP) && !stat(uimage_tmp, &s))
> -		unlink(uimage_tmp);
>  }
>  EXPORT_SYMBOL(uimage_close);
>  
> diff --git a/include/image.h b/include/image.h
> index 3e75d49b88..add9c85874 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -246,6 +246,7 @@ struct resource *file_to_sdram(const char *filename, unsigned long adr);
>  struct uimage_handle {
>  	struct image_header header;
>  	char *name;
> +	char *copy;
>  	struct uimage_handle_data ihd[MAX_MULTI_IMAGE_COUNT];
>  	int nb_data_entries;
>  	size_t data_offset;
> -- 
> 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

Re: [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@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

Re: [PATCH 2/7] fs: implement is_tftp_fs()

[Sascha Hauer]

Hi Stefan,

Thanks for reviewing. I appreciate getting feedback. A new series is
out.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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