Re: [PATCH v5 01/12] sequencer: avoid using errno clobbered by rollback_lock_file()

[Johannes Schindelin]

Hi Martin,

On Tue, 27 Feb 2018, Martin Ågren wrote:

> On 26 February 2018 at 22:29, Johannes Schindelin
>  wrote:
> > As pointed out in a review of the `--recreate-merges` patch series,
> > `rollback_lock_file()` clobbers errno. Therefore, we have to report the
> > error message that uses errno before calling said function.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index e9baaf59bd9..5aa3dc3c95c 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -345,12 +345,14 @@ static int write_message(const void *buf, size_t len, 
> > const char *filename,
> > if (msg_fd < 0)
> > return error_errno(_("could not lock '%s'"), filename);
> > if (write_in_full(msg_fd, buf, len) < 0) {
> > +   error_errno(_("could not write to '%s'"), filename);
> > rollback_lock_file(_file);
> > -   return error_errno(_("could not write to '%s'"), filename);
> > +   return -1;
> > }
> > if (append_eol && write(msg_fd, "\n", 1) < 0) {
> > +   error_errno(_("could not write eol to '%s'"), filename);
> > rollback_lock_file(_file);
> > -   return error_errno(_("could not write eol to '%s'"), 
> > filename);
> > +   return -1;
> > }
> > if (commit_lock_file(_file) < 0) {
> > rollback_lock_file(_file);
> > @@ -2106,16 +2108,17 @@ static int save_head(const char *head)
> >
> > fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
> > if (fd < 0) {
> > +   error_errno(_("could not lock HEAD"));
> > rollback_lock_file(_lock);
> > -   return error_errno(_("could not lock HEAD"));
> > +   return -1;
> > }
> 
> I just noticed this when test-merging my series of lockfile-fixes to pu.
> This `rollback_lock_file()` is not needed, since failure to take the
> lock leaves it unlocked. If one wants to roll back the lock "for
> clarity" or "just to be safe", then the same should arguably be done in
> `write_message()`, just barely visible at the top of this diff.
> 
> Perhaps not worth a reroll. The conflict resolution between this and my
> patch would be to take my hunk.
> 
> https://public-inbox.org/git/cover.1519763396.git.martin.ag...@gmail.com/T/#t

Thank you for working on this!
Dscho

Re: [PATCH v5 01/12] sequencer: avoid using errno clobbered by rollback_lock_file()

[Martin Ågren]

On 26 February 2018 at 22:29, Johannes Schindelin
 wrote:
> As pointed out in a review of the `--recreate-merges` patch series,
> `rollback_lock_file()` clobbers errno. Therefore, we have to report the
> error message that uses errno before calling said function.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd9..5aa3dc3c95c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -345,12 +345,14 @@ static int write_message(const void *buf, size_t len, 
> const char *filename,
> if (msg_fd < 0)
> return error_errno(_("could not lock '%s'"), filename);
> if (write_in_full(msg_fd, buf, len) < 0) {
> +   error_errno(_("could not write to '%s'"), filename);
> rollback_lock_file(_file);
> -   return error_errno(_("could not write to '%s'"), filename);
> +   return -1;
> }
> if (append_eol && write(msg_fd, "\n", 1) < 0) {
> +   error_errno(_("could not write eol to '%s'"), filename);
> rollback_lock_file(_file);
> -   return error_errno(_("could not write eol to '%s'"), 
> filename);
> +   return -1;
> }
> if (commit_lock_file(_file) < 0) {
> rollback_lock_file(_file);
> @@ -2106,16 +2108,17 @@ static int save_head(const char *head)
>
> fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
> if (fd < 0) {
> +   error_errno(_("could not lock HEAD"));
> rollback_lock_file(_lock);
> -   return error_errno(_("could not lock HEAD"));
> +   return -1;
> }

I just noticed this when test-merging my series of lockfile-fixes to pu.
This `rollback_lock_file()` is not needed, since failure to take the
lock leaves it unlocked. If one wants to roll back the lock "for
clarity" or "just to be safe", then the same should arguably be done in
`write_message()`, just barely visible at the top of this diff.

Perhaps not worth a reroll. The conflict resolution between this and my
patch would be to take my hunk.

https://public-inbox.org/git/cover.1519763396.git.martin.ag...@gmail.com/T/#t

Martin

[PATCH v5 01/12] sequencer: avoid using errno clobbered by rollback_lock_file()

[Johannes Schindelin]

As pointed out in a review of the `--recreate-merges` patch series,
`rollback_lock_file()` clobbers errno. Therefore, we have to report the
error message that uses errno before calling said function.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e9baaf59bd9..5aa3dc3c95c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -345,12 +345,14 @@ static int write_message(const void *buf, size_t len, 
const char *filename,
if (msg_fd < 0)
return error_errno(_("could not lock '%s'"), filename);
if (write_in_full(msg_fd, buf, len) < 0) {
+   error_errno(_("could not write to '%s'"), filename);
rollback_lock_file(_file);
-   return error_errno(_("could not write to '%s'"), filename);
+   return -1;
}
if (append_eol && write(msg_fd, "\n", 1) < 0) {
+   error_errno(_("could not write eol to '%s'"), filename);
rollback_lock_file(_file);
-   return error_errno(_("could not write eol to '%s'"), filename);
+   return -1;
}
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
@@ -2106,16 +2108,17 @@ static int save_head(const char *head)
 
fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
if (fd < 0) {
+   error_errno(_("could not lock HEAD"));
rollback_lock_file(_lock);
-   return error_errno(_("could not lock HEAD"));
+   return -1;
}
strbuf_addf(, "%s\n", head);
written = write_in_full(fd, buf.buf, buf.len);
strbuf_release();
if (written < 0) {
+   error_errno(_("could not write to '%s'"), git_path_head_file());
rollback_lock_file(_lock);
-   return error_errno(_("could not write to '%s'"),
-  git_path_head_file());
+   return -1;
}
if (commit_lock_file(_lock) < 0) {
rollback_lock_file(_lock);
-- 
2.16.1.windows.4