From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from stcim.de ([2a01:4f8:120:63a3::2]) by bombadil.infradead.org with esmtps (Exim 4.89 #1 (Red Hat Linux)) id 1eeR9r-0003Dj-97 for barebox@lists.infradead.org; Wed, 24 Jan 2018 19:56:33 +0000 Date: Wed, 24 Jan 2018 20:56:17 +0100 From: Stefan Lengfeld Message-ID: <20180124195617.GD521@porty> References: <20180124074534.7966-1-s.hauer@pengutronix.de> <20180124074534.7966-7-s.hauer@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180124074534.7966-7-s.hauer@pengutronix.de> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 6/7] fs/uimagefs: Use is_tftp_fs() and cache_file() to ease TFTP workaround To: Sascha Hauer Cc: Barebox List 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 > --- > 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