Re: [PATCH v5 3/3] maintenance: add support for systemd timers on Linux

From: Jeff King <peff@peff.net>
Date: Jun 9, 2021, 09:34 UTC
Message-ID: <YMCLQ5pOyg+SSKhD@coredump.intra.peff.net>
In-Reply-To: <20210608134000.663398-4-lenaic@lhuard.fr>

On Tue, Jun 08, 2021 at 03:40:00PM +0200, Lénaïc Huard wrote:

Show 15 quoted lines

> +static int systemd_timer_write_unit_templates(const char *exec_path)
> +{
> +	char *filename;
> +	FILE *file;
> +	const char *unit;
> +
> +	filename = xdg_config_home_systemd("git-maintenance@.timer");
> +	if (safe_create_leading_directories(filename)) {
> +		error(_("failed to create directories for '%s'"), filename);
> +		goto error;
> +	}
> +	file = fopen_or_warn(filename, "w");
> +	if (file == NULL)
> +		goto error;
> +	FREE_AND_NULL(filename);

Here we free the filename variable. But later...

Show 22 quoted lines

> +	unit = "# This file was created and is maintained by Git.\n"
> +	       "# Any edits made in this file might be replaced in the future\n"
> +	       "# by a Git command.\n"
> +	       "\n"
> +	       "[Unit]\n"
> +	       "Description=Optimize Git repositories data\n"
> +	       "\n"
> +	       "[Timer]\n"
> +	       "OnCalendar=%i\n"
> +	       "Persistent=true\n"
> +	       "\n"
> +	       "[Install]\n"
> +	       "WantedBy=timers.target\n";
> +	if (fputs(unit, file) == EOF) {
> +		error(_("failed to write to '%s'"), filename);
> +		fclose(file);
> +		goto error;
> +	}
> +	if (fclose(file) == EOF) {
> +		error_errno(_("failed to flush '%s'"), filename);
> +		goto error;
> +	}

If we see an error we'll try to use it as part of the message. I think the FREE_AND_NULL() can just be moved down here. And really just be free(), since we then immediately reassign it:

Show 5 quoted lines

> +	filename = xdg_config_home_systemd("git-maintenance@.service");
> +	file = fopen_or_warn(filename, "w");
> +	if (file == NULL)
> +		goto error;
> +	FREE_AND_NULL(filename);

And then this one has the same problem. We free here, but...

Show 28 quoted lines

> +	unit = "# This file was created and is maintained by Git.\n"
> +	       "# Any edits made in this file might be replaced in the future\n"
> +	       "# by a Git command.\n"
> +	       "\n"
> +	       "[Unit]\n"
> +	       "Description=Optimize Git repositories data\n"
> +	       "\n"
> +	       "[Service]\n"
> +	       "Type=oneshot\n"
> +	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
> +	       "LockPersonality=yes\n"
> +	       "MemoryDenyWriteExecute=yes\n"
> +	       "NoNewPrivileges=yes\n"
> +	       "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6\n"
> +	       "RestrictNamespaces=yes\n"
> +	       "RestrictRealtime=yes\n"
> +	       "RestrictSUIDSGID=yes\n"
> +	       "SystemCallArchitectures=native\n"
> +	       "SystemCallFilter=@system-service\n";
> +	if (fprintf(file, unit, exec_path, exec_path) < 0) {
> +		error(_("failed to write to '%s'"), filename);
> +		fclose(file);
> +		goto error;
> +	}
> +	if (fclose(file) == EOF) {
> +		error_errno(_("failed to flush '%s'"), filename);
> +		goto error;
> +	}

...use it in the error messages. This one could also just be free() before we return:

Show 7 quoted lines

> +	return 0;
> +
> +error:
> +	free(filename);
> +	systemd_timer_delete_unit_templates();
> +	return -1;
> +}

And all of the jumps to the error label are fine, since it frees the filename (and we don't have to worry about FREE_AND_NULL, since it would always be valid during those jumps).

-Peff