Re: [PATCH 1/4] lockfile: report when rollback fails

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On Tue, Mar 05, 2024 at 04:09:07PM -0600, Justin Tobler wrote:
> On 24/03/04 12:10PM, Patrick Steinhardt wrote:
> > We do not report to the caller when rolling back a lockfile fails, which
> > will be needed by the reftable compaction logic in a subsequent commit.
> > It also cannot really report on all errors because the function calls
> > `delete_tempfile()`, which doesn't return an error either.
> > 
> > Refactor the code so that both `delete_tempfile()` and
> > `rollback_lock_file()` return an error code.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  lockfile.h |  4 ++--
> >  tempfile.c | 21 +++++++++++++--------
> >  tempfile.h |  2 +-
> >  3 files changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/lockfile.h b/lockfile.h
> > index 90af4e66b2..4ed570d3f7 100644
> > --- a/lockfile.h
> > +++ b/lockfile.h
> > @@ -323,9 +323,9 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
> >   * for a `lock_file` object that has already been committed or rolled
> >   * back.
> >   */
> > -static inline void rollback_lock_file(struct lock_file *lk)
> > +static inline int rollback_lock_file(struct lock_file *lk)
> >  {
> > -	delete_tempfile(&lk->tempfile);
> > +	return delete_tempfile(&lk->tempfile);
> >  }
> 
> question: For a lockfile that is already committed or rolled back, is
> the underlying tempfile still active? I'm trying to figure out if it
> possible for an error to be returned in this scenerio. If not, it might
> be nice to clarify in the comment above that, in addition to being a
> NOOP, no error is returned.

No, it's not. I thought that it being a no-op should be clear enough,
but on the other hand it doesn't hurt either to clarify even further.

I'll refrain from sending a v2 only to add this comment as there haven't
been any other things that need to be addressed yet.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]