[PATCH] btrfs-progs: simplify mkfs_main cleanup

[PATCH] btrfs-progs: simplify mkfs_main cleanup

[Mark Harmstone]

mkfs_main is a main-like function, meaning that return and exit are
equivalent. Deduplicate code by adding a new out2 label.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
---
 mkfs/main.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/mkfs/main.c b/mkfs/main.c
index a69aa24b..5705acb6 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1915,6 +1915,8 @@ out:
 	}
 
 	btrfs_close_all_devices();
+
+out2:
 	if (prepare_ctx) {
 		for (i = 0; i < device_count; i++)
 			close(prepare_ctx[i].fd);
@@ -1927,15 +1929,9 @@ out:
 	return !!ret;
 
 error:
-	if (prepare_ctx) {
-		for (i = 0; i < device_count; i++)
-			close(prepare_ctx[i].fd);
-	}
-	free(t_prepare);
-	free(prepare_ctx);
-	free(label);
-	free(source_dir);
-	exit(1);
+	ret = 1;
+	goto out2;
+
 success:
 	exit(0);
 }
-- 
2.44.2

Re: [PATCH] btrfs-progs: simplify mkfs_main cleanup

[Josef Bacik]

On Fri, Jul 19, 2024 at 04:46:43PM +0100, Mark Harmstone wrote:
> mkfs_main is a main-like function, meaning that return and exit are
> equivalent. Deduplicate code by adding a new out2 label.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>  mkfs/main.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/mkfs/main.c b/mkfs/main.c
> index a69aa24b..5705acb6 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -1915,6 +1915,8 @@ out:
>  	}
>  
>  	btrfs_close_all_devices();
> +
> +out2:

Generally we don't like out2/out3 things, that makes it hard to figure out what
is going on, I'd rather it be like out_free.

However all error is doing here now is setting ret = 1 and going to out2.  At
this point you should just make sure ret is set in the places that call 'goto
error' and then move error to where out2 is.

If you're looking for even better cleanup semantics, I would do something like
the following

#define __free(func) __attribute__((cleanup(func)))

static inline void freep(void *ptr)
{
        void *real_ptr = *(void **)ptr;
        if (real_ptr == NULL)
                return;
        free(real_ptr);
}

to one of our common utils headers, and then do

pthread_t *t_prepare __free(freep) = NULL;
struct prepare_device_progress *prepare_ctx __free(freep) = NULL;
char *label __free(freep) = NULL;
char *source_dir __free(freep) = NULL;

and then you don't have to worry about the free stuff, and then you just have
the close part.

If you wanted to be even fancier you could also change how prepare_ctx is
allocated, so it has the number of devices in itself, and then the array of the
fd's, and you could add a custom cleanup function for that which would close all
the fd's and then free the memory.

But at the very least the out2 thing needs to be addressed.  The rest of these
thoughts are possible cleanup options that could be explored later.  Thanks,

Josef