Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user

[Max Gautier <mg@xxxxxxxxxxxxxxxx>]

On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>  
> +/*
> + * TODO: in the future (~2026 ?) remove this cleanup code
> + */
> +static void systemd_delete_user_unit(char const *unit)
> +{
> +	char const	file_start_stale[] =	"# This file was created and is"
> +						" maintained by Git.";
> +	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
> +
> +	char *filename = xdg_config_home_for("systemd/user", unit);
> +	int handle = open(filename, O_RDONLY);
> +
> +	/*
> +	 * Check this is actually our file and we're not removing a legitimate
> +	 * user override.
> +	 */
> +	if (handle == -1 && !is_missing_file_error(errno))
> +		warning(_("failed to delete '%s'"), filename);
> +	else {
> +		read(handle, file_start_user, sizeof(file_start_stale) - 1);

Actually that fails -Werror because I don't check read return.
Alternative below (on top of this one), with one question:
Are VLA using size_t const OK ? It's folded to a constant array by gcc
but I don't know if that causes portability problem with other platforms
? I can always repeat the sizeof expr if it's a problematic construct.

-- >8 --

diff --git a/builtin/gc.c b/builtin/gc.c
index 99b158e481..7fb25ea2b1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)
 /*
  * TODO: in the future (~2026 ?) remove this cleanup code
  */
+
 static void systemd_delete_user_unit(char const *unit)
 {
 	char const	file_start_stale[] =	"# This file was created and is"
 						" maintained by Git.";
-	char		file_start_user[sizeof(file_start_stale)] = {'\0'};
+	size_t const	length = sizeof(file_start_stale);
+	char		file_start_user[length] = {'\0'};
+
 
 	char *filename = xdg_config_home_for("systemd/user", unit);
 	int handle = open(filename, O_RDONLY);
@@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)
 	if (handle == -1 && !is_missing_file_error(errno))
 		warning(_("failed to delete '%s'"), filename);
 	else {
-		read(handle, file_start_user, sizeof(file_start_stale) - 1);
-		close(handle);
-		if (strcmp(file_start_stale, file_start_user) == 0) {
+		if (length - 1 == read(handle, file_start_user, length - 1) &&
+				strcmp(file_start_stale, file_start_user) == 0) {
 			if (unlink(filename) == 0)
 				warning(_("deleted stale unit file '%s'"), filename);
 			else if (!is_missing_file_error(errno))
 				warning(_("failed to delete '%s'"), filename);
 		}
+		close(handle);
 	}
 
 	free(filename);

-- 
Max Gautier