Re: [PATCH v2 21/25] sequencer: refactor write_message()

Johannes Sixt [off-list ref] writes:

Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:

quoted

-static int write_message(struct strbuf *msgbuf, const char *filename)
+static int write_with_lock_file(const char *filename,
+				const void *buf, size_t len, int append_eol)
 {
 	static struct lock_file msg_file;

 	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
 	if (msg_fd < 0)
 		return error_errno(_("Could not lock '%s'"), filename);
-	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		return error_errno(_("Could not write to %s"), filename);
-	strbuf_release(msgbuf);
+	if (write_in_full(msg_fd, buf, len) < 0)
+		return error_errno(_("Could not write to '%s'"), filename);
+	if (append_eol && write(msg_fd, "\n", 1) < 0)
+		return error_errno(_("Could not write eol to '%s"), filename);
 	if (commit_lock_file(&msg_file) < 0)
 		return error(_("Error wrapping up %s."), filename);

 	return 0;
 }

The two error paths in the added lines should both

		rollback_lock_file(&msg_file);

, I think. But I do notice that this is not exactly new, so...

It may not be new for this step, but overall the series is aiming to
libify the stuff, so we should fix fd and lockfile leaks like this
as we notice them.

Thanks.

Hi Junio & Hannes,

On Thu, 15 Sep 2016, Junio C Hamano wrote:

Johannes Sixt [off-list ref] writes:

quoted

Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:

quoted

-static int write_message(struct strbuf *msgbuf, const char *filename)
+static int write_with_lock_file(const char *filename,
+				const void *buf, size_t len, int append_eol)
 {
 	static struct lock_file msg_file;

 	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
 	if (msg_fd < 0)
 		return error_errno(_("Could not lock '%s'"), filename);
-	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		return error_errno(_("Could not write to %s"), filename);
-	strbuf_release(msgbuf);
+	if (write_in_full(msg_fd, buf, len) < 0)
+		return error_errno(_("Could not write to '%s'"), filename);
+	if (append_eol && write(msg_fd, "\n", 1) < 0)
+		return error_errno(_("Could not write eol to '%s"), filename);
 	if (commit_lock_file(&msg_file) < 0)
 		return error(_("Error wrapping up %s."), filename);

 	return 0;
 }

The two error paths in the added lines should both

		rollback_lock_file(&msg_file);

, I think. But I do notice that this is not exactly new, so...

It may not be new for this step, but overall the series is aiming to
libify the stuff, so we should fix fd and lockfile leaks like this
as we notice them.

Makes sense, even for the final commit_lock_file().

Ciao,
Dscho