[PATCH v1] erofs-utils:code clean of write file

[Gao Xiang]

Hi Guifu,

Sorry for some delay, I'm busying in developping new fixed-sized output
LZMA library approach now (I think I'm fully understand LZMA internals,
hopefully in RFC shape and open source a preliminary effective
implementation next month, and I don't want to delay it too longer...)

Some changes are made as below (and a new version at the end of this
message and experimental branch). Comment as well if you have some
other opinions...

p.s. could you take some time looking at the requirement, thanks a lot!
https://lore.kernel.org/r/CAEvUa7=N7qUobof=vwpXF2XfXcW8R67SB3KV1phRN2ZmG23CvQ@mail.gmail.com/

On Wed, Dec 18, 2019 at 11:52:37PM +0800, Li Guifu wrote:
> From: Li Guifu <bluce.liguifu at huawei.com>
> 
> Make a code clean at function erofs_write_file() which
> has multi jump.

I've rewriten the commit message.

> 
> Signed-off-by: Li Guifu <blucerlee at gmail.com>
> ---
>  lib/inode.c | 63 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 0e19b11..052315a 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -302,22 +302,10 @@ static bool erofs_file_is_compressible(struct erofs_inode *inode)
>  	return true;
>  }
>  
> -int erofs_write_file(struct erofs_inode *inode)
> +int erofs_write_file_by_fd(int fd, struct erofs_inode *inode)

I've rename the function to write_uncompressed_file_from_fd and
change the variable order.

>  {
> +	int ret;
>  	unsigned int nblocks, i;
> -	int ret, fd;
> -
> -	if (!inode->i_size) {
> -		inode->datalayout = EROFS_INODE_FLAT_PLAIN;
> -		return 0;
> -	}
> -
> -	if (cfg.c_compr_alg_master && erofs_file_is_compressible(inode)) {
> -		ret = erofs_write_compressed_file(inode);
> -
> -		if (!ret || ret != -ENOSPC)
> -			return ret;
> -	}
>  
>  	/* fallback to all data uncompressed */
>  	inode->datalayout = EROFS_INODE_FLAT_INLINE;
> @@ -327,47 +315,58 @@ int erofs_write_file(struct erofs_inode *inode)
>  	if (ret)
>  		return ret;
>  
> -	fd = open(inode->i_srcpath, O_RDONLY | O_BINARY);
> -	if (fd < 0)
> -		return -errno;
> -
>  	for (i = 0; i < nblocks; ++i) {
>  		char buf[EROFS_BLKSIZ];
>  
>  		ret = read(fd, buf, EROFS_BLKSIZ);
> -		if (ret != EROFS_BLKSIZ) {
> -			if (ret < 0)
> -				goto fail;
> -			close(fd);
> -			return -EAGAIN;
> -		}
> +		if (ret != EROFS_BLKSIZ)
> +			return -errno;

I'd suggest the original approach.

Thanks,
Gao Xiang

>  
>  		ret = blk_write(buf, inode->u.i_blkaddr + i, 1);
>  		if (ret)
> -			goto fail;
> +			return ret;
>  	}
>  
>  	/* read the tail-end data */
>  	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
>  	if (inode->idata_size) {
>  		inode->idata = malloc(inode->idata_size);
> -		if (!inode->idata) {
> -			close(fd);
> +		if (!inode->idata)
>  			return -ENOMEM;
> -		}
>  
>  		ret = read(fd, inode->idata, inode->idata_size);
>  		if (ret < inode->idata_size) {
>  			free(inode->idata);
>  			inode->idata = NULL;
> -			close(fd);
>  			return -EIO;
>  		}
>  	}
> -	close(fd);
> +
>  	return 0;
> -fail:
> -	ret = -errno;
> +}
> +
> +int erofs_write_file(struct erofs_inode *inode)
> +{
> +	int ret, fd;
> +
> +	if (!inode->i_size) {
> +		inode->datalayout = EROFS_INODE_FLAT_PLAIN;
> +		return 0;
> +	}
> +
> +	if (cfg.c_compr_alg_master && erofs_file_is_compressible(inode)) {
> +		ret = erofs_write_compressed_file(inode);
> +
> +		if (!ret || ret != -ENOSPC)
> +			return ret;
> +	}
> +
> +	fd = open(inode->i_srcpath, O_RDONLY | O_BINARY);
> +	if (fd < 0)
> +		return -errno;
> +
> +	ret = erofs_write_file_by_fd(fd, inode);
> +
>  	close(fd);
>  	return ret;
>  }
> -- 
> 2.17.1
> 


>From 11302fc4dc5d53a7730405765828a744aee114f6 Mon Sep 17 00:00:00 2001
From: Li Guifu <blucerlee at gmail.com>
Date: Wed, 18 Dec 2019 23:52:37 +0800
Subject: [PATCH] erofs-utils: clean up erofs_write_file()

Introduce write_uncompressed_file_from_fd() to make
error handling path in erofs_write_file() clearer.

Signed-off-by: Li Guifu <blucerlee at gmail.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 lib/inode.c | 58 ++++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index 0e19b11..bd0652b 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -302,24 +302,11 @@ static bool erofs_file_is_compressible(struct erofs_inode *inode)
 	return true;
 }
 
-int erofs_write_file(struct erofs_inode *inode)
+static int write_uncompressed_file_from_fd(struct erofs_inode *inode, int fd)
 {
+	int ret;
 	unsigned int nblocks, i;
-	int ret, fd;
 
-	if (!inode->i_size) {
-		inode->datalayout = EROFS_INODE_FLAT_PLAIN;
-		return 0;
-	}
-
-	if (cfg.c_compr_alg_master && erofs_file_is_compressible(inode)) {
-		ret = erofs_write_compressed_file(inode);
-
-		if (!ret || ret != -ENOSPC)
-			return ret;
-	}
-
-	/* fallback to all data uncompressed */
 	inode->datalayout = EROFS_INODE_FLAT_INLINE;
 	nblocks = inode->i_size / EROFS_BLKSIZ;
 
@@ -327,47 +314,60 @@ int erofs_write_file(struct erofs_inode *inode)
 	if (ret)
 		return ret;
 
-	fd = open(inode->i_srcpath, O_RDONLY | O_BINARY);
-	if (fd < 0)
-		return -errno;
-
 	for (i = 0; i < nblocks; ++i) {
 		char buf[EROFS_BLKSIZ];
 
 		ret = read(fd, buf, EROFS_BLKSIZ);
 		if (ret != EROFS_BLKSIZ) {
 			if (ret < 0)
-				goto fail;
-			close(fd);
+				return -errno;
 			return -EAGAIN;
 		}
 
 		ret = blk_write(buf, inode->u.i_blkaddr + i, 1);
 		if (ret)
-			goto fail;
+			return ret;
 	}
 
 	/* read the tail-end data */
 	inode->idata_size = inode->i_size % EROFS_BLKSIZ;
 	if (inode->idata_size) {
 		inode->idata = malloc(inode->idata_size);
-		if (!inode->idata) {
-			close(fd);
+		if (!inode->idata)
 			return -ENOMEM;
-		}
 
 		ret = read(fd, inode->idata, inode->idata_size);
 		if (ret < inode->idata_size) {
 			free(inode->idata);
 			inode->idata = NULL;
-			close(fd);
 			return -EIO;
 		}
 	}
-	close(fd);
 	return 0;
-fail:
-	ret = -errno;
+}
+
+int erofs_write_file(struct erofs_inode *inode)
+{
+	int ret, fd;
+
+	if (!inode->i_size) {
+		inode->datalayout = EROFS_INODE_FLAT_PLAIN;
+		return 0;
+	}
+
+	if (cfg.c_compr_alg_master && erofs_file_is_compressible(inode)) {
+		ret = erofs_write_compressed_file(inode);
+
+		if (!ret || ret != -ENOSPC)
+			return ret;
+	}
+
+	/* fallback to all data uncompressed */
+	fd = open(inode->i_srcpath, O_RDONLY | O_BINARY);
+	if (fd < 0)
+		return -errno;
+
+	ret = write_uncompressed_file_from_fd(inode, fd);
 	close(fd);
 	return ret;
 }
-- 
2.20.1