On 21/12/2024 03:21, Matthias van de Meent wrote:
On Sat, 21 Dec 2024 at 01:05, Thomas Munro <[email protected]> wrote:
The patch does not seem unreasonable and I'd like to help tidy this
up, but ... hmm, could we also consider going the other way?
register_unlink_segment(), mdunlinkfiletag() and the macro that
populates md.c's FileTag are internal to md.c, and we don't expect
external code to be pushing md.c SYNC_UNLINK_REQUEST requests into the
request queue (who would do that and what could the motivation
possibly be?) Doesn't feel like a supported usage to me... So my
question is: what bad thing would happen if we just renamed
register_unlink_segment() to register_unlink_tombstone() without
fork/seg arguments, to make it clear that it's not really a general
purpose unreliable segment unlink mechanism that we want anyone to
build more stuff on top of?
I just noticed I misinterpreted the conditions in mdunlinkfork, so
that I thought it allowed a user to pass their own forknum into
register_unlink_segment (as that is called with the user-provided
forknum). Instead, that branch is only taken when forknum ==
MAIN_FORKNUM, so I think you might be right that going in the other
direction is more desirable.
In that case, something along the lines of the attached would then be
better - it removes the fork and segno from register_unlink_segment's
arguments (renamed to register_unlink_tombstone), and Asserts() that
mdunlinkfiletag only receives a FileTag that contains expected values.
Renaming the function and the assertion makes sense to me. Committed to
master, thanks!
- Heikki
