Generalize traversing a Clevis config#3708
Generalize traversing a Clevis config#3708mulkieran wants to merge 2 commits intostratis-storage:masterfrom
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
2 similar comments
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
There is some way to do the recursive call non-greedily... |
mulkieran
force-pushed
the
generalize_clevis_config_traverse
branch
3 times, most recently
from
c4182ea to
ffcd83b
Compare
|
The challenge problem is to turn the |
mulkieran
force-pushed
the
generalize_clevis_config_traverse
branch
2 times, most recently
from
67f797f to
d82b208
Compare
|
Cockpit tests failed for commit ffcd83b. @martinpitt, @jelly, @mvollmer please check. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
mulkieran
force-pushed
the
generalize_clevis_config_traverse
branch
from
d82b208 to
0c0c5df
Compare
jbaublitz
left a comment
jbaublitz
left a comment
There was a problem hiding this comment.
I'm not entirely clear on some of the design choices here.
| tang_func: &dyn Fn(&Map<String, Value>) -> StratisResult<T>, | ||
| tpm2_func: &dyn Fn(&Map<String, Value>) -> StratisResult<T>, | ||
| sss: &mut (&T, &mut dyn FnMut(T, T) -> StratisResult<T>), |
There was a problem hiding this comment.
Can you explain why you made these references? I don't see that pattern used in most closure usage in the Rust code I've seen. My understanding is that Fn() itself already represents a reference.
| recursion_limit - 1, | ||
| )?, | ||
| ) | ||
| obj.iter().try_fold(sss.0.clone(), |acc, (pin, config)| { |
There was a problem hiding this comment.
Can you explain why you opted for a reference here? My brief analysis of the ownership here leads me to believe that a move and an owned data type would be okay here.
There was a problem hiding this comment.
I can change sss.0 so that it is not a reference. I've added a commit for that. But you can't move sss into the closure, because it is used after the closure returns. So, AFAICT, if you add a move, you have to be able to clone sss.
There was a problem hiding this comment.
I think this gets back to my other comment: why do all of these need to be references? I feel like ownership may remove the need for cloning.
There was a problem hiding this comment.
sss is recursively passed and then used on the return value. I don't believe that it can be moved into the closure for that reason.
Retain laziness in traversing the structure and early exit on error. Signed-off-by: mulhern <amulhern@redhat.com>
Note that it is still necessary to clone it, to pass as the argument to the recursive call _and_ to use as the base for a fold. Signed-off-by: mulhern <amulhern@redhat.com>
mulkieran
force-pushed
the
generalize_clevis_config_traverse
branch
from
0c0c5df to
818b9e9
Compare
jbaublitz
left a comment
jbaublitz
left a comment
There was a problem hiding this comment.
Just to give a little bit of context, my main concern here is that we don't yet have a reason to generalize this function. Because of the complexity this adds to the traversal, I generally would want to see a case where this generalization would be needed to avoid code duplication. If this is the only code using the abstraction, I hesitate to introduce the complexity just yet as I believe this version is harder to read upfront.
After playing with the code a little bit I see why you have written it the way you have. I don't see a much better way after trying quite a few different variations.
Do you have any concrete plans to use this abstraction or are you doing it out of principle? If there's no current foreseen usage of this, I would vote that we hold off on this until we bump into a situation where it's necessary.
I would have to say that this is a judgement that is essentially subjective.
I am happy to let this PR sit until the next time that there is some need to traverse the Clevis config. But I would also consider implementing something similar if we have the general need to recursively traverse anything else. |
2 participants
No description provided.