On Thu, Aug 08, 2024 at 07:14:15AM -0500, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
> > + /*
> > + * We have found the first entry. Verify that all the
> > + * subsequent tables we have compacted still exist in
> > + * the modified stack in the exact same order as we
> > + * have compacted them.
> > + */
> > + for (size_t j = 1; j < last - first + 1; j++) {
> > + const char *old = first + j < st->merged->stack_len ?
> > + st->readers[first + j]->name : NULL;
> > + const char *new = names[i + j];
> > +
> > + /*
> > + * If some entries are missing or in case the tables
> > + * have changed then we need to bail out. Again, this
> > + * shouldn't ever happen because we have locked the
> > + * tables we are compacting.
> > + */
>
> Okay, this is exactly what I was saying above. It still does makes sense
> to keep this check to ensure future versions don't break it.
Yeah, exactly. It's mostly about defense in depth, but should in theory
never be needed.
> > + if (!old || !new || strcmp(old, new)) {
> > + err = REFTABLE_OUTDATED_ERROR;
> > + goto done;
> > + }
> > + }
> > +
> > + new_offset = i;
> > + break;
> > + }
> > +
> > + /*
> > + * In case we didn't find our compacted tables in the stack we
> > + * need to bail out. In theory, this should have never happened
> > + * because we locked the tables we are compacting.
> > + */
> > + if (new_offset < 0) {
> > + err = REFTABLE_OUTDATED_ERROR;
> > + goto done;
> > + }
> > +
> > + /*
> > + * We have found the new range that we want to replace, so
> > + * let's update the range of tables that we want to replace.
> > + */
> > + last_to_replace = last + (new_offset - first);
> > + first_to_replace = new_offset;
>
> Nit: might be easier to read as
>
> first_to_replace = new_offset;
> last_to_replace = first_to_replace + (last - first);
True. Initially I didn't have the `first_to_replace` variables, but now
that I do it certainly makes it easier to order it naturally.
Patrick
Attachment:
signature.asc
Description: PGP signature
