lib: Move validation out of lcfs_node_add_child() #341
Conversation
cgwalters
force-pushed
the
fix-regress-uninit-mode
branch
from
ca17c97
to
6290705
Compare
|
Ah yeah of course since this is C a ~5 line unit test has a double-unref, thankfully caught by ASAN. |
There was a problem hiding this comment.
The patch looks fine as it is but it's sort of clear that the API use by ostree is not what was intended here...
Would it be super disruptive to assert that it's invalid to make changes to the node after adding it to the parent? I guess that would more or less be an ABI break and need an soname bump, right? I'm thinking about the recent _rdev64() changes... What's our guarantee here?
|
Yeah, agree. Here's one thing I've been thinking about as followups to all this:
Actually we could just add
Right, I don't want to scope that into this at this time. It would be painful. |
This sounds nice. Is your plan to land this PR, add those APIs, then back out the rdev64 setter, then deprecate the existing |
|
My hope was to land this PR basically as is alongside #342 and get a new release out and leave other things to followup. |
|
Adding a new API is going to need more tests, and we'd need to rework the fuzzing entrypoint to support both the old and new API, which is quite doable but also not trivial, so I'd rather get fixes out for what we have now as a release. |
| @@ -390,12 +390,6 @@ int lcfs_write_to(struct lcfs_node_s *root, struct lcfs_write_options_s *options | |||
| struct lcfs_ctx_s *ctx; | |||
| int res; | |||
|
|
|||
| // Verify the root; because lcfs_node_add_child also verifies children, | |||
| // we should have sanity checked all nodes. | |||
| if (lcfs_node_last_ditch_validation(root) < 0) { | |||
There was a problem hiding this comment.
One very minor nag: we handle validation of the node from the main writer code currently but your patch moves it to the erofs implementation code. Of course, there are no other implementations... but it seems a bit odd.
I guess the implementation is the thing iterating the nodes in the end, though, so there's not really much of another way...
There was a problem hiding this comment.
Of course, there are no other implementations... but it seems a bit odd.
Yeah the current code structure is AIUI a legacy of the time when there were two composefs implementations - one kernel native, one using the architecture today.
It probably makes sense to fold some of lcfs-writer.c and lcfs-writer-erofs.c - but I wouldn't say all?
Anyways yeah, AFAICS the place I moved the validation is the only place we can do it right now.
Unfortunately at least ostree does: node = lcfs_node_new() lcfs_node_add_child(parent, node); lcfs_node_set_mode(node, ...) So we basically have a completely uninitialized node as part of the tree. This failed our basic "validate the mode" logic. We can't do any validation at all in `lcfs_node_add_child()` so move it to the first time we walk the tree as part of `lcfs_node_write_to()`. Also as part of fixing this: Today we don't really have coverage of the C library as it may be used by external consumers. Add a unit test to verify this, and we can build on this more. Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
fix-regress-uninit-mode
branch
from
6290705
to
b8a238c
Compare
| { | ||
| char *bufp = NULL; | ||
| size_t bufsz = 0; | ||
| FILE *buf = open_memstream(&bufp, &bufsz); |
There was a problem hiding this comment.
The memstream was more than I was expecting!
| options.file_write_cb = write_cb; | ||
|
|
||
| int r = lcfs_write_to(node, &options); | ||
| int saved_errno = errno; |
There was a problem hiding this comment.
It took me a real long time to convince myself that this was right, but indeed we set errno = EINVAL in the mode-validation code. Is there some story behind the various approaches to error handling in libcomposefs? The mount code seems to prefer returning negative errnos, at least internally...
There was a problem hiding this comment.
The mount code seems to prefer returning negative errnos, at least internally...
It's quite possible that I or someone else messed up the sign at some point 😢
There was a problem hiding this comment.
I mean that instead of
errno = EINVAL;
return -1;you get kernel/systemd-style
return -EINVAL;I don't think anyone has ever returned positive errno values.... as far as I know :)
Unfortunately at least ostree does:
node = lcfs_node_new()
lcfs_node_add_child(parent, node);
lcfs_node_set_mode(node, ...)
So we basically have a completely uninitialized node as part of the tree.
This failed our basic "validate the mode" logic.
We can't do any validation at all in
lcfs_node_add_child()so move it to the first time we walk the tree as part oflcfs_node_write_to().Also as part of fixing this: Today we don't really have coverage of the C library as it may be used by external consumers. Add a unit test to verify this, and we can build on this more.