| Message ID | 20250312121148.1879604-3-meetsoni3017@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | reftable: return proper error codes from block_writer_add | expand |
On Wed, Mar 12, 2025 at 05:41:48PM +0530, Meet Soni wrote: > diff --git a/reftable/writer.c b/reftable/writer.c > index f3ab1035d6..0d8181e227 100644 > --- a/reftable/writer.c > +++ b/reftable/writer.c > @@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w, > * done. Otherwise the block writer may have hit the block size limit > * and needs to be flushed. > */ > - if (!block_writer_add(w->block_writer, rec)) { > - err = 0; > + err = block_writer_add(w->block_writer, rec); > + if (err == 0) > goto done; > - } Style: we'd typically say `if (!err)` here, even though I see that we have explicit comparisons with 0 elsewhere in this file, too. So I guess ultimately this is okay. > @@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w, > goto done; > > /* > - * Try to add the record to the writer again. If this still fails then > - * the record does not fit into the block size. > - * > - * TODO: it would be great to have `block_writer_add()` return proper > - * error codes so that we don't have to second-guess the failure > - * mode here. > + * Try to add the record to the writer again. > */ My comment on the preceding version still applies here: the second sentence (the one starting with "If this still fails...") should be retained. > err = block_writer_add(w->block_writer, rec); > - if (err) { > - err = REFTABLE_ENTRY_TOO_BIG_ERROR; > + if (err) > goto done; > - } > > done: > return err; > @@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key) > if (arg->err < 0) > goto done; > > + /* > + * Try to add the record to the writer. If this succeeds then we're > + * done. Otherwise the block writer may have hit the block size limit > + * and needs to be flushed. > + */ > arg->err = block_writer_add(arg->w->block_writer, &rec); > if (arg->err == 0) > goto done; > > + if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR) > + goto done; Good catch that there is another such pattern! > + /* > + * The current block is full, so we need to flush and reinitialize the > + * writer to start writing the next block. > + */ > arg->err = writer_flush_block(arg->w); > if (arg->err < 0) > goto done; But there is another case further down where we do `block_writer_add()` and then re-try in case the write fails. This one is a bit more curious: if the write fails, we don't create a new block -- after all we have just created one. Instead, we reset the record's offset length to zero before retrying. I _think_ that this is done because we know that when resetting the offset we would write less data to the block, as can be seen in `reftable_obj_record_encode()`. But I'm honestly not quite sure here as I haven't yet done a deep dive into object records -- after all, we don't even really use them in Git. In any case, I think that this callsite also needs adjustment and warrants a comment. And if so, all changes to `write_object_record()` should probably go into a separate commit, as well. Thanks! Patrick
diff --git a/reftable/writer.c b/reftable/writer.c index f3ab1035d6..0d8181e227 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w, * done. Otherwise the block writer may have hit the block size limit * and needs to be flushed. */ - if (!block_writer_add(w->block_writer, rec)) { - err = 0; + err = block_writer_add(w->block_writer, rec); + if (err == 0) goto done; - } + if (err != REFTABLE_ENTRY_TOO_BIG_ERROR) + goto done; /* * The current block is full, so we need to flush and reinitialize the * writer to start writing the next block. @@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w, goto done; /* - * Try to add the record to the writer again. If this still fails then - * the record does not fit into the block size. - * - * TODO: it would be great to have `block_writer_add()` return proper - * error codes so that we don't have to second-guess the failure - * mode here. + * Try to add the record to the writer again. */ err = block_writer_add(w->block_writer, rec); - if (err) { - err = REFTABLE_ENTRY_TOO_BIG_ERROR; + if (err) goto done; - } done: return err; @@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key) if (arg->err < 0) goto done; + /* + * Try to add the record to the writer. If this succeeds then we're + * done. Otherwise the block writer may have hit the block size limit + * and needs to be flushed. + */ arg->err = block_writer_add(arg->w->block_writer, &rec); if (arg->err == 0) goto done; + if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR) + goto done; + + /* + * The current block is full, so we need to flush and reinitialize the + * writer to start writing the next block. + */ arg->err = writer_flush_block(arg->w); if (arg->err < 0) goto done;
Previously, writer_add_record() and write_object_record() would flush the current block and retry appending the record whenever block_writer_add() returned any nonzero error. This forced an assumption that every failure meant the block was full, even when errors such as memory allocation or I/O failures occurred. Update the writer code to inspect the error code returned by block_writer_add() and only flush and reinitialize the writer when the error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately propagate it. All call sites now handle various error codes returned by block_writer_add(). Signed-off-by: Meet Soni <meetsoni3017@gmail.com> --- reftable/writer.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)