Re: [PATCH 4/9] spaceman/defrag: ctrl-c handler

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jul 09, 2024 at 12:10:23PM -0700, Wengang Wang wrote:
> Add this handler to break the defrag better, so it has
> 1. the stats reporting
> 2. remove the temporary file
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  spaceman/defrag.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/spaceman/defrag.c b/spaceman/defrag.c
> index 9f11e36b..61e47a43 100644
> --- a/spaceman/defrag.c
> +++ b/spaceman/defrag.c
> @@ -297,6 +297,13 @@ get_time_delta_us(struct timeval *pre_time, struct timeval *cur_time)
>  	return us;
>  }
>  
> +static volatile bool usedKilled = false;
> +void defrag_sigint_handler(int dummy)
> +{
> +	usedKilled = true;

Not sure why some of these variables are camelCase and others not.
Or why this global variable doesn't have a g_ prefix like the others?

> +	printf("Please wait until current segment is defragmented\n");

Is it actually safe to call printf from a signal handler?  Handlers must
be very careful about what they call -- regreSSHion was a result of
openssh not getting this right.

(Granted spaceman isn't as critical...)

Also would you rather SIGINT merely terminate the spaceman process?  I
think the file locks drop on termination, right?

--D

> +};
> +
>  /*
>   * defragment a file
>   * return 0 if successfully done, 1 otherwise
> @@ -345,6 +352,8 @@ defrag_xfs_defrag(char *file_path) {
>  		goto out;
>  	}
>  
> +	signal(SIGINT, defrag_sigint_handler);
> +
>  	do {
>  		struct timeval t_clone, t_unshare, t_punch_hole;
>  		struct defrag_segment segment;
> @@ -434,7 +443,7 @@ defrag_xfs_defrag(char *file_path) {
>  		if (time_delta > max_punch_us)
>  			max_punch_us = time_delta;
>  
> -		if (stop)
> +		if (stop || usedKilled)
>  			break;
>  	} while (true);
>  out:
> -- 
> 2.39.3 (Apple Git-146)
> 
>